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

[Jeff Layton]

On Wed, 31 Jul 2013 13:21:21 +1200
Andrew Bartlett <abartlet at samba.org> wrote:

> On Tue, 2013-07-30 at 21:08 -0400, Jeff Layton wrote:
> > 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.
> 
> Can you at least add the tests to the existing framework then?
> 
> Thanks,
> 

No. I have no tests for this, and no familiarity with the samba testing
framework. I don't have a lot of time to spare on this, I'm afraid. I
simply noticed this by inspection as I was working on some similar code
in cifs-utils and took the time to code up some similar fixes for samba.

-- 
Jeff Layton <jlayton at samba.org>