Fix panic on malformed ACE binary by validating unmarshal lengths (#30)

[p0dalirius]

Linked Issue

Closes #30

Root Cause

Three leaf Unmarshal functions read their input via slice indexing without first validating len:

• ace/mask/AccessControlMask.go::Unmarshal at L33 reads marshalledData[:4].
• sid/authority/SecurityIdentifierAuthority.go::Unmarshal at L67-L69 reads marshalledData[0:2], [2:4], [4:6].
• object/flags/AccessControlObjectTypeFlags.go::Unmarshal at L27 reads rawBytes[0:4].

When a truncated ACE binary travels down the parse chain — for example an ACCESS_ALLOWED ACE whose Header.Size is exactly 4 (header only, no mask or SID), or an ACCESS_ALLOWED_OBJECT ACE with missing flag bytes — one of these leaves is eventually called with a slice shorter than the required field, causing runtime error: slice bounds out of range and crashing the process. This matches the stack signature in the report (ace/AccessControlEntry.go → DiscretionaryAccessControlList.Unmarshal → NtSecurityDescriptor.Unmarshal).

Fix Description

Validate len(input) at the top of each of the three leaf unmarshals and return a descriptive parse error on short input. No change to the successful-path behaviour or to marshalled output. The existing callers (AccessControlEntry.Unmarshal, SID.Unmarshal, AccessControlObjectType.Unmarshal) already propagate errors upward, so the improvement reaches NtSecurityDescriptor.Unmarshal.

How Verified

• Runtime reproduction (pre-fix, using the current tree): exercised each leaf and the full ACE path on truncated inputs in a harness wrapped with recover(). Five distinct panic signatures confirmed (mask with 0/3 bytes; authority with 0/4 bytes; full ACE with Size=4 + ACCESS_ALLOWED).
• Post-fix: same harness reports no panic for every case and a non-nil error is returned instead.
• Tests: added regression tests at every layer. All pass. go test ./... green.

Test Coverage

Added:

• ace/AccessControlEntry_test.go::TestAccessControlEntry_Unmarshal_MalformedNoPanic — nine truncated-ACE shapes; each asserts recover() sees no panic and a non-nil error is returned.
• ace/mask/AccessControlMask_test.go::TestAccessControlMask_Unmarshal_TruncatedReturnsError — 0–3 byte inputs.
• sid/authority/SecurityIdentifierAuthority_test.go::Test_SecurityIdentifierAuthority_Unmarshal_TruncatedReturnsError — 0–5 byte inputs.
• object/flags/AccessControlObjectTypeFlags_test.go::TestAccessControlObjectTypeFlags_Unmarshal_TruncatedReturnsError — 0–3 byte inputs.

Scope of Change

• Files changed: ace/mask/AccessControlMask.go, sid/authority/SecurityIdentifierAuthority.go, object/flags/AccessControlObjectTypeFlags.go, plus the four test files.
• Submodule pointer updated: no
• Behavioral changes outside the bug fix: none — any input that previously parsed successfully continues to parse identically; only previously-panicking paths now surface errors.

Risk and Rollout

Narrow. The three added checks are the first thing each function does and exit early on malformed input. No successful parse is affected. Safe to merge without staged rollout.

Notes

This PR addresses the panic reported in #30. A broader audit turned up no other reachable panic on the NtSecurityDescriptor.Unmarshal path — the SID sub-authority loop, the DACL/SACL entry loop, the GUID read, and the ACE header already bounds-check their inputs, and every other caller either validates upfront or exits via an error from the three fixed leaves.

[p0dalirius]

Expanded the scope of this PR after a second audit pass: ran a harness that calls every exported Unmarshal directly with 0–40 byte inputs and every From* entry point with a mix of malformed strings. Four additional leaves panicked on empty/short input even though the production call chain pre-slices them correctly:

• ace/aceflags.AccessControlEntryFlag.Unmarshal
• ace/acetype.AccessControlEntryType.Unmarshal
• acl/revision.AccessControlListRevision.Unmarshal
• securitydescriptor/control.NtSecurityDescriptorControl.Unmarshal

External callers invoking these directly could still crash the process, so I added length guards and regression tests for each. Also added go test -fuzz targets for NtSecurityDescriptor.Unmarshal and FromSDDLString; running each for 15 s produced >2 M and >1 M iterations respectively with zero panics.

Net result: every path through the parsing functions now ends in a handled error — no reachable panic path remains on the NtSecurityDescriptor.Unmarshal or FromSDDLString entry points, and every exported Unmarshal leaf is also panic-safe when called directly.

Commit: 0bae9e8.