From bff0064c286c1079d2d6d19befffdf83c8aa3069 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 21 Jun 2024 11:29:36 +1200 Subject: [PATCH 01/15] docs-xml:manpages: allow for longer version strings The default value (30) truncates "Samba 4.21.0pre1-DEVELOPERBUILD" to "Samba 4.21.0pre1-DEVELOPE" in the bottom left corner of the man page. ("Samba 4.21.0pre1-DEVELOPE" is only 25 bytes long, not 30, but let's not worry about that). On narrow terminals (< ~75 columns) this makes it more likely that the version string will run into the date string. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15672 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 7fb38aee129789cce28ddf54bd7234f8c5f57d97) --- docs-xml/xslt/man.xsl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs-xml/xslt/man.xsl b/docs-xml/xslt/man.xsl index e252b56d5e5..a1870079ba6 100644 --- a/docs-xml/xslt/man.xsl +++ b/docs-xml/xslt/man.xsl @@ -11,6 +11,9 @@ + +40 + -- 2.39.1 From 2176003566e787e94b739ec70be95cd4fcc2939c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 21 Jun 2024 09:21:43 +1200 Subject: [PATCH 02/15] cmdline:burn: '-U' does not imply secrets without '%' We return true from this function when a secret has been erased, and were accidentally treating as if it had secrets. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15671 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit f3b240da5c209a51fa43de23e8ecfea2f32bbfd5) --- lib/cmdline/cmdline.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index db962146bd2..aaaff446979 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -182,9 +182,11 @@ bool samba_cmdline_burn(int argc, char *argv[]) if (is_user) { q = strchr_m(p, '%'); - if (q != NULL) { - p = q; + if (q == NULL) { + /* -U without '%' has no secret */ + continue; } + p = q; } else { p += ulen; } -- 2.39.1 From 42ef9970b112b209f613d294232658ec5fb87be3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 17:04:47 +1200 Subject: [PATCH 03/15] selftest: run the cmdline tests that we already have BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit f17a2b1b25f2ffa5e3caeb8f81101e66b843cc29) --- selftest/tests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/selftest/tests.py b/selftest/tests.py index 0d5db685015..ad14bbdfc61 100644 --- a/selftest/tests.py +++ b/selftest/tests.py @@ -503,3 +503,5 @@ plantestsuite("samba.unittests.run_conditional_ace", "none", [os.path.join(bindir(), "test_run_conditional_ace")]) plantestsuite("samba.unittests.claim_conversion", "none", [os.path.join(bindir(), "test_claim_conversion")]) +plantestsuite("samba.unittests.cmdline", "none", + [os.path.join(bindir(), "test_cmdline")]) -- 2.39.1 From 773938a5116f49e5d48315174034c847844b16a5 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 15:05:03 +1200 Subject: [PATCH 04/15] cmdline:tests: extend cmdline_burn tests BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 05128a1f5f17c55a8d8da42c6c52c4235adf36d4) --- lib/cmdline/tests/test_cmdline.c | 32 +++++++++++++++++++++++++------- selftest/knownfail.d/cmdline | 1 + 2 files changed, 26 insertions(+), 7 deletions(-) create mode 100644 selftest/knownfail.d/cmdline diff --git a/lib/cmdline/tests/test_cmdline.c b/lib/cmdline/tests/test_cmdline.c index 16dd09c63fa..89d93996479 100644 --- a/lib/cmdline/tests/test_cmdline.c +++ b/lib/cmdline/tests/test_cmdline.c @@ -24,6 +24,7 @@ #include #include #include +#include "replace.h" #include "lib/cmdline/cmdline.h" @@ -62,19 +63,36 @@ static void torture_cmdline_sanity_check_bad(void **state) static void torture_cmdline_burn(void **state) { char arg1[] = "-U Administrator%secret"; - char arg2[] = "--user=Administrator%secret"; - char arg3[] = "--user=Administrator%super%secret"; - char arg4[] = "--password=super%secret"; + char arg2[] = "--no-no-no-not-secret=not%secret"; + char arg3[] = "--user=Administrator%secret"; + char arg4[] = "--user=Administrator%super%secret"; + char arg5[] = "--password=super%secret"; + char arg6[] = "--no-no-no-not-secret=not%secret"; + char arg7[] = "-U"; + char arg8[] = "fish%chips"; + char arg9[] = "--password"; + char arg10[] = "fish%chips"; + char arg11[] = "--password2"; + char arg12[] = "fish%chips"; - char *argv[] = { arg1, arg2, arg3, arg4, NULL }; - int argc = 4; + char *argv[] = { arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, + arg9, arg10, arg11, arg12, NULL }; + int argc = ARRAY_SIZE(argv) - 1; samba_cmdline_burn(argc, argv); assert_string_equal(arg1, "-U Administrator"); - assert_string_equal(arg2, "--user=Administrator"); + assert_string_equal(arg2, "--no-no-no-not-secret=not%secret"); assert_string_equal(arg3, "--user=Administrator"); - assert_string_equal(arg4, "--password"); + assert_string_equal(arg4, "--user=Administrator"); + assert_string_equal(arg5, "--password"); + assert_string_equal(arg6, "--no-no-no-not-secret=not%secret"); + assert_string_equal(arg7, "-U"); + assert_string_equal(arg8, "fish"); + assert_string_equal(arg9, "--password"); + assert_string_equal(arg10, ""); + assert_string_equal(arg11, "--password2"); + assert_string_equal(arg12, ""); } int main(int argc, char *argv[]) diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline new file mode 100644 index 00000000000..c9e4a86609c --- /dev/null +++ b/selftest/knownfail.d/cmdline @@ -0,0 +1 @@ +^samba.unittests.cmdline.torture_cmdline_burn.none.$ -- 2.39.1 From ce72f405e042dda74e5764d792ca8ba94b3323da Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 15:20:27 +1200 Subject: [PATCH 05/15] cmdline:burn: do not retain false memories If argv contains a secret option without an '=' (or in the case of "-U", the username is separated by space), we will get to the `if (strlen(p) == ulen) { continue; }` without resetting the found and is_user variables. This *sometimes* has the right effect, because the next string in argv ought to contain the secret. But in a case like {"--password", "1234567890"}, where the secret string is the same length as the option, we *again* take that branch and the password is not redacted, though the argument after it will be unless it is also of the same length. If we always set the flags at the start we avoid this. This makes things worse in the short term for secrets that are not the same length as their options, but we'll get to that in another commit soon. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 2f6020cf3dadf484251701040e09a10fba2f644e) --- lib/cmdline/cmdline.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index aaaff446979..3e0545e7b89 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -150,6 +150,9 @@ bool samba_cmdline_burn(int argc, char *argv[]) return false; } + found = false; + is_user = false; + /* * Take care that this list must be in longest-match * first order @@ -192,8 +195,6 @@ bool samba_cmdline_burn(int argc, char *argv[]) } memset_s(p, strlen(p), '\0', strlen(p)); - found = false; - is_user = false; burnt = true; } } -- 2.39.1 From 1487946950f5f5cdf89cd1fa5f6f5f9e468c17f2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 15:40:16 +1200 Subject: [PATCH 06/15] cmdline:burn: handle arguments separated from their --options We weren't treating "--password secret" the same as "--password=secret", which sometimes led to secrets not being redacted. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 53a1184525279741e116350a9b53da15cb2f41d0) --- lib/cmdline/cmdline.c | 27 ++++++++++++++++++++++++++- selftest/knownfail.d/cmdline | 1 - 2 files changed, 26 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/cmdline diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 3e0545e7b89..48801be2606 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -180,7 +180,32 @@ bool samba_cmdline_burn(int argc, char *argv[]) char *q = NULL; if (strlen(p) == ulen) { - continue; + /* + * The option string has no '=', so + * its argument will come in the NEXT + * argv member. If there is one, we + * can just step forward and take it, + * setting ulen to 0. + * + * {"--password=secret"} --> {"--password"} + * {"--password", "secret"} --> {"--password", ""} + * {"-Uadmin%secret"} --> {"-Uadmin"} + * {"-U", "admin%secret"} --> {"-U", "admin"} + */ + i++; + if (i == argc) { + /* + * this looks like an invalid + * command line, but that's + * for the caller to decide. + */ + return burnt; + } + p = argv[i]; + if (p == NULL) { + return false; + } + ulen = 0; } if (is_user) { diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline deleted file mode 100644 index c9e4a86609c..00000000000 --- a/selftest/knownfail.d/cmdline +++ /dev/null @@ -1 +0,0 @@ -^samba.unittests.cmdline.torture_cmdline_burn.none.$ -- 2.39.1 From c67e9a14561846dc8866b477fbd7dbe77644319b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 16:03:30 +1200 Subject: [PATCH 07/15] cmdline:burn: always return true if burnt Before we have been trying to cram three cases into a boolean return value: * cmdline had secrets, we burnt them -> true * cmdline had no secrets, all good -> false * cmdline has NULL string, WTF! emergency! -> false This return value is only used by Python which wants to know whether to go to the trouble of replacing the command line. If samba_cmdline_burn() returns false, no action is taken. If samba_cmdline_burn() burns a password and then hits a NULL, it would be better not to do nothing. It would be better to crash. And that is what Python will end up doing, by some talloc returning NULL triggering a MemoryError. What about the case like {"--foo", NULL, "-Ua%b"} where the secret comes after the NULL? That will still be ignored by Python, as it is by all C tools, but we are hoping that can't happen anyway. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit d3d8dffc0212662456a6251baee5afd432160fa2) --- lib/cmdline/cmdline.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 48801be2606..fa3bfefeced 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -147,7 +147,7 @@ bool samba_cmdline_burn(int argc, char *argv[]) for (i = 0; i < argc; i++) { p = argv[i]; if (p == NULL) { - return false; + return burnt; } found = false; @@ -203,7 +203,7 @@ bool samba_cmdline_burn(int argc, char *argv[]) } p = argv[i]; if (p == NULL) { - return false; + return burnt; } ulen = 0; } -- 2.39.1 From 78d6f1dd0c60508649af215dabba2a182e8e54ca Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 27 Jun 2024 16:33:16 +1200 Subject: [PATCH 08/15] cmdline:burn: localise some variables As this function increases in complexity, it helps to keep things close. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit f5233ddf974f9649d8a12b151b6843412eab489c) --- lib/cmdline/cmdline.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index fa3bfefeced..d20c606d503 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -138,24 +138,22 @@ void samba_cmdline_set_machine_account_fn( bool samba_cmdline_burn(int argc, char *argv[]) { bool burnt = false; - bool found = false; - bool is_user = false; - char *p = NULL; int i; - size_t ulen = 0; for (i = 0; i < argc; i++) { + bool found = false; + bool is_user = false; + size_t ulen = 0; + char *p = NULL; + p = argv[i]; if (p == NULL) { return burnt; } - found = false; - is_user = false; - /* * Take care that this list must be in longest-match - * first order + * first order (e.g. --password2 before --password). */ if (strncmp(p, "-U", 2) == 0) { ulen = 2; @@ -177,8 +175,6 @@ bool samba_cmdline_burn(int argc, char *argv[]) } if (found) { - char *q = NULL; - if (strlen(p) == ulen) { /* * The option string has no '=', so @@ -209,7 +205,7 @@ bool samba_cmdline_burn(int argc, char *argv[]) } if (is_user) { - q = strchr_m(p, '%'); + char *q = strchr_m(p, '%'); if (q == NULL) { /* -U without '%' has no secret */ continue; -- 2.39.1 From 99cc80074b36b39bfe7074af376d9bf163c6e5dc Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 29 Jun 2024 11:30:19 +1200 Subject: [PATCH 09/15] cmdline:burn: do not burn options starting --user-*, --password-* We have options that start with --user or --password that we don't want to burn. Some grepping says: 2 --user1 1 --user2 10 --user-allowed-to-authenticate-from 6 --user-allowed-to-authenticate-to 2 --user-allow-ntlm-auth 25 --user-authentication-policy 1 --user-config 4 --user-domgroups 5 --user-ext-name 2 --user-groups 6 --user-info 27 --username 1 --username2 2 --userou 1 --users 2 --user-sidinfo 6 --user-sids 14 --user-tgt-lifetime-mins 2 --password2 118 --password-file 2 --password-from-stdin # from here, grepping for strings around POPT_ constants 5 "user" 2 "user1" 2 "user2" 1 "userd" 1 "user-domgroups" 1 "user-groups" 1 "user-info" 2 "username" 1 "user-sidinfo" 1 "user-sids" 1 passwordd 4 "password" Not all of these use lib/cmdline, but I think most do, via Python which defers to cmdline_burn(). Note that there are options we should burn that aren't on this list, like --adminpass. That's another matter. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 6effed31899a1be8194a851e5a4023276b8a5f38) --- lib/cmdline/cmdline.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index d20c606d503..993b5aefe9e 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -135,6 +135,21 @@ void samba_cmdline_set_machine_account_fn( cli_credentials_set_machine_account_fn = fn; } +/* + * Are the strings p and option equal from the point of view of option + * parsing, meaning is the next character '\0' or '='. + */ +static bool strneq_cmdline_exact(const char *p, const char *option, size_t len) +{ + if (strncmp(p, option, len) == 0) { + if (p[len] == 0 || p[len] == '=') { + return true; + } + } + return false; +} + + bool samba_cmdline_burn(int argc, char *argv[]) { bool burnt = false; @@ -151,25 +166,21 @@ bool samba_cmdline_burn(int argc, char *argv[]) return burnt; } - /* - * Take care that this list must be in longest-match - * first order (e.g. --password2 before --password). - */ if (strncmp(p, "-U", 2) == 0) { ulen = 2; found = true; is_user = true; - } else if (strncmp(p, "--user", 6) == 0) { + } else if (strneq_cmdline_exact(p, "--user", 6)) { ulen = 6; found = true; is_user = true; - } else if (strncmp(p, "--password2", 11) == 0) { + } else if (strneq_cmdline_exact(p, "--password2", 11)) { ulen = 11; found = true; - } else if (strncmp(p, "--password", 10) == 0) { + } else if (strneq_cmdline_exact(p, "--password", 10)) { ulen = 10; found = true; - } else if (strncmp(p, "--newpassword", 13) == 0) { + } else if (strneq_cmdline_exact(p, "--newpassword", 13)) { ulen = 13; found = true; } -- 2.39.1 From 632a13710cab031f736f3d54f54379106df0269d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 29 Jun 2024 13:43:03 +1200 Subject: [PATCH 10/15] cmdline: test_cmdline tests more burning We have more secret arguments, like --client-password, --adminpass, so we are going to use an allowlist for options containing 'pass', but we don't want to burn the likes of --group=passionfruit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit c4df89e9640c1306aa390cdacaa974c870c3f5bb) --- lib/cmdline/tests/test_cmdline.c | 24 +++++++++++++++++++++++- selftest/knownfail.d/cmdline | 1 + 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/cmdline diff --git a/lib/cmdline/tests/test_cmdline.c b/lib/cmdline/tests/test_cmdline.c index 89d93996479..f9733546288 100644 --- a/lib/cmdline/tests/test_cmdline.c +++ b/lib/cmdline/tests/test_cmdline.c @@ -62,6 +62,7 @@ static void torture_cmdline_sanity_check_bad(void **state) static void torture_cmdline_burn(void **state) { + /* arg1 would require -U' Administrator%secret' */ char arg1[] = "-U Administrator%secret"; char arg2[] = "--no-no-no-not-secret=not%secret"; char arg3[] = "--user=Administrator%secret"; @@ -74,9 +75,23 @@ static void torture_cmdline_burn(void **state) char arg10[] = "fish%chips"; char arg11[] = "--password2"; char arg12[] = "fish%chips"; + char arg13[] = "--username=Admonisher % secretest"; + /* + * The next two are not used in samba (--client-password + * appears in a Heimdal script that won't use lib/cmdline even + * if built) and are burnt by virtue of not being in the allow + * list. + */ + char arg14[] = "--client-password=bean stew"; + char arg15[] = "--enpassant="; /* like --enpassant='', no effect on affect next arg */ + char arg16[] = "bean"; + char arg17[] = "--bean=password"; + char arg18[] = "--name"; + char arg19[] = "Compass Alompass"; char *argv[] = { arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, - arg9, arg10, arg11, arg12, NULL }; + arg9, arg10, arg11, arg12, arg13, arg14, arg15, arg16, arg17, + arg18, arg19, NULL }; int argc = ARRAY_SIZE(argv) - 1; samba_cmdline_burn(argc, argv); @@ -93,6 +108,13 @@ static void torture_cmdline_burn(void **state) assert_string_equal(arg10, ""); assert_string_equal(arg11, "--password2"); assert_string_equal(arg12, ""); + assert_string_equal(arg13, "--username=Admonisher "); + assert_string_equal(arg14, "--client-password"); + assert_string_equal(arg15, "--enpassant"); + assert_string_equal(arg16, "bean"); + assert_string_equal(arg17, "--bean=password"); + assert_string_equal(arg18, "--name"); + assert_string_equal(arg19, "Compass Alompass"); } int main(int argc, char *argv[]) diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline new file mode 100644 index 00000000000..c9e4a86609c --- /dev/null +++ b/selftest/knownfail.d/cmdline @@ -0,0 +1 @@ +^samba.unittests.cmdline.torture_cmdline_burn.none.$ -- 2.39.1 From ee89e9a194d7de66422571edc9dd6a4c9063aa8b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 29 Jun 2024 13:44:46 +1200 Subject: [PATCH 11/15] cmdline:burn: use allowlist to ensure more passwords burn We treat any option containing 'pass' with suspicion, unless we know it is OK. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit f1fbba6dc609590854c0d7c5e72b58fabc356695) --- lib/cmdline/cmdline.c | 86 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 9 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 993b5aefe9e..5fb9e306d99 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -149,6 +149,75 @@ static bool strneq_cmdline_exact(const char *p, const char *option, size_t len) return false; } +/* + * If an option looks like a password, and it isn't in the allow list, we + * will burn it. + * + * *ulen is set to zero if found is false (we don't need it in that case). + */ +static bool burn_possible_password(const char *p, size_t *ulen) +{ + size_t i, len; + static const char *allowed[] = { + "--bad-password-count-reset", + "--badpassword-frequency", + "--change-user-password", + "--force-initialized-passwords", + "--machine-pass", /* distinct from --machinepass */ + "--managed-password-interval", + "--no-pass", + "--no-pass2", + "--no-passthrough", + "--no-password", + "--passcmd", + "--passwd", + "--passwd_path", + "--password-file", + "--password-from-stdin", + "--random-password", + "--smbpasswd-style", + "--strip-passed-output", + "--with-smbpasswd-file", + }; + char *equals = NULL; + *ulen = 0; + + for (i = 0; i < ARRAY_SIZE(allowed); i++) { + bool safe; + len = strlen(allowed[i]); + safe = strneq_cmdline_exact(p, allowed[i], len); + if (safe) { + return false; + } + } + /* + * We have found a suspicious option, and we need to work out where to + * burn it from. It could be + * + * --secret-password=cow -> password after '=' + * --secret-password -> password is in next argument. + * + * but we also have the possibility of + * + * --cow=secret-password + * + * that is, the 'pass' in this option string is not in the option but + * the argument to it, which should not be burnt. + */ + equals = strchr(p, '='); + if (equals == NULL) { + *ulen = strlen(p); + } else { + char *pass = (strstr(p, "pass")); + if (pass > equals) { + /* this is --foo=pass, not --pass=foo */ + return false; + } + *ulen = equals - p; + } + DBG_NOTICE("burning command line argument %*s\n", (int)(*ulen), p); + return true; +} bool samba_cmdline_burn(int argc, char *argv[]) { @@ -174,15 +243,14 @@ bool samba_cmdline_burn(int argc, char *argv[]) ulen = 6; found = true; is_user = true; - } else if (strneq_cmdline_exact(p, "--password2", 11)) { - ulen = 11; - found = true; - } else if (strneq_cmdline_exact(p, "--password", 10)) { - ulen = 10; - found = true; - } else if (strneq_cmdline_exact(p, "--newpassword", 13)) { - ulen = 13; - found = true; + } else if (strncmp(p, "--", 2) == 0 && strstr(p, "pass")) { + /* + * We have many secret options like --password, + * --adminpass, --client-password, and we could easily + * add more, so we will use an allowlist to let the + * safe ones through (of which there are also many). + */ + found = burn_possible_password(p, &ulen); } if (found) { -- 2.39.1 From a85a1042d50346689acb109e56139f733b57d4c3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Jul 2024 11:23:36 +1200 Subject: [PATCH 12/15] cmdline:burn: explicitly burn --username This is the long form of -U in samba-tool. Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 63a83fb7bb312731047f361f89766e0be492f83e) --- lib/cmdline/cmdline.c | 4 ++++ selftest/knownfail.d/cmdline | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/cmdline diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 5fb9e306d99..81f37774dca 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -243,6 +243,10 @@ bool samba_cmdline_burn(int argc, char *argv[]) ulen = 6; found = true; is_user = true; + } else if (strneq_cmdline_exact(p, "--username", 10)) { + ulen = 10; + found = true; + is_user = true; } else if (strncmp(p, "--", 2) == 0 && strstr(p, "pass")) { /* * We have many secret options like --password, diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline deleted file mode 100644 index c9e4a86609c..00000000000 --- a/selftest/knownfail.d/cmdline +++ /dev/null @@ -1 +0,0 @@ -^samba.unittests.cmdline.torture_cmdline_burn.none.$ -- 2.39.1 From 577bff99115a9440cb917f34936237254c218765 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Jul 2024 11:50:43 +1200 Subject: [PATCH 13/15] cmdline:burn: add a note about short option combinations BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit 97be45f9ea3410392cd37eab5cfafd3ad00cfe57) --- lib/cmdline/cmdline.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 81f37774dca..3eabedfc1d8 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -236,6 +236,22 @@ bool samba_cmdline_burn(int argc, char *argv[]) } if (strncmp(p, "-U", 2) == 0) { + /* + * Note: this won't catch combinations of + * short options like + * `samba-tool -NUAdministrator%...`, which is + * not possible in general outside of the + * actual parser (consider for example + * `-NHUroot%password`, which parses as + * `-N -H 'Uroot%password'`). We don't know + * here which short options might take + * arguments. + * + * This is an argument for embedding redaction + * inside the parser (e.g. by adding a flag to + * the option definitions), but we decided not + * to do that in order to share cmdline_burn(). + */ ulen = 2; found = true; is_user = true; -- 2.39.1 From 6e660fb73de53f104a49f774053213d49cb933e8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Jul 2024 16:13:04 +1200 Subject: [PATCH 14/15] cmdline: samba-tool test for bad option warning Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton (cherry picked from commit d2b119e34b4e523a3bc6699e4d8a370bf8403d0b) --- python/samba/tests/samba_tool/help.py | 9 +++++++++ selftest/knownfail.d/cmdline | 2 ++ 2 files changed, 11 insertions(+) create mode 100644 selftest/knownfail.d/cmdline diff --git a/python/samba/tests/samba_tool/help.py b/python/samba/tests/samba_tool/help.py index fa7836d8432..16eb6b74c5d 100644 --- a/python/samba/tests/samba_tool/help.py +++ b/python/samba/tests/samba_tool/help.py @@ -79,3 +79,12 @@ class HelpTestCase(SambaToolCmdTest): known_commands = new_commands self.assertEqual(failed_commands, []) + + def test_bad_password_option(self): + """Do we get a warning with an invalid --pass option?""" + (result, out, err) = self.run_command(["samba-tool", + "processes", + "--pass-the-salt-please", + "pleeease"]) + self.assertIn("if '--pass-the-salt-please' is not misspelt", err) + self.assertIn("the appropriate list in is_password_option", err) diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline new file mode 100644 index 00000000000..80488258617 --- /dev/null +++ b/selftest/knownfail.d/cmdline @@ -0,0 +1,2 @@ +^samba.tests.samba_tool.help.samba.tests.samba_tool.help.HelpTestCase.test_bad_password_option + -- 2.39.1 From 5b8e5f9d8bf0a1f39831fed3b5603f9bf09b27b8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Jul 2024 14:31:58 +1200 Subject: [PATCH 15/15] cmdline:burn: list commands to always burn; warn on unknown We burn arguments to all unknown options containing "pass" (e.g. "--passionate=false") in case they are a password option, but is bad in the case where the unknown option takes no argument but the next option *is* a password (like "--overpass --password2 barney". In that case "--password2" would be burnt and not "barney". The burning behaviour doesn't change with this commit, but users will now see an error message explaining that the option was unknown. This is not so much aimed at end users -- for who an invalid option will hopefully lead to --help like output -- but to developers who add a new "pass" option. This also slightly speeds up the processing of known password options, which is a little bit important because we are in a race to replace the command line in /proc before an attacker sees it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674 Signed-off-by: Douglas Bagnall Reviewed-by: Jo Sutton Autobuild-User(master): Douglas Bagnall Autobuild-Date(master): Wed Jul 10 06:28:08 UTC 2024 on atb-devel-224 (cherry picked from commit 86843685419921e28c37f3c1b33011f14940e02f) --- lib/cmdline/cmdline.c | 58 +++++++++++++++++++++++++++++++----- selftest/knownfail.d/cmdline | 2 -- 2 files changed, 51 insertions(+), 9 deletions(-) delete mode 100644 selftest/knownfail.d/cmdline diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 3eabedfc1d8..e3e068a11b6 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -150,14 +150,36 @@ static bool strneq_cmdline_exact(const char *p, const char *option, size_t len) } /* - * If an option looks like a password, and it isn't in the allow list, we - * will burn it. + * Return true if the argument to the option should be redacted. * - * *ulen is set to zero if found is false (we don't need it in that case). + * The option name is presumed to contain the substring "pass". It is checked + * against a list of options that specify secrets. If it is there, the value + * should be redacted and we return early. + * + * Otherwise, it is checked against a list of known safe options. If it is + * there, we return false. + * + * If the option is not in either list, we assume it might be secret and + * redact the argument, but warn loadly about it. The hope is that developers + * will see what they're doing and add the option to the appropriate list. + * + * If true is returned, *ulen will be set to the apparent length of the + * option. It is set to zero if false is returned (we don't need it in that + * case). */ -static bool burn_possible_password(const char *p, size_t *ulen) +static bool is_password_option(const char *p, size_t *ulen) { size_t i, len; + static const char *must_burn[] = { + "--password", + "--newpassword", + "--password2", + "--adminpass", + "--dnspass", + "--machinepass", + "--krbtgtpass", + "--fixed-password", + }; static const char *allowed[] = { "--bad-password-count-reset", "--badpassword-frequency", @@ -179,9 +201,20 @@ static bool burn_possible_password(const char *p, size_t *ulen) "--strip-passed-output", "--with-smbpasswd-file", }; + char *equals = NULL; *ulen = 0; + for (i = 0; i < ARRAY_SIZE(must_burn); i++) { + bool secret; + len = strlen(must_burn[i]); + secret = strneq_cmdline_exact(p, must_burn[i], len); + if (secret) { + *ulen = len; + return true; + } + } + for (i = 0; i < ARRAY_SIZE(allowed); i++) { bool safe; len = strlen(allowed[i]); @@ -215,7 +248,18 @@ static bool burn_possible_password(const char *p, size_t *ulen) } *ulen = equals - p; } - DBG_NOTICE("burning command line argument %*s\n", (int)(*ulen), p); + /* + * This message will be seen with Python tools when an option + * is misspelt, but not with C tools, because in C burning + * happens after the command line is parsed, while in Python + * it happens before (on a copy of argv). + * + * In either case it will appear for a newly added option, and + * we hope developers will notice it before pushing. + */ + DBG_ERR("\nNote for developers: if '%*s' is not misspelt, it should be " + "added to the appropriate list in is_password_option().\n\n", + (int)(*ulen), p); return true; } @@ -266,11 +310,11 @@ bool samba_cmdline_burn(int argc, char *argv[]) } else if (strncmp(p, "--", 2) == 0 && strstr(p, "pass")) { /* * We have many secret options like --password, - * --adminpass, --client-password, and we could easily + * --adminpass, --newpassword, and we could easily * add more, so we will use an allowlist to let the * safe ones through (of which there are also many). */ - found = burn_possible_password(p, &ulen); + found = is_password_option(p, &ulen); } if (found) { diff --git a/selftest/knownfail.d/cmdline b/selftest/knownfail.d/cmdline deleted file mode 100644 index 80488258617..00000000000 --- a/selftest/knownfail.d/cmdline +++ /dev/null @@ -1,2 +0,0 @@ -^samba.tests.samba_tool.help.samba.tests.samba_tool.help.HelpTestCase.test_bad_password_option - -- 2.39.1