RPC_SID structure does not match the MS documentation

[Dramelac]

Configuration

impacket version: Impacket v0.10.1.dev1+20220830.171426.b74c9e1e
Python version: Python 3.9.2
Target OS: Debian 11

Context

The data generated by impacket.dcerpc.v5.dtypes.RPC_SID.getData() does not correspond to the structure documented by Microsoft https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/f992ad60-0fe4-4b87-9fed-beb478836861
According to the MS documentation, the first byte should be the Revision byte but impacket seems to duplicate the SubAuthorityCount at the right place AND at the beginning of the packet.

Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from impacket.dcerpc.v5.dtypes import RPC_SID
>>> x = RPC_SID()
>>> x.fromCanonical('S-1-5-21-706573996-2287698545-1520902760-1000')
>>> x.getData()
b'\x05\x00\x00\x00\x01\x05\x00\x00\x00\x00\x00\x05\x15\x00\x00\x00\xacv\x1d*q\x82[\x88h"\xa7Z\xe8\x03\x00\x00'

It should have come out with this result: b'\x01\x05\x00\x00\x00\x00\x00\x05\x15\x00\x00\x00\xacv\x1d*q\x82[\x88h"\xa7Z\xe8\x03\x00\x00'

Same error for the parsing:

>>> y = RPC_SID(b'\x01\x05\x00\x00\x00\x00\x00\x05\x15\x00\x00\x00\xacv\x1d*q\x82[\x88h"\xa7Z\xe8\x03\x00\x00')
>>> y.formatCanonical()
'S-0-0'

Is there a reason why this structure behaves like this in impacket or is it a mistake?

Waiting for an answer / correction, I was able to use the equivalent LDAP implementation (impacket.ldap.ldaptypes.LDAP_SID) which behaves correctly.

Thank you

[0xdeaddood]

Hi @Dramelac!

AFAIK the RPC_SID structure returned by get_Data() corresponds to the Transfer NDR Syntax implementation, where if we have a conformant array, the first item in the structure must be the array size. If Beto @asolino is here, maybe he can confirm it for us.

Waiting for an answer / correction, I was able to use the equivalent LDAP implementation (impacket.ldap.ldaptypes.LDAP_SID) which behaves correctly.

I think the most appropriate is to use the RPC_SID structure. I found that using it with the PAC_UPN_DNS_INFO works fine. As for the PAC_REQUESTOR structure, as you mentioned here, the structure seems to cause problems. A potential workaround could be to simply remove the first four bytes returned by getData()[4:].

[Dramelac]

Hi @0xdeaddood !

Although it works well to generate a new PAC ticket, this quick fix is problematic when you want to parse an existing PAC_REQUESTOR from an actual ticket
For example with this awesome tool for debugging kerberos ticket #1201 by @ShutdownRepo (a must have in impacket btw). This tool is just an exemple use case, there are other cases where you want to parse a kerberos ticket it's not just on edge case (diamond ticket, saphir ticket, etc)

But generally with your workaround, if we want to parse a kerberos ticket with the impakcet lib, we will have a bug every time... and a qucik & dirty fix will be needed everytime :/ which is not ideal for a lib that is supposed to be reliable but that's my opinion

PoC:

# Ticket generation
pacRequestor = PAC_REQUESTOR()
pacRequestor['UserSid'].fromCanonical('S-1-5-21-4088429403-1159899800-2753317549-1108')
pacData = pacRequestor.getData()[4:]
# Ticket parsing
PAC_REQUESTOR(pacData)['UserSid'].formatCanonical()  # Return S-0-0 because of the missing 4 bits..

The quick & dirty fix should be someting like that:

PAC_REQUESTOR(b'\x05\x00\x00\x00'+pacData)['UserSid'].formatCanonical()

But thats not a very elegant solution because the \x05 byte can change depending on the SID ..
Another dirty fix more dynamic could be:

PAC_REQUESTOR(pacData[1:5]+pacData)['UserSid'].formatCanonical()

[Dramelac]

I can also add that this behavior of RPC_SID impacts other cases than kerberos and I have already heard here and there about using the LDAP_SID structure in workaround, it might be time to create a sustainable solution that correctly implements the SID for all present and future uses and stop using LDAP_SID as a quick & dirty fix

[0xdeaddood]

Hi @Dramelac!

Thanks again for all your work in this issue and the extra-pac implementation!

After double checking the MS-PAC standard, I found that the PAC_REQUESTOR structure contains a single SID structure (not the RPC_SID structure!). That is the reason why when using RPC_SID (and NDR encoding), the example fails.

The PAC_REQUESTOR structure is a variable length buffer of the PAC that SHOULD contain the SID ([MSDN-SID]) of the client that requested the ticket

So, as you mentioned before, the best solution is to create a new SID structure for use in non-RPC protocols. That structure is defined in the winnt.h library. We think the best place to include it in Impacket is under [MS-DTYP] 2.4.2.2 SID--Packet Representation.

I opened #1545 with this new structure and the modifications in ticketer.

[0xdeaddood]

Fixed in #1545

Closing