[enhancement] Missing support for ACCESS_ALLOWED_CALLBACK_ACE_TYPE / ACCESS_DENIED_CALLBACK_ACE_TYPE

[Macmod]

One important use case of ACCESS_ALLOWED_CALLBACK_ACE_TYPE / ACCESS_DENIED_CALLBACK_ACE_TYPE is when parsing the SD located at HKLM:\SYSTEM\CurrentControlSet\Services\CertSvc\Configuration\<CANAME>\EnrollmentAgentRights in ADCS-enabled servers. This SD includes ACEs of this kind, and relevant information about the templates affected by each restriction in the ApplicationData.

[Macmod]

Just for reference, currently the SID's Unmarshall method is including the length of the ApplicationData that comes afterwards, and returning len(sid)+len(applicationdata) as the "BytesRead". So to actually implement this some logic should probably be changed there too.

[Macmod]

According to MS-DTYP, ApplicationData is present in other types of ACEs:

• SYSTEM_AUDIT_OBJECT_ACE_TYPE
• SYSTEM_AUDIT_CALLBACK_ACE_TYPE
• SYSTEM_AUDIT_CALLBACK_OBJECT_ACE_TYPE
• ACCESS_ALLOWED_CALLBACK_ACE_TYPE
• ACCESS_DENIED_CALLBACK_ACE_TYPE
• ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE
• ACCESS_DENIED_CALLBACK_OBJECT_ACE_TYPE

Maybe the right way to go is to actually include the ApplicationData in the ACE struct and leave it nil for ACLs that don't have it - the alternative would be to have specific types for each ACE types, using struct embedding with a base ACE type and including extra fields in specialized types. Although it would look nice, for now I think that's overcomplicated and could break compatibility.

I'm doing some testing in my fork, will share later if it works well.

[Macmod]

Proposed fix so far:
Macmod@2115f6a

I'm still having trouble with TestNtSecurityDescriptor_Involution - was that SD taken from a real environment?
From what I checked, in the test's descriptor, the Group SID in the end of the descriptor seems to be missing one subauthority, as it reports 5 but only 4 are present:

Header    | 0100 149c cc000000 e8000000 14000000 a0000000
SACL      | 04008c00030000000240140020000c00010100000000000100000000075a38002000000003000000be3b0ef3f09fd111b6030000f80367c1a57a96bfe60dd011a28500aa003049e2010100000000000100000000075a38002000000003000000bf3b0ef3f09fd111b6030000f80367c1a57a96bfe60dd011a28500aa003049e2010100000000000100000000
DACL      | 02002c000100000000002400ff010f0001050000000000051500000028bb82279261b9fe2474aa5d00020000
Owner SID | 01 05 000000000005 15000000 28bb8227 9261b9fe 2474aa5d 00020000
Group SID | 01 05 000000000005 15000000 28bb8227 9261b9fe 20000000 MISSING
             |  |  |            |          |       |       |        |- There should be a 5th subauthority here
             |  |  |            |          |       |       |
             |  |  |            |          |       |       |- 4th Subauthority
             |  |  |            |          |       |- 3rd Subauthority
             |  |  |            |          |- 2nd Subauthority
             |  |  |            |- 1st Subauthority
             |  |  |- IdentifierAuthority
             |  |- SubAuthorityCount
             |- Revision

Is that expected somehow?

[p0dalirius]

Hey,

Yes it is a real SD from an environment.

I am refactoring the dataset test data to pull it from a full environment and have a really high and reliable coverage (See PR #40)

For context, I remember having had troubles with this though. I remember that the last 4 bytes of some NTSDs were missing (were supposed to be 00000000). At the time, I had implemented an automatic padding of the SID.Unmarshal() function:

winacl/sid/SecurityIdentifier.go

Lines 313 to 316 in 076b620

	buffer := marshalledData[sid.RawBytesSize:]
	if len(buffer) < 4 {
	buffer = append(buffer, make([]byte, 4-len(buffer))...)
	}

I thought that this was something unreliable in Microsoft's NTSD format, where there is tolerance on the last bytes up to the expected 4bytes so you can trim them. I thought that:

If last 4 bytes are 00000000 => You can omit them and Windows is accepting this NTSD
If last 4 bytes are 11000000 => You can write 11 and Windows is accepting this NTSD
If last 4 bytes are 11220000 => You can write 1122 and Windows is accepting this NTSD
If last 4 bytes are 11223300 => You can write 112233 and Windows is accepting this NTSD
If last 4 bytes are 11223344 => You can write 11223344 and Windows is accepting this NTSD

I will do tests by removing my padding function and see what happens with all the NTSDs of an Active Directory, and then I will try with other NTSD sources,

Best regards,

[Macmod]

Yes, the problem really is that if we have ApplicationData we have to assume it's the last field of the ACE after the SID, and if the SID doesn't conform to its expected size, then it becomes an ambiguous problem. But thanks for taking the time to figure that out @p0dalirius, hopefully we can find a solution. :)

Let me know if I can help in any way.