From: Douglas Bagnall Date: Tue, 9 Sep 2025 01:36:16 +0000 (+1200) Subject: CVE-2025-10230: s4/tests: check that wins hook sanitizes names X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=90b01ac9029169f0a185e74233a71b19a1b4acf0;p=thirdparty%2Fsamba.git 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 Reviewed-by: Gary Lockyer --- diff --git a/python/samba/tests/usage.py b/python/samba/tests/usage.py index eb43bba64f4..dae71ecfda8 100644 --- 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 index 00000000000..52388ce5749 --- /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 1ab178489b2..9b0bce4e26d 100755 --- a/selftest/target/Samba4.pm +++ b/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 diff --git a/source4/torture/nbt/wins.c b/source4/torture/nbt/wins.c index 8c847b5ac50..7d7321752d6 100644 --- 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 index 00000000000..f15379c28ca --- /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()