The Samba-Bugzilla – Bug 2211
Incorrect parsing of security descriptor sent from Windows NT
Last modified: 2005-08-24 10:16:30 UTC
We're working on a VFS module we've developed that provides NT security
semantics and support for alternate data streams. We are currently storing
the security descriptors in the extended attributes after marshalling the
SEC_DESC structure. While doing this we encountered the following problem :
NT seems to be sending a few ( 4 ) extra bytes as padding when it sends an
ACE containing a well known SID. Samba unmarshalls the ACE's properly if
they are sent from XP, but gets the sizes wrong if they are sent from NT as
the extra bytes sent by NT are thrown away. The result is that the offsets
that are stored in the SEC_DESC structure turn out to be wrong. When samba
tries to marshall a structure with the incorrect offsets it runs out of
memory and sends an NT_STATUS_ACCESS_DENIED.
I've attached a patch that should fix this problem.
What the patch basically does is calculate the actual number of bytes read
for an ACL and then re-calculates the offsets in the security descriptor.
Created attachment 879 [details]
Patch for parse_sec.c which fixes the mentioned problem
Can you send me a network trace showing this please ? It's not that I don't
trust you :-) but I'd really like to see an on-the-wire representation of this
before I commit the patch.
Created attachment 892 [details]
Ethereal trace of the "set security descriptor" command from Windows NT
This security descriptor has three ACEs, and the sizes of all the three ACES
are the same when sent from Windows NT (36). Looking at the second ACE - which
is "Everyone: Full control", the size is set to 36, whereas the actual ACE
takes up only 20 bytes, and there are 16 extra bytes sent. This is what causes
the bug, and the attached patch fixes it.
Created attachment 898 [details]
New proposed patch.
Ok, I think the patch might be a little complex. Plus, what it's really doing is
pointing out a flaw in our handling of size calculations on all marshalling
when we're doing the _pre/_post calls (there are only about 4 in Samba).
I am attaching a patch I'd like to use - can you review this please ?
Thanks, the patch works, only if a few typos are fixed though:
The following code:
if (prs_uint8("ace extra space", ps, depth, &c))
if (!prs_uint8("ace extra space", ps, depth, &c))
and similarly, this:
if (prs_uint8("acl extra space", ps, depth, &c))
if (!prs_uint8("acl extra space", ps, depth, &c))
And it appears to me that the second part of the patch (offset recalculation)
isn't necessary, because the offsets aren't changing at all. Since we're now
padding the ACEs and ACLs with extra bytes to match the original size while
marshalling, the original offsets obtained remain valid.
On second thoughts, maybe we can do away with the first part of the patch (the
zero padding of ACEs and ACLs), and do only the offset recalculation. If we
don't pad it, the size in the respective structure is the actual size of the
data and not the bloated, padded size as sent by NT. Then we can recalculate the
offsets while marshalling, and send a smaller, leaner security descriptor
This seems better to me than padding it.
I've tested the patch in both ways (leaving out either the padding or the offset
recalculation), and my code works fine both ways.
Doing just the offset calculation won't work, as if the size
field is larger than the linearized size when marshalling the
compound elements, then the offset calculations will still be wrong.
Thanks for finding the typos.
As far as I can see, the size field is always being reset to the actual
linearized size by the prs_uint16_post call while marshalling.
Yes, but it's the linearized size from when we did the _pre to where we
are now with the _post. That means if the composite linearization is less
than the size, then we linearize extra zero bytes to make up the difference,
then the size put on the wire will match the one in the struct.
I've committed this, so I'm closing the bug now. Thanks a *lot*
for your help on this.
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.