Bug 2211 - Incorrect parsing of security descriptor sent from Windows NT
Incorrect parsing of security descriptor sent from Windows NT
Status: CLOSED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.9
All Linux
: P3 minor
: none
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-05 11:41 UTC by Mrinal Kalakrishnan
Modified: 2005-08-24 10:16 UTC (History)
0 users

See Also:


Attachments
Patch for parse_sec.c which fixes the mentioned problem (4.40 KB, patch)
2005-01-05 11:42 UTC, Mrinal Kalakrishnan
no flags Details
Ethereal trace of the "set security descriptor" command from Windows NT (376 bytes, application/octet-stream)
2005-01-16 22:56 UTC, Mrinal Kalakrishnan
no flags Details
New proposed patch. (3.70 KB, patch)
2005-01-19 18:49 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mrinal Kalakrishnan 2005-01-05 11:41:21 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.
Comment 1 Mrinal Kalakrishnan 2005-01-05 11:42:30 UTC
Created attachment 879 [details]
Patch for parse_sec.c which fixes the mentioned problem
Comment 2 Jeremy Allison 2005-01-13 14:42:05 UTC
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.
Thanks,
Jeremy.
Comment 3 Mrinal Kalakrishnan 2005-01-16 22:56:58 UTC
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.
Comment 4 Jeremy Allison 2005-01-19 18:49:22 UTC
Created attachment 898 [details]
New proposed patch.
Comment 5 Jeremy Allison 2005-01-19 18:49:45 UTC
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,

Jeremy.
Comment 6 Mrinal Kalakrishnan 2005-01-20 00:32:37 UTC
Thanks, the patch works, only if a few typos are fixed though:

The following code:

if (prs_uint8("ace extra space", ps, depth, &c))
        return False;

should be:

if (!prs_uint8("ace extra space", ps, depth, &c))
        return False;

and similarly, this:

if (prs_uint8("acl extra space", ps, depth, &c))
	return False;

should be:

if (!prs_uint8("acl extra space", ps, depth, &c))
	return False;

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.
Comment 7 Mrinal Kalakrishnan 2005-01-20 00:38:35 UTC
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
across. :-)
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.
Comment 8 Jeremy Allison 2005-01-20 11:27:41 UTC
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.
Jeremy.
Comment 9 Mrinal Kalakrishnan 2005-01-20 11:51:17 UTC
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.
Comment 10 Jeremy Allison 2005-01-20 14:57:19 UTC
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.
Jeremy.
Comment 11 Gerald (Jerry) Carter 2005-08-24 10:16:30 UTC
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.