Skip to content

Bound SACL/DACL Unmarshal by AclSize to prevent over-reading adjacent components (#78)#86

Merged
p0dalirius merged 1 commit into
mainfrom
enhancement-bound-acl-unmarshal-by-aclsize
Jun 1, 2026
Merged

Bound SACL/DACL Unmarshal by AclSize to prevent over-reading adjacent components (#78)#86
p0dalirius merged 1 commit into
mainfrom
enhancement-bound-acl-unmarshal-by-aclsize

Conversation

@p0dalirius

Copy link
Copy Markdown
Collaborator

Linked Issue

Closes #78

Motivation

AccessControlList.Unmarshal is handed the entire remaining buffer of a security descriptor — in a real descriptor the ACL is followed by the Owner and Group SIDs. The ACE loop was bounded only by Header.AceCount, and each AccessControlEntry.Unmarshal only checked its size against that whole tail. A corrupt or oversized AceCount therefore walked past the ACL and mis-parsed the following SID bytes as ACEs. Parsing must be confined to the region the header declares via AclSize.

What Changed

  • DiscretionaryAccessControlList.Unmarshal and SystemAccessControlList.Unmarshal now compute the ACE region as AclSize - headerSize and slice the working buffer to exactly that region (aceData) before the loop.
  • Added two validations: AclSize smaller than the header size, and AclSize claiming more bytes than are available, each return a descriptive error.
  • ACE parsing reads from the bounded aceData; if AceCount cannot be satisfied within AclSize, the per-ACE Unmarshal now fails and the error is wrapped with the ACE index.

Design Notes

For well-formed input AclSize == headerSize + sum(ACE sizes), so the accumulated RawBytesSize (and the returned consumed count) is unchanged — the real-dataset involution test confirms byte-for-byte round-trips still hold. The bound only changes behavior for malformed input, turning a silent over-read into an error.

Acceptance Criteria Check

  • AceCount exceeding what fits in AclSize returns an error — TestDACL_Unmarshal_AceCountExceedsAclSize.
  • Well-formed ACL followed by trailing bytes parses the same ACEs; consumed size equals AclSize — TestDACL_Unmarshal_TrailingBytesNotConsumed.
  • New test constructs an ACL with trailing non-ACE bytes and asserts the parser does not read past AclSize — same test (consumed == AclSize, trailing DEADBEEF not consumed).
  • Existing ACL/SecurityDescriptor round-trip tests pass — go test ./... green, including TestNtSecurityDescriptor_Involution over the real Windows/AD datasets.

How Verified

  • Tests: acl/DiscretionaryAccessControlList_aclsize_test.goTestDACL_Unmarshal_TrailingBytesNotConsumed, TestDACL_Unmarshal_AceCountExceedsAclSize.
  • go build ./... and go test ./... pass.

Test Coverage

  • Added: the two tests above, exercising both ACL types through the shared change.

Scope of Change

  • Files changed: acl/DiscretionaryAccessControlList.go, acl/SystemAccessControlList.go, acl/DiscretionaryAccessControlList_aclsize_test.go
  • Submodule pointer updated: no
  • Public API changes: none (signatures unchanged; Unmarshal may now return an error on malformed input it previously mis-parsed)
  • Behavioral changes outside the stated enhancement: none

Risk and Rollout

Low. Well-formed descriptors are unaffected (verified against the dataset involution test); only malformed/oversized ACLs now error instead of silently over-reading. Safe to merge.

@p0dalirius p0dalirius self-assigned this Jun 1, 2026
@p0dalirius p0dalirius merged commit 51a6c98 into main Jun 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant