[PATCH RFC] libwbclient: fix conversion logic in wbcStringToSid

[Volker Lendecke]

Hi, Jeff!

This looks very good, thanks!

We have another function called dom_sid_parse_endp somewhere
that might need similar care...

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?

>  
>  	/* 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.

>  		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?

>  			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...

Thanks,

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de