Attachment 18381 Details for Bug 15674 – patch for Samba v4.20 version 2

[patch] patch for Samba v4.20 version 2

bug15674-v4-20-v2.patch (text/plain), 33.81 KB, created by Douglas Bagnall on 2024-07-20 06:34:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Douglas Bagnall

Created: 2024-07-20 06:34:30 UTC

Size: 33.81 KB

patch

obsolete

>From bff0064c286c1079d2d6d19befffdf83c8aa3069 Mon Sep 17 00:00:00 2001
>From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 @@
> <xsl:param name="use.id.as.filename" select="1"/>
> <xsl:param name="man.endnotes.are.numbered" select="0"/>
> 
>+
>+<xsl:param name="man.th.extra2.max.length">40</xsl:param>
>+
> 
>-- 
>2.39.1
>
>
>From 2176003566e787e94b739ec70be95cd4fcc2939c Mon Sep 17 00:00:00 2001
>From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <cmocka.h>
> #include <time.h>
> #include <sys/time.h>
>+#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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15674
>(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 <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Jo Sutton <josutton@catalyst.net.nz>
>
>Autobuild-User(master): Douglas Bagnall <dbagnall@samba.org>
>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
>

Flags:

jsutton: review+

Actions: View

Attachments on bug 15674: 18376 | 18377 | 18381 | 18382