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

Andrew Bartlett abartlet at samba.org
Wed Jul 31 00:33:34 MDT 2013


On Tue, 2013-07-30 at 22:07 -0400, Jeff Layton wrote:
> On Tue, 30 Jul 2013 21:34:57 -0400
> Jeff Layton <jlayton at samba.org> wrote:
> 
> > 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.
> > 
> 
> Did you mean something like this patch? It looks like there aren't any
> existing tests for the other code that this patches however, so I'm
> unclear on how to add such a patch for those.

That's what I meant, plus filling more binary SID examples in the table
below.

Andrew Bartlett

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




More information about the samba-technical mailing list