CVE-2025-10230: s4/tests: check that wins hook sanitizes names

An smb.conf can contain a 'wins hook' parameter, which names a script
to run when a WINS name is changed. The man page says

    The second argument is the NetBIOS name. If the name is not a
    legal name then the wins hook is not called. Legal names contain
    only letters, digits, hyphens, underscores and periods.

but it turns out the legality check is not performed if the WINS
server in question is the source4 nbt one. It is not expected that
people will run this server, but they can. This is bad because the
name is passed unescaped into a shell command line, allowing command
injection.

For this test we don't care whether the WINS server is returning an
error code, just whether it is running the wins hook. The tests show
it often runs the hook it shouldn't, though some characters are
incidentally blocked because the name has to fit in a DN before it
gets to the hook, and DNs have a few syntactic restrictions (e.g.,
blocking '<', '>', and ';').

The source3 WINS server that is used by Samba when not run as a DC is
not affected and not here tested.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15903

Signed-off-by: Douglas Bagnall <[email protected]>
Reviewed-by: Gary Lockyer <[email protected]>

Author: douglasbagnall

python/samba/tests/usage.py

@@ -73,6 +73,7 @@
     'lib/ldb/tests/python/api.py',
     'source4/selftest/tests.py',
     'buildtools/bin/waf',
+    'testprogs/blackbox/wins_hook_test',
     'selftest/tap2subunit',
     'script/show_test_time',
     'source4/scripting/bin/subunitrun',
@@ -89,6 +90,7 @@
     'selftest/tap2subunit',
     'wintest/test-s3.py',
     'wintest/test-s4-howto.py',
+    'testprogs/blackbox/wins_hook_test',
 }
 
 

selftest/knownfail.d/samba4.nbt.wins.wins_bad_names

@@ -0,0 +1 @@
+samba4.nbt.wins.wins_bad_names

selftest/target/Samba4.pm

@@ -1637,6 +1637,7 @@ sub provision_ad_dc_ntvfs($$$)
 	ldap server require strong auth = allow_sasl_without_tls_channel_bindings
 	raw NTLMv2 auth = yes
 	lsa over netlogon = yes
+	wins hook = $ENV{SRCDIR_ABS}/testprogs/blackbox/wins_hook_test
         rpc server port = 1027
         auth event notification = true
 	dsdb event notification = true

source4/torture/nbt/wins.c

@@ -31,6 +31,10 @@
 #include "torture/nbt/proto.h"
 #include "param/param.h"
 
+/* rcode used when you don't want to check the rcode */
+#define WINS_TEST_RCODE_WE_DONT_CARE 255
+
+
 #define CHECK_VALUE(tctx, v, correct) \
 	torture_assert_int_equal(tctx, v, correct, "Incorrect value")
 
@@ -137,7 +141,9 @@ static bool nbt_test_wins_name(struct torture_context *tctx, const char *address
 					address));
 
 		CHECK_STRING(tctx, io.out.wins_server, address);
-		CHECK_VALUE(tctx, io.out.rcode, 0);
+		if (register_rcode != WINS_TEST_RCODE_WE_DONT_CARE) {
+			CHECK_VALUE(tctx, io.out.rcode, 0);
+		}
 
 		torture_comment(tctx, "register the name correct address\n");
 		name_register.in.name		= *name;
@@ -185,7 +191,9 @@ static bool nbt_test_wins_name(struct torture_context *tctx, const char *address
 			talloc_asprintf(tctx, "Bad response from %s for name register\n",
 					address));
 
-		CHECK_VALUE(tctx, name_register.out.rcode, 0);
+		if (register_rcode != WINS_TEST_RCODE_WE_DONT_CARE) {
+			CHECK_VALUE(tctx, name_register.out.rcode, 0);
+		}
 		CHECK_STRING(tctx, name_register.out.reply_addr, myaddress);
 	}
 
@@ -203,7 +211,9 @@ static bool nbt_test_wins_name(struct torture_context *tctx, const char *address
 	torture_assert_ntstatus_ok(tctx, status, talloc_asprintf(tctx, "Bad response from %s for name register", address));
 
 	CHECK_STRING(tctx, io.out.wins_server, address);
-	CHECK_VALUE(tctx, io.out.rcode, register_rcode);
+	if (register_rcode != WINS_TEST_RCODE_WE_DONT_CARE) {
+		CHECK_VALUE(tctx, io.out.rcode, register_rcode);
+	}
 
 	if (register_rcode != NBT_RCODE_OK) {
 		return true;
@@ -532,6 +542,124 @@ static bool nbt_test_wins(struct torture_context *tctx)
 	return ret;
 }
 
+/*
+ * Test that the WINS server does not call 'wins hook' when the name
+ * contains dodgy characters.
+ */
+static bool nbt_test_wins_bad_names(struct torture_context *tctx)
+{
+	const char *address = NULL;
+	const char *wins_hook_file = NULL;
+	bool ret = true;
+	int err;
+	bool ok;
+	struct nbt_name name = {};
+	size_t i, j;
+	FILE *fh = NULL;
+
+	struct {
+		const char *name;
+		bool should_succeed;
+	} test_cases[] = {
+		{"NORMAL", true},
+		{"|look|", false},
+		{"look&true", false},
+		{"look\\;false", false},
+		{"&ls>foo", false},  /* already fails due to DN syntax */
+		{"has spaces", false},
+		{"hyphen-dot.0", true},
+	};
+
+	wins_hook_file = talloc_asprintf(tctx, "%s/wins_hook_writes_here",
+					 getenv("SELFTEST_TMPDIR"));
+
+	if (!torture_nbt_get_name(tctx, &name, &address)) {
+		return false;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
+		err =  unlink(wins_hook_file);
+		if (err != 0 && errno != ENOENT) {
+			/* we expect ENOENT, but nothing else */
+			torture_comment(tctx,
+					"unlink %zu of '%s' failed\n",
+					i, wins_hook_file);
+		}
+
+		name.name = test_cases[i].name;
+		name.type = NBT_NAME_CLIENT;
+		ok = nbt_test_wins_name(tctx, address,
+					&name,
+					NBT_NODE_H,
+					true,
+					WINS_TEST_RCODE_WE_DONT_CARE
+			);
+		if (ok == false) {
+			/*
+			 * This happens when the name interferes with
+			 * the DN syntax when it is put in winsdb.
+			 *
+			 * The wins hook will not be reached.
+			 */
+			torture_comment(tctx, "tests for '%s' failed\n",
+					name.name);
+		}
+
+		/*
+		 * poll for the file being created by the wins hook.
+		 */
+		for (j = 0; j < 10; j++) {
+			usleep(200000);
+			fh = fopen(wins_hook_file, "r");
+			if (fh != NULL) {
+				break;
+			}
+		}
+
+		if (fh == NULL) {
+			if (errno == ENOENT) {
+				if (test_cases[i].should_succeed) {
+					torture_comment(
+						tctx,
+						"wins hook for '%s' failed\n",
+						test_cases[i].name);
+					ret = false;
+				}
+			} else {
+				torture_comment(
+					tctx,
+					"wins hook for '%s' unexpectedly failed with %d\n",
+					test_cases[i].name,
+					errno);
+				ret = false;
+			}
+		} else {
+			char readback[17] = {0};
+			size_t n = fread(readback, 1, 16, fh);
+			torture_comment(tctx,
+					"wins hook wrote '%s' read '%.*s'\n",
+					test_cases[i].name,
+					(int)n, readback);
+
+			if (! test_cases[i].should_succeed) {
+				torture_comment(tctx,
+						"wins hook for '%s' should fail\n",
+						test_cases[i].name);
+				ret = false;
+			}
+			fclose(fh);
+		}
+	}
+	err = unlink(wins_hook_file);
+	if (err != 0 && errno != ENOENT) {
+		torture_comment(tctx, "final unlink of '%s' failed\n",
+				wins_hook_file);
+	}
+	torture_assert(tctx, ret, "wins hook failure\n");
+	return ret;
+}
+
+
 /*
   test WINS operations
 */
@@ -540,6 +668,8 @@ struct torture_suite *torture_nbt_wins(TALLOC_CTX *mem_ctx)
 	struct torture_suite *suite = torture_suite_create(mem_ctx, "wins");
 
 	torture_suite_add_simple_test(suite, "wins", nbt_test_wins);
+	torture_suite_add_simple_test(suite, "wins_bad_names",
+				      nbt_test_wins_bad_names);
 
 	return suite;
 }

testprogs/blackbox/wins_hook_test

@@ -0,0 +1,15 @@
+#!/usr/bin/python3
+
+import os
+import sys
+
+filename = f"{os.environ['SELFTEST_TMPDIR']}/wins_hook_writes_here"
+
+f = open(filename, 'wb')
+
+# Some names may truncate argv (e.g. '&'), for which we leave the file
+# empty.
+if len(sys.argv) > 2:
+    f.write(os.fsencode(sys.argv[2]))
+
+f.close()