[PATCH RFC] libwbclient: fix conversion logic in wbcStringToSid

[Jeff Layton]

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;
 
 	/* 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);
 		if (p == q)
 			break;
-		if (q == NULL) {
+		if (x > UINT_MAX) {
 			wbc_status = WBC_ERR_INVALID_SID;
 			BAIL_ON_WBC_ERROR(wbc_status);
 		}
 		sid->sub_auths[sid->num_auths++] = x;
 
-		if (*q != '-') {
+		if (*q != '-')
 			break;
-		}
+
 		p = q + 1;
 	}
 
-- 
1.8.3.1