[PATCH v2 0/4] samba: fix conversion of strings to binary SIDs and vice versa
Jeff Layton
jlayton at samba.org
Tue Jul 30 19:08:36 MDT 2013
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>
More information about the samba-technical
mailing list