Fix NtSecurityDescriptor.Marshal emitting offsets for an unset Owner/Group (#82)#83
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 #82Root Cause
NtSecurityDescriptor.Marshalgated Owner/Group serialization only onntsd.Owner != nil/ntsd.Group != nil.NewSecurityDescriptor()initializes both fields to&identity.Identity{}— a non-nil but unset Identity whose SID is the zero value. Marshalling such a descriptor therefore wrote non-zeroOffsetOwner/OffsetGroupand an 8-byte all-zero SID for each, so the output claimed an owner and group ofS-0-0-0, a SID with revision level 0 that is invalid per MS-DTYP (the only valid SID revision is 1). The SACL/DACL blocks already gate onlen(Entries) > 0, but the Owner/Group blocks had no equivalent "is this actually set" check.Fix Description
The Owner and Group blocks now additionally require
SID.RevisionLevel != 0before emitting the component, and set the corresponding offset to 0 otherwise — mirroring the presence checks used for the SACL and DACL.RevisionLevelis the canonical marker of a populated SID: bothSID.FromStringandSID.Unmarshalset it to a non-zero value, while the zero-value Identity has 0. This is the minimal change and does not touch the parsing path.How Verified
NewSecurityDescriptor().Marshal()produced 36 bytes withOffsetOwner=20,OffsetGroup=28, each pointing at a zero SID that parsed back asS-0-0-0.OffsetOwner=0,OffsetGroup=0, and round-trips toOwner == nil,Group == nil.TestNtSecurityDescriptor_Marshal_UnsetOwnerGroup.go build ./...andgo test ./...pass; existing tests that set Owner/Group viaFromString(RevisionLevel = 1) are unaffected.Test Coverage
securitydescriptor/NtSecurityDescriptor_test.go->TestNtSecurityDescriptor_Marshal_UnsetOwnerGroup.Scope of Change
securitydescriptor/NtSecurityDescriptor.go,securitydescriptor/NtSecurityDescriptor_test.goRisk and Rollout
Low blast radius. A populated owner/group (RevisionLevel = 1) marshals exactly as before; only the unset zero-value Identity, which previously serialized to an invalid SID, is now correctly represented as absent. Safe to merge directly.