Fix AclSize computation by Ebrix · Pull Request #4832 · secdev/scapy

Ebrix · 2025-09-13T13:19:14Z

This comes from a legitimate packet crafted by Windows via a call to `RegSaveKey`

>>> import_hexcap()
0000   00 00 00 00 4e df f0 ae f8 72 26 4c 93 57 dc 26
 eb f1 66 00 00 0010   9e eb f1 66 00 00 00 00 40 00 40 00 00 00 00 00
20   00 00 02 000020   00 00 02 00 00 00 00 00 20 00 00 00 00 00 00 00
0030   00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
   43 00 3a 00 50040   43 00 3a 00 5c 00 55 00 73 00 65 00 72 00 73 00
0050   5c 00 41 00 64 00 6d 00 69 00 6e 00 5c 00 44 00
0060   65 00 73 00 6b 00 74 00 6f 00 70 00 5c 00 71 00
72 00 65 00 67 00070   77 00 65 00 72 00 2e 00 72 00 65 00 67 00 00 00
0080   00 00 02 00 00 00 00 00 18 00 00 00 00 00 00 00
090   00 00 02 00 00 00 00 00 34 00 00 00 34 00 00 00
00a0   00 00 00 00 00 00 00090   00 00 02 00 00 00 00 00 34 00 00 00 34 00 00 00
00a0   00 00 00 00 00 00 00 00 34 00 00 00 00 00 00 00
00b0   00 00 00 00 00 00 00 00 34 00 00 00 00 00 00 00
00c0   01 00 04 90 00 00 00 00 00 00 00 00 00 00 00 00
00d0   14 00 00 00 02 00 20 00 01 00 00 00 00 00 18 00
00e0   00 00 00 10 01 02 00 00 00 00 00 05 20 00 00 00
00f0   20 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00

b'\x00\x00\x00\x00N\xdf\xf0\xae\xf8r&L\x93W\xdc&\x9e\xeb\xf1f\x00\x00\x00\x00@\x00@\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00C\x00:\x00\\\x00U\x00s\x00e\x00r\x00s\x00\\\x00A\x00d\x00m\x00i\x00n\x00\\\x00D\x00e\x00s\x00k\x00t\x00o\x00p\x00\\\x00q\x00w\x00e\x00r\x00.\x00r\x00e\x00g\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x004\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00 \x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>> win_op = BaseRegSaveKey_Request(_, ndr64=True)
>>> win_op.show()
###[ BaseRegSaveKey_Request ]###
  \hKey      \
   |###[ NDRContextHandle ]###
   |  attributes= 0
   |  uuid      = b'N\xdf\xf0\xae\xf8r&L\x93W\xdc&\x9e\xeb\xf1f'
  \lpFile    \
   |###[ RPC_UNICODE_STRING ]###
   |  Length    = 64
   |  MaximumLength= 64
   |  \Buffer    \
   |   |###[ NDRPointer ]###
   |   |  referent_id= 0x20000
   |   |  \value     \
   |   |   |###[ NDRConformantArray ]###
   |   |   |  max_count = 32
   |   |   |  \value     \
   |   |   |   |###[ NDRVaryingArray ]###
   |   |   |   |  offset    = 0
   |   |   |   |  actual_count= 32
   |   |   |   |  value     = b'C:\\Users\\Admin\\Desktop\\qwer.reg\x00'
  \pSecurityAttributes\
   |###[ NDRPointer ]###
   |  referent_id= 0x20000
   |  \value     \
   |   |###[ PRPC_SECURITY_ATTRIBUTES ]###
   |   |  nLength   = 24
   |   |  \RpcSecurityDescriptor\
   |   |   |###[ RPC_SECURITY_DESCRIPTOR ]###
   |   |   |  \lpSecurityDescriptor\
   |   |   |   |###[ NDRPointer ]###
   |   |   |   |  referent_id= 0x20000
   |   |   |   |  \value     \
   |   |   |   |   |###[ NDRConformantArray ]###
   |   |   |   |   |  max_count = 52
   |   |   |   |   |  \value     \
   |   |   |   |   |   |###[ NDRVaryingArray ]###
   |   |   |   |   |   |  offset    = 0
   |   |   |   |   |   |  actual_count= 52
   |   |   |   |   |   |  value     = b'\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00 \x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00'
   |   |   |  cbInSecurityDescriptor= 52
   |   |   |  cbOutSecurityDescriptor= 52
   |   |  bInheritHandle= 0
###[ Padding ]###
     load      = b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

>>> win_sd = SECURITY_DESCRIPTOR(b'\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00 \x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00')
>>> win_sd.show()
###[ SECURITY_DESCRIPTOR ]###
  Revision  = 1
  Sbz1      = 0
  Control   = DACL_PRESENT+DACL_PROTECTED+SELF_RELATIVE
  OwnerSidOffset= 0
  GroupSidOffset= 0
  SACLOffset= 0
  DACLOffset= 20
  Data =
     \DACL      \
      |###[ WINNT_ACL ]###
      |  AclRevision= 2
      |  Sbz1      = 0
      |  AclSize   = 32
      |  AceCount  = 1
      |  Sbz2      = 0
      |  \Aces      \
      |   |###[ WINNT_ACE_HEADER ]###
      |   |  AceType   = ACCESS_ALLOWED
      |   |  AceFlags  =
      |   |  AceSize   = 24
      |   |###[ WINNT_ACCESS_ALLOWED_ACE ]###
      |   |     Mask      = GENERIC_ALL
      |   |     \Sid       \
      |   |      |###[ WINNT_SID ]###
      |   |      |  Revision  = 1
      |   |      |  SubAuthorityCount= 2
      |   |      |  \IdentifierAuthority\
      |   |      |   |###[ WINNT_SID_IDENTIFIER_AUTHORITY ]###
      |   |      |   |  Value     = b'\x00\x00\x00\x00\x00\x05'
      |   |      |  SubAuthority= [32, 544]

The security descriptor was created using the following C code :

void GetSecurityDescriptor(
    PSECURITY_DESCRIPTOR* pSecDesc
)
{

    LPCWSTR sddl = L"D:P(A;;GA;;;BA)";
    if (!ConvertStringSecurityDescriptorToSecurityDescriptor(
        sddl,
        SDDL_REVISION_1, pSecDesc, NULL)) {
        return;
    }

}

...

GetSecurityDescriptor(&pSecDesc);
sa.lpSecurityDescriptor = pSecDesc;

res = RegSaveKey(hKey, backupPath, &sa);

We can find exactly what we requested. The security descriptor match our expectation and AclSize == 32 (0x20)

This is what we get when performing the same operation with Scapy

>>> import_hexcap()
0000   00 00 00 00 c8 ff 68 02 7c 79 4f 49 bd d3 e2 c7
0010   60 3a 1d d3 00 00 00 00 1e 00 1e 00 00 00 00 00
0020   00 00 02 00 00 00 00 00 0f 00 00 00 00 00 00 00
0030   00 00 00 00 00 00 00 00 0f 00 00 00 00 00 00 00
0040   43 00 3a 00 5c 00 61 00 7a 00 65 00 72 00 61 00
0050   7a 00 65 00 72 00 2e 00 72 00 65 00 67 00 00 00
0060   00 00 02 00 00 00 00 00 6c 00 00 00 00 00 00 00
00 00
00a0   01 00 04 90 00 00 00070   00 00 02 00 00 00 00 00 34 00 00 00 34 00 00 00
 00
00c0   00 00 00 10 01 02 00 00 00 00 00 05 20080   00 00 00 00 00 00 00 00 34 00 00 00 00 00 00 00
0090   00 00 00 00 00 00 00 00 34 00 00 00 00 00 00 00
00a0   01 00 04 90 00 00 00 00 00 00 00 00 00 00 00 00
00b0   14 00 00 00 02 00 26 00 01 00 00 00 00 00 18 00
00c0   00 00 00 10 01 02 00 00 00 00 00 05 20 00 00 00
00d0   20 02 00 00

b'\x00\x00\x00\x00\xc8\xffh\x02|yOI\xbd\xd3\xe2\xc7`:\x1d\xd3\x00\x00\x00\x00\x1e\x00\x1e\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00C\x00:\x00\\\x00a\x00z\x00e\x00r\x00a\x00z\x00e\x00r\x00.\x00r\x00e\x00g\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00l\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x004\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x004\x00\x00\x00\x00\x00\x00\x00\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00&\x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00'
>>> scap_op = BaseRegSaveKey_Request(_, ndr64=True)
>>> scap_op.show()
###[ BaseRegSaveKey_Request ]###
  \hKey      \
   |###[ NDRContextHandle ]###
   |  attributes= 0
   |  uuid      = b'\xc8\xffh\x02|yOI\xbd\xd3\xe2\xc7`:\x1d\xd3'
  \lpFile    \
   |###[ RPC_UNICODE_STRING ]###
   |  Length    = 30
   |  MaximumLength= 30
   |  \Buffer    \
   |   |###[ NDRPointer ]###
   |   |  referent_id= 0x20000
   |   |  \value     \
   |   |   |###[ NDRConformantArray ]###
   |   |   |  max_count = 15
   |   |   |  \value     \
   |   |   |   |###[ NDRVaryingArray ]###
   |   |   |   |  offset    = 0
   |   |   |   |  actual_count= 15
   |   |   |   |  value     = b'C:\\azerazer.reg'
  \pSecurityAttributes\
   |###[ NDRPointer ]###
   |  referent_id= 0x20000
   |  \value     \
   |   |###[ PRPC_SECURITY_ATTRIBUTES ]###
   |   |  nLength   = 108
   |   |  \RpcSecurityDescriptor\
   |   |   |###[ RPC_SECURITY_DESCRIPTOR ]###
   |   |   |  \lpSecurityDescriptor\
   |   |   |   |###[ NDRPointer ]###
   |   |   |   |  referent_id= 0x20000
   |   |   |   |  \value     \
   |   |   |   |   |###[ NDRConformantArray ]###
   |   |   |   |   |  max_count = 52
   |   |   |   |   |  \value     \
   |   |   |   |   |   |###[ NDRVaryingArray ]###
   |   |   |   |   |   |  offset    = 0
   |   |   |   |   |   |  actual_count= 52
   |   |   |   |   |   |  value     = b'\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00&\x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00'
   |   |   |  cbInSecurityDescriptor= 52
   |   |   |  cbOutSecurityDescriptor= 52
   |   |  bInheritHandle= 0

>>> scap_sd = SECURITY_DESCRIPTOR(b'\x01\x00\x04\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x14\x00\x00\x00\x02\x00&\x00\x01\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x10\x01\x02\x00\x00\x00\x00\x00\x05 \x00\x00\x00 \x02\x00\x00')
>>> scap_sd.show()
###[ SECURITY_DESCRIPTOR ]###
  Revision  = 1
  Sbz1      = 0
  Control   = DACL_PRESENT+DACL_PROTECTED+SELF_RELATIVE
  OwnerSidOffset= 0
  GroupSidOffset= 0
  SACLOffset= 0
  DACLOffset= 20
  Data =
     \DACL      \
      |###[ WINNT_ACL ]###
      |  AclRevision= 2
      |  Sbz1      = 0
      |  AclSize   = 38
      |  AceCount  = 1
      |  Sbz2      = 0
      |  \Aces      \
      |   |###[ WINNT_ACE_HEADER ]###
      |   |  AceType   = ACCESS_ALLOWED
      |   |  AceFlags  =
      |   |  AceSize   = 24
      |   |###[ WINNT_ACCESS_ALLOWED_ACE ]###
      |   |     Mask      = GENERIC_ALL
      |   |     \Sid       \
      |   |      |###[ WINNT_SID ]###
      |   |      |  Revision  = 1
      |   |      |  SubAuthorityCount= 2
      |   |      |  \IdentifierAuthority\
      |   |      |   |###[ WINNT_SID_IDENTIFIER_AUTHORITY ]###
      |   |      |   |  Value     = b'\x00\x00\x00\x00\x00\x05'
      |   |      |  SubAuthority= [32, 544]

The security descriptor was created with the following code :

class WellKnownSIDs(Enum):
    """
    Well-known SIDs.

    .. notes::
    This class should be filled with more values as needs arise
    """

    SY = WINNT_SID.fromstr("S-1-5-18")  # Local System
    BA = WINNT_SID.fromstr("S-1-5-32-544")  # Built-in Administrators


DEFAULT_SECURITY_DESCRIPTOR = SECURITY_DESCRIPTOR(
    Control=0x1000 | 0x8000 | 0x4,
    # OwnerSid=WellKnownSIDs.SY.value,  # Local System SID
    # GroupSid=WellKnownSIDs.SY.value,  # Local System SID
    DACL=WINNT_ACL(
        AclRevision=2,
        Sbz1=0,
        Aces=[
            WINNT_ACE_HEADER(
                AceType=0x0,  # ACCESS_ALLOWED_ACE_TYPE
                AceFlags=0x0,  # No flags
            )
            / WINNT_ACCESS_ALLOWED_ACE(
                Mask=AccessRights.GENERIC_ALL,  # GA
                Sid=WellKnownSIDs.BA.value,  # Built-in Administrators SID
            ),
        ],
    ),
    ndr64=True,
)

We can see that the AclSize here is not 32 but 38 (0x26). Our request ends-up on an invalid argument error.

>>> hexdiff(win_sd, scap_sd)
0000 0000   01 00 04 90 00 00 00 00  00 00 00 00 00 00 00 00   ................
0010        14 00 00 00 02 00 20 00  01 00 00 00 00 00 18 00   ...... .........
     0010   14 00 00 00 02 00 26 00  01 00 00 00 00 00 18 00   ......&.........
0020 0020   00 00 00 10 01 02 00 00  00 00 00 05 20 00 00 00   ............ ...
0030 0030   20 02 00 00                                         ...

fixes #4831

gpotter2 · 2025-09-13T17:14:05Z

Hi & thanks a lot for the PR.

Could you add a test?
For instance after this one

scapy/test/scapy/layers/smb2.uts

Line 552 in d73bbc1

= SMB2 - Build and dissect SECURITY_DESCRIPTOR

Also the PEP8 test is failing.

codecov · 2025-09-13T17:15:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.88%. Comparing base (e35797e) to head (3046d95).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4832   +/-   ##
=======================================
  Coverage   80.88%   80.88%           
=======================================
  Files         368      368           
  Lines       90231    90231           
=======================================
+ Hits        72979    72980    +1     
+ Misses      17252    17251    -1

Files with missing lines	Coverage Δ
scapy/layers/smb2.py	`88.49% <ø> (ø)`

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR fixes the computation of AclSize in the WINNT_ACL class to match Windows-generated security descriptors. The issue was that Scapy was incorrectly calculating the ACL size by adding 14 bytes instead of 8 bytes to the length of the ACEs, causing a mismatch with legitimate Windows packets.

Fixed the AclSize computation in WINNT_ACL to properly calculate the total size by adding 8 bytes (header size) instead of 14 bytes
Added clarifying comments explaining the header structure and size calculation
Added a test to verify that the computed AclSize matches the actual length of the ACL

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
scapy/layers/smb2.py	Fixed AclSize calculation and added explanatory comments for the header structure
test/scapy/layers/smb2.uts	Added test assertion to verify AclSize computation matches actual ACL length

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Ebrix force-pushed the master branch from 4e9d674 to 8ebbcd4 Compare September 14, 2025 11:36

Fix AclSize computation: issue secdev#4831

3046d95

Ebrix force-pushed the master branch from 8ebbcd4 to 3046d95 Compare September 14, 2025 11:48

gpotter2 requested a review from Copilot September 14, 2025 12:58

Copilot AI reviewed Sep 14, 2025

View reviewed changes

Comment thread scapy/layers/smb2.py

gpotter2 approved these changes Sep 14, 2025

View reviewed changes

gpotter2 merged commit c7aa3e9 into secdev:master Sep 14, 2025
24 checks passed

gpotter2 added this to the 2.7.0 milestone Nov 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix AclSize computation#4832

Fix AclSize computation#4832
gpotter2 merged 1 commit into
secdev:masterfrom
Ebrix:master

Ebrix commented Sep 13, 2025

Uh oh!

gpotter2 commented Sep 13, 2025

Uh oh!

codecov Bot commented Sep 13, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Ebrix commented Sep 13, 2025

This comes from a legitimate packet crafted by Windows via a call to RegSaveKey

This is what we get when performing the same operation with Scapy

Uh oh!

gpotter2 commented Sep 13, 2025

Uh oh!

codecov Bot commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

This comes from a legitimate packet crafted by Windows via a call to `RegSaveKey`

codecov Bot commented Sep 13, 2025 •

edited

Loading