The Samba-Bugzilla – Attachment 18382 Details for
Bug 15674
cmdline_burn does not always burn secrets
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
patch for Samba 4.19 version 2
bug15674-v4-19-v2.patch (text/plain), 33.87 KB, created by
Douglas Bagnall
on 2024-07-20 06:35:19 UTC
(
hide
)
Description:
patch for Samba 4.19 version 2
Filename:
MIME Type:
Creator:
Douglas Bagnall
Created:
2024-07-20 06:35:19 UTC
Size:
33.87 KB
patch
obsolete
>From 5b0144fda6d21f34f0616e1c6e80fa5523f5efd6 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"/> > >+<!-- make room for long version numbers --> >+<xsl:param name="man.th.extra2.max.length">40</xsl:param> >+ > <!-- > Our ulink stylesheet omits @url part if content was specified > --> >-- >2.39.1 > > >From b041635b3f92c13aa536c320c5712ffc0e2d5ece 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 893b4918fd556961b9bc4ac0621f87ffc119bf7f 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) > >[jsutton@samba.org Fixed conflict in selftest/tests.py] >--- > selftest/tests.py | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/selftest/tests.py b/selftest/tests.py >index 76950f447eb..deb3c0be377 100644 >--- a/selftest/tests.py >+++ b/selftest/tests.py >@@ -487,3 +487,6 @@ plantestsuite("samba.unittests.compression.lzxpress_huffman", "none", > plantestsuite("samba.unittests.compression.lzxpress_plain", "none", > [os.path.join(bindir(), > "default/lib/compression/test_lzxpress_plain")]) >+ >+plantestsuite("samba.unittests.cmdline", "none", >+ [os.path.join(bindir(), "test_cmdline")]) >-- >2.39.1 > > >From 95d4c9444287d625ed7205fc8ae87047f9b70196 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 3c0102273ee90177a84a315f4843e58b24512127 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 a6a8794d37fd8c914eea48cad91bb09a78a80d5f 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 e8af97518427b3590d758de5e54623f208326df6 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 404d3e90b69a979b671ce8f6c0b5d97b645ec5bf 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 9fca8bbf9beedabd562a3c108f53f644f91776aa 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 612693742446299e166c7623394f9da08b84c69b 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 cd99c069e0c6aded74d42cfc0e178918de47e0cc 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 de79a9cb305cb216bdca5ee541640135015fadb5 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 afdf9d0a70f8f8638dfc5cb1075b1143ad483688 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 11b3e5a70f91cb6638c38852d2a32f5ac473699d 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 cda3ee6841f131b9e09ff5734e7ba256cc5f6d66 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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
jsutton
:
review+
Actions:
View
Attachments on
bug 15674
:
18376
|
18377
|
18381
| 18382