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>

6 files changed, 231 insertions, 27 deletions

diff --git a/exception_lists/check_rtime b/exception_lists/check_rtime
index bc342adb97..eb69ff4335 100644
--- a/exception_lists/check_rtime
+++ b/exception_lists/check_rtime
@@ -27,6 +27,7 @@
 # Copyright 2018 Joyent, Inc.
 # Copyright 2020 Oxide Computer Company
 # Copyright 2022 Garrett D'Amore <garrett@damore.org>
+# Copyright 2022 Tintri by DDN, Inc. All rights reserved.
 #
 
 # This file provides exceptions to the usual rules applied to ELF objects by
@@ -232,6 +233,7 @@ FORBIDDEN_DEP   usr/lib/netsvc/yp/ypxfrd        # C++
 
 # libfakekernel is a test environment, not intended for general use
 FORBIDDEN libfakekernel\.so
+FORBIDDEN_DEP opt/smbsrv-tests/tests/smb_sid/large_sids_kern
 FORBIDDEN_DEP usr/MACH(lib)/libzpool.so.1
 FORBIDDEN_DEP usr/bin/ztest
 FORBIDDEN_DEP usr/bin/raidz_test
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;
diff --git a/usr/src/pkg/manifests/system-test-smbsrvtest.p5m b/usr/src/pkg/manifests/system-test-smbsrvtest.p5m
index 109cb6ccb9..96025785e0 100644
--- a/usr/src/pkg/manifests/system-test-smbsrvtest.p5m
+++ b/usr/src/pkg/manifests/system-test-smbsrvtest.p5m
@@ -9,7 +9,7 @@
 # http://www.illumos.org/license/CDDL.
 #
 
-# Copyright 2021 Tintri by DDN, Inc.  All rights reserved.
+# Copyright 2022 Tintri by DDN, Inc.  All rights reserved.
 
 set name=pkg.fmri value=pkg:/system/test/smbsrvtest@$(PKGVERS)
 set name=pkg.summary value="SMB Server Functional Tests"
@@ -27,6 +27,9 @@ file path=opt/smbsrv-tests/include/default.cfg mode=0444
 file path=opt/smbsrv-tests/include/smbtor-excl-rpc.txt mode=0444
 file path=opt/smbsrv-tests/include/smbtor-excl-smb2.txt mode=0444
 dir  path=opt/smbsrv-tests/tests
+dir  path=opt/smbsrv-tests/tests/smb_sid
+file path=opt/smbsrv-tests/tests/smb_sid/large_sids_kern mode=0555
+file path=opt/smbsrv-tests/tests/smb_sid/large_sids_lib mode=0555
 dir  path=opt/smbsrv-tests/tests/smbtorture
 file path=opt/smbsrv-tests/tests/smbtorture/runst-rpc mode=0555
 file path=opt/smbsrv-tests/tests/smbtorture/runst-smb2 mode=0555
diff --git a/usr/src/test/smbsrv-tests/cmd/smbsrvtests.ksh b/usr/src/test/smbsrv-tests/cmd/smbsrvtests.ksh
index aab3ad0c8f..dd2053bc01 100644
--- a/usr/src/test/smbsrv-tests/cmd/smbsrvtests.ksh
+++ b/usr/src/test/smbsrv-tests/cmd/smbsrvtests.ksh
@@ -12,7 +12,7 @@
 #
 
 #
-# Copyright 2021 Tintri by DDN, Inc.  All rights reserved.
+# Copyright 2022 Tintri by DDN, Inc. All rights reserved.
 #
 
 # Run all the smbsrv-tests
@@ -49,3 +49,5 @@ set -x
 
 $SMBSRV_TESTS/tests/smbtorture/runst-smb2
 $SMBSRV_TESTS/tests/smbtorture/runst-rpc
+$SMBSRV_TESTS/tests/smb_sid/large_sids_lib
+$SMBSRV_TESTS/tests/smb_sid/large_sids_kern
diff --git a/usr/src/test/smbsrv-tests/tests/smb_sid/Makefile b/usr/src/test/smbsrv-tests/tests/smb_sid/Makefile
new file mode 100644
index 0000000000..d3e11c6ef5
--- /dev/null
+++ b/usr/src/test/smbsrv-tests/tests/smb_sid/Makefile
@@ -0,0 +1,66 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2012, 2016 by Delphix. All rights reserved.
+# Copyright 2022 Tintri by DDN, Inc. All rights reserved.
+#
+include $(SRC)/cmd/Makefile.cmd
+include $(SRC)/test/Makefile.com
+
+PROG = large_sids_lib large_sids_kern
+KERN_OBJS = smb_sid.o
+
+large_sids_lib := LDLIBS += -L$(ROOT)/usr/lib/smbsrv -lsmb
+large_sids_lib := LDFLAGS += -R/usr/lib/smbsrv
+large_sids_kern := LDLIBS64 += -lfakekernel
+
+smb_sid.o := CPPFLAGS.first += -I $(SRC)/lib/libfakekernel/common -D_FAKE_KERNEL
+
+ROOTOPTPKG = $(ROOT)/opt/smbsrv-tests
+TESTDIR = $(ROOTOPTPKG)/tests/smb_sid
+
+CMDS = $(PROG:%=$(TESTDIR)/%)
+$(CMDS) := FILEMODE = 0555
+
+CSTD = $(CSTD_GNU99)
+
+all: $(PROG)
+
+$(TESTDIR):
+	$(INS.dir)
+
+$(TESTDIR)/%: %
+	$(INS.file)
+
+%_lib: %.c
+	$(LINK.c) -o $@ $< $(LDLIBS)
+	$(POST_PROCESS)
+
+%_kern: %.c $(KERN_OBJS)
+	$(LINK64.c) -o $@ $^ $(LDLIBS64)
+	$(POST_PROCESS)
+
+smb_sid.c: $(SRC)/common/smbsrv/smb_sid.c
+	$(CP) $^ $@
+
+%.o: %.c
+	$(COMPILE64.c) $<
+
+install: all $(CMDS)
+
+clobber: clean
+	-$(RM) $(PROG)
+
+clean:
+	-$(RM) $(KERN_OBJS)
+
+$(CMDS): $(TESTDIR) $(PROG)
diff --git a/usr/src/test/smbsrv-tests/tests/smb_sid/large_sids.c b/usr/src/test/smbsrv-tests/tests/smb_sid/large_sids.c
new file mode 100644
index 0000000000..267fdd734c
--- /dev/null
+++ b/usr/src/test/smbsrv-tests/tests/smb_sid/large_sids.c
@@ -0,0 +1,104 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/*
+ * Copyright 2022 Tintri by DDN, Inc. All rights reserved.
+ */
+
+/*
+ * Test usr/src/common/smbsrv/smb_sid.c with large SIDs
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <smbsrv/smb_sid.h>
+#include <limits.h>
+
+void
+test_sid(const char *sidstr, uint8_t idauth, const uint32_t *subauths,
+    size_t subauth_cnt)
+{
+	char newstr[1024];
+	smb_sid_t *sid;
+	int i;
+
+	sid = smb_sid_fromstr(sidstr);
+	if (!smb_sid_isvalid(sid)) {
+		fprintf(stderr, "SID %s not valid: %p\n", sidstr, sid);
+		exit(1);
+	}
+
+	smb_sid_tostr(sid, newstr);
+
+	if (strncmp(sidstr, newstr, sizeof (newstr)) != 0) {
+		fprintf(stderr, "SID %s did not match decoded SID %s\n",
+		    sidstr, newstr);
+		exit(5);
+	}
+
+	if (subauths == NULL) {
+		smb_sid_free(sid);
+		return;
+	}
+
+	if (sid->sid_authority[5] != idauth) {
+		fprintf(stderr, "Wrong SID authority %u (expected %u): %s\n",
+		    sid->sid_authority, idauth, sidstr);
+		exit(2);
+	}
+
+	if (sid->sid_subauthcnt != subauth_cnt) {
+		fprintf(stderr, "Wrong subauthcnt %u (expected %u): %s\n",
+		    sid->sid_subauthcnt, subauth_cnt, sidstr);
+		exit(3);
+	}
+
+	for (i = 0; i < subauth_cnt; i++) {
+		if (sid->sid_subauth[i] != subauths[i]) {
+			fprintf(stderr,
+			    "Wrong subauthcnt %u (expected %u): %s\n",
+			    sid->sid_subauthcnt, subauth_cnt, sidstr);
+			exit(4);
+		}
+	}
+
+	smb_sid_free(sid);
+}
+
+int
+main(int argc, char *argv[])
+{
+	char sid[1024];
+	uint32_t subauths[NT_SID_SUBAUTH_MAX];
+	size_t len = sizeof (sid);
+	int off = 0;
+	int i, idauth;
+
+	if (argc > 1) {
+		test_sid(argv[1], 0, NULL, 0);
+		goto out;
+	}
+
+	for (idauth = 2; idauth <= UINT8_MAX; idauth += 11) {
+		off = snprintf(&sid[0], len, "S-1-%u", idauth);
+		for (i = 0; i < NT_SID_SUBAUTH_MAX; i++) {
+			subauths[i] = arc4random();
+			off += snprintf(&sid[off], len - off,
+			    "-%u", subauths[i]);
+		}
+		test_sid(sid, idauth, subauths, NT_SID_SUBAUTH_MAX);
+	}
+
+out:
+	printf("success!\n");
+	return (0);
+}