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

[Jeff Layton]

On Wed, 31 Jul 2013 11:45:17 +1200
Andrew Bartlett <abartlet at samba.org> wrote:

> 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
> 

That sounds great, but I won't have the time to work on that any time
soon. If you or someone else more familar with this code would like to
do that however, then that'd be great.

-- 
Jeff Layton <jlayton at samba.org>