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)

Autobuild-User(v4-19-test): Jule Anger <janger@samba.org>
Autobuild-Date(v4-19-test): Tue Jul 23 08:43:59 UTC 2024 on atb-devel-224

diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index 3eabedfc1d803732355d413bae267f71916cb2f5..e3e068a11b6b5288d62dabaee9b01a3748fd457c 100644 (file)
--- 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 (file)
index 8048825..0000000
--- 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
-