[PATCH RFC] libwbclient: fix conversion logic in wbcStringToSid

[Jeff Layton]

On Sun, 21 Jul 2013 16:22:11 +0200
Volker Lendecke <Volker.Lendecke at SerNet.DE> wrote:

> Hi, Jeff!
> 
> This looks very good, thanks!
> 
> We have another function called dom_sid_parse_endp somewhere
> that might need similar care...
> 

Ok, I'll take a look at that too.

> On Fri, Jul 12, 2013 at 04:24:12PM -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton at samba.org>
> > 
> > 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 with this code:
> > 
> > - 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 consider this a RFC patch...
> > 
> > Signed-off-by: Jeff Layton <jlayton at samba.org>
> > ---
> >  nsswitch/libwbclient/wbc_sid.c | 41 ++++++++++++++++++++++-------------------
> >  1 file changed, 22 insertions(+), 19 deletions(-)
> > 
> > diff --git a/nsswitch/libwbclient/wbc_sid.c b/nsswitch/libwbclient/wbc_sid.c
> > index bab6933..f27adf9 100644
> > --- a/nsswitch/libwbclient/wbc_sid.c
> > +++ b/nsswitch/libwbclient/wbc_sid.c
> > @@ -88,13 +88,15 @@ wbcErr wbcSidToString(const struct wbcDomainSid *sid,
> >  	return WBC_ERR_SUCCESS;
> >  }
> >  
> > +#define AUTHORITY_MASK	(~(0xffffffffffffULL))
> > +
> >  /* Convert a character string to a binary SID */
> >  wbcErr wbcStringToSid(const char *str,
> >  		      struct wbcDomainSid *sid)
> >  {
> >  	const char *p;
> >  	char *q;
> > -	uint32_t x;
> > +	uint64_t x;
> >  	wbcErr wbc_status = WBC_ERR_UNKNOWN_FAILURE;
> >  
> >  	if (!sid) {
> > @@ -115,46 +117,47 @@ wbcErr wbcStringToSid(const char *str,
> >  	/* Get the SID revision number */
> >  
> >  	p = str+2;
> > -	x = (uint32_t)strtol(p, &q, 10);
> > -	if (x==0 || !q || *q!='-') {
> > +	x = (uint64_t)strtoul(p, &q, 10);
> > +	if (x==0 || x > UCHAR_MAX || !q || *q!='-') {
> >  		wbc_status = WBC_ERR_INVALID_SID;
> >  		BAIL_ON_WBC_ERROR(wbc_status);
> >  	}
> >  	sid->sid_rev_num = (uint8_t)x;
> >  
> > -	/* Next the Identifier Authority.  This is stored in big-endian
> > -	   in a 6 byte array. */
> > -
> > +	/*
> > +	 * Next the Identifier Authority.  This is stored big-endian in a
> > +	 * 6 byte array. If the authority value is > UINT_MAX, then it should
> > +	 * be expressed as a hex value, according to MS-DTYP.
> > +	 */
> >  	p = q+1;
> > -	x = (uint32_t)strtol(p, &q, 10);
> > -	if (!q || *q!='-') {
> > +	x = (uint64_t)strtoull(p, &q, 0);
> > +	if (!q || *q!='-' || (x & AUTHORITY_MASK)) {
> >  		wbc_status = WBC_ERR_INVALID_SID;
> >  		BAIL_ON_WBC_ERROR(wbc_status);
> >  	}
> > -	sid->id_auth[5] = (x & 0x000000ff);
> > -	sid->id_auth[4] = (x & 0x0000ff00) >> 8;
> > -	sid->id_auth[3] = (x & 0x00ff0000) >> 16;
> > -	sid->id_auth[2] = (x & 0xff000000) >> 24;
> > -	sid->id_auth[1] = 0;
> > -	sid->id_auth[0] = 0;
> > +	sid->id_auth[5] = (x & 0x0000000000ff);
> > +	sid->id_auth[4] = (x & 0x00000000ff00) >> 8;
> > +	sid->id_auth[3] = (x & 0x000000ff0000) >> 16;
> > +	sid->id_auth[2] = (x & 0x0000ff000000) >> 24;
> > +	sid->id_auth[1] = (x & 0x00ff00000000) >> 32;
> > +	sid->id_auth[0] = (x & 0xff0000000000) >> 48;
> 
> I'm not familiar with C enough (after having tried to
> program it for 2 decades...) -- 0xff0000000000, doesn't that
> possibly need a LL specifier at the end on 32 bit?
> 

It sure does -- sparse complains about that too. I fixed it in the
cifs-utils version of this patch, I'll go ahead and fix it too. Good
catch.

> >  
> >  	/* now read the the subauthorities */
> > -
> >  	p = q +1;
> >  	sid->num_auths = 0;
> >  	while (sid->num_auths < WBC_MAXSUBAUTHS) {
> > -		x=(uint32_t)strtoul(p, &q, 10);
> > +		x=(uint64_t)strtoull(p, &q, 10);
> 
> Is this cast really necessary? Similarly above.
> 

Probably not, at least on most arches. strtoull returns an unsigned
long long, so if you assume that that's the same as a uint64_t on all
arches then it's not really necessary.

> >  		if (p == q)
> >  			break;
> > -		if (q == NULL) {
> > +		if (x > UINT_MAX) {
> 
> UINT_MAX, can't that be 2^64-1 on 64 bit? Shouldn't we use
> UINT32_MAX here?
> 

As far as I know, all arches (at least on linux) make sure that "int"
is 32 bit. That said, UINT32_MAX does sound more explicit. I'll fix it.

> >  			wbc_status = WBC_ERR_INVALID_SID;
> >  			BAIL_ON_WBC_ERROR(wbc_status);
> >  		}
> >  		sid->sub_auths[sid->num_auths++] = x;
> >  
> > -		if (*q != '-') {
> > +		if (*q != '-')
> >  			break;
> > -		}
> > +
> 
> That's Samba style, Linux kernel is different...
> 

Fair enough -- I'll drop that hunk.

Thanks for the review. I'll send a v2 patch when I get a chance.
-- 
Jeff Layton <jlayton at redhat.com>