15050 SMB server mishandles some SIDs

Portions contributed by: Prashanth Badari <prbadari@tintri.com>
Reviewed by: Gordon Ross <gordon.w.ross@gmail.com>
Reviewed by: Toomas Soome <tsoome@me.com>
Approved by: Dan McDonald <danmcd@mnx.io>

1 files changed, 52 insertions, 25 deletions

diff --git a/usr/src/common/smbsrv/smb_sid.c b/usr/src/common/smbsrv/smb_sid.c
index 9d4dc978ae..c5700b0d87 100644
--- a/usr/src/common/smbsrv/smb_sid.c
+++ b/usr/src/common/smbsrv/smb_sid.c
@@ -20,7 +20,7 @@
  */
 /*
  * Copyright (c) 2007, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright 2014 Nexenta Systems, Inc.  All rights reserved.
+ * Copyright 2022 Tintri by DDN, Inc. All rights reserved.
  */
 
 #if !defined(_KERNEL) && !defined(_FAKE_KERNEL)
@@ -29,6 +29,7 @@
 #include <stdlib.h>
 #include <syslog.h>
 #else	/* !_KERNEL && !_FAKE_KERNEL */
+#include <sys/int_limits.h> /* Needed for _FAKE_KERNEL */
 #include <sys/types.h>
 #include <sys/systm.h>
 #include <sys/sunddi.h>
@@ -50,7 +51,7 @@ smb_sid_isvalid(smb_sid_t *sid)
 		return (B_FALSE);
 
 	return ((sid->sid_revision == NT_SID_REVISION) &&
-	    (sid->sid_subauthcnt < NT_SID_SUBAUTH_MAX));
+	    (sid->sid_subauthcnt <= NT_SID_SUBAUTH_MAX));
 }
 
 /*
@@ -317,7 +318,7 @@ smb_sid_t *
 smb_sid_fromstr(const char *sidstr)
 {
 	smb_sid_t *sid;
-	smb_sid_t *retsid;
+	smb_sid_t *retsid = NULL;
 	const char *p;
 	int size;
 	uint8_t i;
@@ -329,35 +330,47 @@ smb_sid_fromstr(const char *sidstr)
 	if (strncmp(sidstr, "S-1-", 4) != 0)
 		return (NULL);
 
+	sua = 0;
+	(void) ddi_strtoul(&sidstr[4], (char **)&p, 10, &sua);
+
+	/*
+	 * If ddi_strtoul() did the right thing, *p will point at the first '-'
+	 * after the identifier authority.
+	 * The IdentifierAuthority can be up to 2^48, but all known ones
+	 * currently fit into a uint8_t.
+	 * TODO: support IdentifierAuthorities > 255 (those over UINT32_MAX are
+	 * hex-formatted).
+	 */
+	if (sua > UINT8_MAX || (*p != '-' && *p != '\0'))
+		return (NULL);
+
 	size = sizeof (smb_sid_t) + (NT_SID_SUBAUTH_MAX * sizeof (uint32_t));
 	sid = kmem_zalloc(size, KM_SLEEP);
-
 	sid->sid_revision = NT_SID_REVISION;
-	sua = 0;
-	(void) ddi_strtoul(&sidstr[4], 0, 10, &sua);
 	sid->sid_authority[5] = (uint8_t)sua;
 
-	for (i = 0, p = &sidstr[5]; i < NT_SID_SUBAUTH_MAX && *p; ++i) {
-		while (*p && *p == '-')
+	for (i = 0; i < NT_SID_SUBAUTH_MAX && *p; ++i) {
+		while (*p == '-')
 			++p;
 
-		if (*p < '0' || *p > '9') {
-			kmem_free(sid, size);
-			return (NULL);
-		}
+		if (*p < '0' || *p > '9')
+			goto out;
 
 		sua = 0;
-		(void) ddi_strtoul(p, 0, 10, &sua);
+		(void) ddi_strtoul(p, (char **)&p, 10, &sua);
+		if (sua > UINT32_MAX)
+			goto out;
 		sid->sid_subauth[i] = (uint32_t)sua;
 
-		while (*p && *p != '-')
-			++p;
+		if (*p != '\0' && *p != '-')
+			goto out;
 	}
 
 	sid->sid_subauthcnt = i;
 	retsid = smb_sid_dup(sid);
-	kmem_free(sid, size);
 
+out:
+	kmem_free(sid, size);
 	return (retsid);
 }
 #else /* _KERNEL */
@@ -368,6 +381,7 @@ smb_sid_fromstr(const char *sidstr)
 	const char *p;
 	int size;
 	uint8_t i;
+	unsigned long sua;
 
 	if (sidstr == NULL)
 		return (NULL);
@@ -375,17 +389,29 @@ smb_sid_fromstr(const char *sidstr)
 	if (strncmp(sidstr, "S-1-", 4) != 0)
 		return (NULL);
 
+	sua = strtoul(&sidstr[4], (char **)&p, 10);
+
+	/*
+	 * If strtoul() did the right thing, *p will point at the first '-'
+	 * after the identifier authority.
+	 * The IdentifierAuthority can be up to 2^48, but all known ones
+	 * currently fit into a uint8_t.
+	 * TODO: support IdentifierAuthorities > 255 (those over UINT32_MAX are
+	 * hex-formatted).
+	 */
+	if (sua > UINT8_MAX || (*p != '-' && *p != '\0'))
+		return (NULL);
+
 	size = sizeof (smb_sid_t) + (NT_SID_SUBAUTH_MAX * sizeof (uint32_t));
 
-	if ((sid = malloc(size)) == NULL)
+	if ((sid = calloc(size, 1)) == NULL)
 		return (NULL);
 
-	bzero(sid, size);
 	sid->sid_revision = NT_SID_REVISION;
-	sid->sid_authority[5] = atoi(&sidstr[4]);
+	sid->sid_authority[5] = (uint8_t)sua;
 
-	for (i = 0, p = &sidstr[5]; i < NT_SID_SUBAUTH_MAX && *p; ++i) {
-		while (*p && *p == '-')
+	for (i = 0; i < NT_SID_SUBAUTH_MAX && *p; ++i) {
+		while (*p == '-')
 			++p;
 
 		if (*p < '0' || *p > '9') {
@@ -393,10 +419,11 @@ smb_sid_fromstr(const char *sidstr)
 			return (NULL);
 		}
 
-		sid->sid_subauth[i] = strtoul(p, NULL, 10);
-
-		while (*p && *p != '-')
-			++p;
+		sid->sid_subauth[i] = strtoul(p, (char **)&p, 10);
+		if (*p != '\0' && *p != '-') {
+			free(sid);
+			return (NULL);
+		}
 	}
 
 	sid->sid_subauthcnt = i;