[PATCH v2 0/4] samba: fix conversion of strings to binary SIDs and vice versa
Andrew Bartlett
abartlet at samba.org
Tue Jul 30 19:21:21 MDT 2013
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,
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