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 <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>

diff --git a/python/samba/tests/usage.py b/python/samba/tests/usage.py
index 3312bfe37465d1b89cde2aba3bd87d76c11a3f5a..afee5a5ebcd5ff1218027245923d4e059d5921ef 100644 (file)
--- a/python/samba/tests/usage.py
+++ b/python/samba/tests/usage.py
@@ -73,6 +73,7 @@ EXCLUDE_USAGE = {
     '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 @@ EXCLUDE_HELP = {
     'selftest/tap2subunit',
     'wintest/test-s3.py',
     'wintest/test-s4-howto.py',
+    'testprogs/blackbox/wins_hook_test',
 }
 
 

diff --git a/selftest/knownfail.d/samba4.nbt.wins.wins_bad_names b/selftest/knownfail.d/samba4.nbt.wins.wins_bad_names
new file mode 100644 (file)
index 0000000..52388ce
--- /dev/null
+++ b/selftest/knownfail.d/samba4.nbt.wins.wins_bad_names
@@ -0,0 +1 @@
+samba4.nbt.wins.wins_bad_names

diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
index e917f65fc36ae24447ea62bfbecaae5d2ae40056..110f4c5072b0d60427346a08365ea4335737ca26 100755 (executable)
--- a/selftest/target/Samba4.pm
+++ b/selftest/target/Samba4.pm
@@ -1628,6 +1628,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

diff --git a/source4/torture/nbt/wins.c b/source4/torture/nbt/wins.c
index 8c847b5ac50989801263b2aaea9c11de097d8851..7d7321752d6417f83c26182580ba2154f9eea6db 100644 (file)
--- a/source4/torture/nbt/wins.c
+++ b/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;
 }

diff --git a/testprogs/blackbox/wins_hook_test b/testprogs/blackbox/wins_hook_test
new file mode 100755 (executable)
index 0000000..f15379c
--- /dev/null
+++ b/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()