Reject NtSecurityDescriptor component offsets inside the header (#90)#91
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #90Motivation
NtSecurityDescriptor.Unmarshalvalidated each component offset only withOffset < len(RawBytes). With no lower bound, a non-zero offset pointing into the fixed 20-byte header (for exampleOffsetOwner = 4) was accepted and the header bytes were mis-parsed as a SID/ACL, producing garbage rather than an error. For a parser of untrusted security descriptors, an offset overlapping the header is structurally invalid and should be rejected. This complements theAclSizebound for ACLs (#78): together they confine each component to a valid region.What Changed
ntSecurityDescriptorHeaderSize = 20and used it for the existingRawBytesSizeinitialization as well as the new checks.Unmarshal(Owner, Group, DACL, SACL) now requiresOffset >= ntSecurityDescriptorHeaderSize && Offset < len(RawBytes); the error messages were updated to state the valid range.Acceptance Criteria Check
TestNtSecurityDescriptor_Unmarshal_OffsetInsideHeader(setsOffsetOwner = 4).Offset == 0(component absent) is still skipped without error — unchanged outer!= 0guard; covered by the dataset involution tests, which include descriptors with absent components.go test ./...green, includingTestNtSecurityDescriptor_Involutionover the real Windows/AD datasets; the new test also asserts the unmodified descriptor parses cleanly before corruption.How Verified
securitydescriptor/NtSecurityDescriptor_offset_test.go—TestNtSecurityDescriptor_Unmarshal_OffsetInsideHeader.go build ./...andgo test ./...pass.Test Coverage
Scope of Change
securitydescriptor/NtSecurityDescriptor.go,securitydescriptor/NtSecurityDescriptor_offset_test.goUnmarshal; it may now return an error on malformed offsets it previously mis-parsed)Risk and Rollout
Low. Well-formed descriptors place every component at or past the 20-byte header, so valid input is unaffected (verified by the dataset involution test); only offsets that overlap the header now error. Safe to merge.
Notes
Related: #78 (bound ACL parsing by AclSize), #82 (Marshal no longer emits offsets for an unset Owner/Group). Full mutual-extent overlap checking between components is deferred as noted in the issue.