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

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton at samba.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-samba-add-torture-tests-for-string-to-SID-changes.patch
Type: text/x-patch
Size: 1304 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130730/455e9e49/attachment.bin>