[PATCH v2 0/4] samba: fix conversion of strings to binary SIDs and vice versa

[Andrew Bartlett]

On Tue, 2013-07-30 at 16:23 -0400, Jeff Layton wrote:
> This is the second posting. The first was a single patch, but Volker
> pointed out that there were other places that needed similar work.
> Here is a summary of the changes:
> 
> v2:
> - fix conversion logic in sid-to-string routines
> - fix the libcli versions of these routines
> - eliminate unneeded "code cleanup" deltas and casts
> - ensure that we add ULL specifiers on long constants
> - use UINT32_MAX instead of the more ambiguous UINT_MAX
> 
> The original patch description follows:
> 
> While implementing a similar routine for cifs-utils, I used the logic in
> wbcStringToSid. Unfortunately, I couldn't use it directly due to a
> number of problems in this code (some worse than others):
> 
> - We're not converting signed integers here, so it makes more sense to
>   to use strtou* variants.
> 
> - The code doesn't check whether the revision string is larger than a
>   uint8_t can hold. It should.
> 
> - It's possible to have legitimate authority bytes in fields 0 and 1,
>   so we need to convert that value using strtoull and mask and shift
>   those bytes too.
> 
> - We should also ensure that the value in the authority field does not
>   exceed what the id_auth array can hold. We should also check for it
>   to return ULONG_MAX. Checking the returned value vs. a newly declared
>   AUTHORITY_MASK should cover both cases.
> 
> - If the authority is >= UINT_MAX then it should be represented as a
>   hex value (according to MS-DTYP).
> 
> - rather than just ignoring high-order bits when converting the
>   subauthority values, we should instead ensure that it's not
>   larger than a 32-bit value, and throw an error if it is.
> 
> - the check for a NULL endptr value in the subauthority loop seems
>   to be entirely bogus. It's not clear to me when strtoul would ever
>   set that to NULL.
> 
> Now, all that said...I'm not very familiar with this code and I don't
> have a great way to test it so I'd prefer someone with more knowledge
> of this code take these patches and shepherd them in.

Testing this should not be hard.  We should simply have a table of
string and binary SIDs that convert back and forth between them, and
confirm that works both ways, and another table of plausible but
actually invalid SIDs to ensure we reject. 

source3/torture/torture.c:run_local_string_to_sid() would be a good
place to start, but should be extended to confirm the expected results. 

It would be great if this could be migrated into the source4 torture
code, because that allows us to use the torture_assert macros, but
that's entirely a personal preference.

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Catalyst IT                   http://catalyst.net.nz