Skip to content

Guard against uint16 overflow when marshalling oversized ACLs and ACEs (#79)#87

Merged
p0dalirius merged 1 commit into
mainfrom
enhancement-marshal-uint16-overflow-guards
Jun 1, 2026
Merged

Guard against uint16 overflow when marshalling oversized ACLs and ACEs (#79)#87
p0dalirius merged 1 commit into
mainfrom
enhancement-marshal-uint16-overflow-guards

Conversation

@p0dalirius

Copy link
Copy Markdown
Collaborator

Linked Issue

Closes #79

Motivation

AclSize (ACL header) and Header.Size (ACE header) are uint16 fields. The marshallers computed them with unchecked conversions (uint16(8 + len(...)) and uint16(len(...) + headerSize)). A DACL with many/large ACEs, or an ACE with a large ApplicationData payload (resource-attribute / conditional-expression ACEs), can legitimately exceed 64 KB, at which point the cast wraps modulo 65536 and writes a structurally invalid, too-small size. Returning an error is far safer than emitting silently corrupt output a reader would then truncate.

What Changed

  • DiscretionaryAccessControlList.Marshal and SystemAccessControlList.Marshal compute totalSize = 8 + len(body) and return an error if it exceeds 0xFFFF before assigning Header.AclSize.
  • AccessControlEntry.Marshal computes totalSize = len(body) + headerSize and returns an error if it exceeds 0xFFFF; the existing size comparison and the Header.Size assignment now both use that validated value, so the guard covers the comparison branch as well as the assignment.

Acceptance Criteria Check

  • Marshalling an ACL whose body exceeds 65535 bytes errors instead of wrapping — TestDACL_Marshal_OversizedAclSize, TestSACL_Marshal_OversizedAclSize.
  • Marshalling an ACE whose body + header exceeds 65535 bytes errors instead of wrapping — TestACE_Marshal_OversizedApplicationData.
  • Normal-sized ACLs/ACEs marshal as before — TestDACL_Marshal_NormalSizeOK, TestACE_Marshal_NormalSizeOK, plus the full existing suite including the real-dataset involution test.
  • New tests in acl/ and ace/ cover the overflow boundary.

How Verified

  • Tests: ace/AccessControlEntry_overflow_test.go and acl/AccessControlList_overflow_test.go (oversized cases error; normal cases succeed).
  • go build ./... and go test ./... pass.

Test Coverage

  • Added: the overflow/normal tests listed above for both ACL types and the ACE.

Scope of Change

  • Files changed: acl/DiscretionaryAccessControlList.go, acl/SystemAccessControlList.go, ace/AccessControlEntry.go, acl/AccessControlList_overflow_test.go, ace/AccessControlEntry_overflow_test.go
  • Submodule pointer updated: no
  • Public API changes: none; Marshal may now return an error in the overflow case where it previously returned silently-wrong bytes
  • Behavioral changes outside the stated enhancement: none

Risk and Rollout

Low. Normal-sized data marshals byte-identically (verified by the dataset involution test); only inputs that genuinely cannot fit the 16-bit size fields now error instead of producing corrupt output. Safe to merge.

@p0dalirius p0dalirius self-assigned this Jun 1, 2026
@p0dalirius p0dalirius merged commit e799ef8 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