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. Thanks, Jeremy.
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, Jeremy.
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.
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.
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.
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. Jeremy.
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.