From 7b94d37e0b42fb574032d7d26533d2ead6eaf9aa Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 15:27:00 +1200 Subject: [PATCH 1/7] lib/cmdline: Return if the commandline was redacted in samba_cmdline_burn() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 848fea1a01a4ddc1598150823d5d0784d3ef0be4) --- lib/cmdline/cmdline.c | 7 +++++-- lib/cmdline/cmdline.h | 4 +++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 9f4e964f289..2f11804fe5e 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -134,8 +134,9 @@ void samba_cmdline_set_machine_account_fn( cli_credentials_set_machine_account_fn = fn; } -void samba_cmdline_burn(int argc, char *argv[]) +bool samba_cmdline_burn(int argc, char *argv[]) { + bool burnt = false; bool found = false; bool is_user = false; char *p = NULL; @@ -145,7 +146,7 @@ void samba_cmdline_burn(int argc, char *argv[]) for (i = 0; i < argc; i++) { p = argv[i]; if (p == NULL) { - return; + return false; } if (strncmp(p, "-U", 2) == 0) { @@ -180,8 +181,10 @@ void samba_cmdline_burn(int argc, char *argv[]) memset_s(p, strlen(p), '\0', strlen(p)); found = false; is_user = false; + burnt = true; } } + return burnt; } static bool is_popt_table_end(const struct poptOption *o) diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h index e254a1db5c3..b9cb4764bea 100644 --- a/lib/cmdline/cmdline.h +++ b/lib/cmdline/cmdline.h @@ -147,8 +147,10 @@ void samba_cmdline_set_machine_account_fn( * @param[in] argc The number of arguments. * * @param[in] argv[] The argument array we should remove secrets from. + * + * @return true if a password was removed, false otherwise. */ -void samba_cmdline_burn(int argc, char *argv[]); +bool samba_cmdline_burn(int argc, char *argv[]); /** * @brief Sanity check the command line options. -- 2.25.1 From ba7428836a6b072faebd52addc59637d7c5be959 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 14:31:30 +1200 Subject: [PATCH 2/7] python: Move PyList_AsStringList to common code so we can reuse BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit fd81759e2ed44cac3bc67243a39256f953969103) --- python/modules.c | 35 +++++++++++++++++++++++++++++++++++ python/modules.h | 7 +++++++ python/pyglue.c | 1 + source4/auth/pyauth.c | 34 ---------------------------------- source4/auth/wscript_build | 4 +++- 5 files changed, 46 insertions(+), 35 deletions(-) diff --git a/python/modules.c b/python/modules.c index d8b330b6b28..83dc91c08a1 100644 --- a/python/modules.c +++ b/python/modules.c @@ -71,3 +71,38 @@ error: Py_XDECREF(mod_sys); return false; } + +const char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, + const char *paramname) +{ + const char **ret; + Py_ssize_t i; + if (!PyList_Check(list)) { + PyErr_Format(PyExc_TypeError, "%s is not a list", paramname); + return NULL; + } + ret = talloc_array(NULL, const char *, PyList_Size(list)+1); + if (ret == NULL) { + PyErr_NoMemory(); + return NULL; + } + + for (i = 0; i < PyList_Size(list); i++) { + const char *value; + Py_ssize_t size; + PyObject *item = PyList_GetItem(list, i); + if (!PyUnicode_Check(item)) { + PyErr_Format(PyExc_TypeError, "%s should be strings", paramname); + return NULL; + } + value = PyUnicode_AsUTF8AndSize(item, &size); + if (value == NULL) { + talloc_free(ret); + return NULL; + } + ret[i] = talloc_strndup(ret, value, size); + } + ret[i] = NULL; + return ret; +} + diff --git a/python/modules.h b/python/modules.h index 75108d77907..331adde0230 100644 --- a/python/modules.h +++ b/python/modules.h @@ -20,7 +20,14 @@ #ifndef __SAMBA_PYTHON_MODULES_H__ #define __SAMBA_PYTHON_MODULES_H__ +#include + bool py_update_path(void); /* discard signature of 'func' in favour of 'target_sig' */ #define PY_DISCARD_FUNC_SIG(target_sig, func) (target_sig)(void(*)(void))func + +const char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, + const char *paramname); + #endif /* __SAMBA_PYTHON_MODULES_H__ */ + diff --git a/python/pyglue.c b/python/pyglue.c index 64be7389b70..e135adefb5d 100644 --- a/python/pyglue.c +++ b/python/pyglue.c @@ -20,6 +20,7 @@ #include #include "python/py3compat.h" #include "includes.h" +#include "python/modules.h" #include "version.h" #include "param/pyparam.h" #include "lib/socket/netif.h" diff --git a/source4/auth/pyauth.c b/source4/auth/pyauth.c index ec6065d7d0a..ee36b5e5efa 100644 --- a/source4/auth/pyauth.c +++ b/source4/auth/pyauth.c @@ -350,40 +350,6 @@ static PyObject *py_session_info_set_unix(PyObject *module, } -static const char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, - const char *paramname) -{ - const char **ret; - Py_ssize_t i; - if (!PyList_Check(list)) { - PyErr_Format(PyExc_TypeError, "%s is not a list", paramname); - return NULL; - } - ret = talloc_array(NULL, const char *, PyList_Size(list)+1); - if (ret == NULL) { - PyErr_NoMemory(); - return NULL; - } - - for (i = 0; i < PyList_Size(list); i++) { - const char *value; - Py_ssize_t size; - PyObject *item = PyList_GetItem(list, i); - if (!PyUnicode_Check(item)) { - PyErr_Format(PyExc_TypeError, "%s should be strings", paramname); - return NULL; - } - value = PyUnicode_AsUTF8AndSize(item, &size); - if (value == NULL) { - talloc_free(ret); - return NULL; - } - ret[i] = talloc_strndup(ret, value, size); - } - ret[i] = NULL; - return ret; -} - static PyObject *PyAuthContext_FromContext(struct auth4_context *auth_context) { return pytalloc_reference(&PyAuthContext, auth_context); diff --git a/source4/auth/wscript_build b/source4/auth/wscript_build index 9b94143ba7c..57bb9f751c6 100644 --- a/source4/auth/wscript_build +++ b/source4/auth/wscript_build @@ -85,10 +85,12 @@ pytalloc_util = bld.pyembed_libname('pytalloc-util') pyparam_util = bld.pyembed_libname('pyparam_util') pyldb_util = bld.pyembed_libname('pyldb-util') pycredentials = 'pycredentials' +libpython = bld.pyembed_libname('LIBPYTHON') + bld.SAMBA_PYTHON('pyauth', source='pyauth.c', public_deps='auth_system_session', - deps='samdb %s %s %s %s auth4' % (pytalloc_util, pyparam_util, pyldb_util, pycredentials), + deps=f'samdb {pytalloc_util} {pyparam_util} {pyldb_util} {pycredentials} {libpython} auth4', realname='samba/auth.so' ) -- 2.25.1 From e53a3407926df32b248eacb2955c19e313e618b9 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 14:32:46 +1200 Subject: [PATCH 3/7] python: Remove const from PyList_AsStringList() The returned strings are not owned by python, so need not be const. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 5afd206d1d8f0344a2f1fa7a238204d1fb164eda) --- python/modules.c | 8 ++++---- python/modules.h | 4 ++-- source4/auth/pyauth.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/python/modules.c b/python/modules.c index 83dc91c08a1..ca563ff07d2 100644 --- a/python/modules.c +++ b/python/modules.c @@ -72,16 +72,16 @@ error: return false; } -const char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, - const char *paramname) +char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, + const char *paramname) { - const char **ret; + char **ret; Py_ssize_t i; if (!PyList_Check(list)) { PyErr_Format(PyExc_TypeError, "%s is not a list", paramname); return NULL; } - ret = talloc_array(NULL, const char *, PyList_Size(list)+1); + ret = talloc_array(NULL, char *, PyList_Size(list)+1); if (ret == NULL) { PyErr_NoMemory(); return NULL; diff --git a/python/modules.h b/python/modules.h index 331adde0230..356937d71f9 100644 --- a/python/modules.h +++ b/python/modules.h @@ -26,8 +26,8 @@ bool py_update_path(void); /* discard signature of 'func' in favour of 'target_sig' */ #define PY_DISCARD_FUNC_SIG(target_sig, func) (target_sig)(void(*)(void))func -const char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, - const char *paramname); +char **PyList_AsStringList(TALLOC_CTX *mem_ctx, PyObject *list, + const char *paramname); #endif /* __SAMBA_PYTHON_MODULES_H__ */ diff --git a/source4/auth/pyauth.c b/source4/auth/pyauth.c index ee36b5e5efa..b41108b7072 100644 --- a/source4/auth/pyauth.c +++ b/source4/auth/pyauth.c @@ -367,7 +367,7 @@ static PyObject *py_auth_context_new(PyTypeObject *type, PyObject *args, PyObjec struct tevent_context *ev; struct ldb_context *ldb = NULL; NTSTATUS nt_status; - const char **methods; + const char *const *methods; const char *const kwnames[] = {"lp_ctx", "ldb", "methods", NULL}; @@ -413,7 +413,7 @@ static PyObject *py_auth_context_new(PyTypeObject *type, PyObject *args, PyObjec mem_ctx, ev, NULL, lp_ctx, &auth_context); } else { if (py_methods != Py_None) { - methods = PyList_AsStringList(mem_ctx, py_methods, "methods"); + methods = (const char * const *)PyList_AsStringList(mem_ctx, py_methods, "methods"); if (methods == NULL) { talloc_free(mem_ctx); return NULL; -- 2.25.1 From da7ed14b8fb0ae38d856a818a80217035777891a Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 13:29:22 +1200 Subject: [PATCH 4/7] python: Add glue.burn_commandline() method This uses samba_cmdline_burn() to as to have common command line redaction code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 3f9e455898554b726bf1689f743b2d9cb6b59537) --- python/pyglue.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ python/wscript | 1 + 2 files changed, 60 insertions(+) diff --git a/python/pyglue.c b/python/pyglue.c index e135adefb5d..8378aa797d4 100644 --- a/python/pyglue.c +++ b/python/pyglue.c @@ -26,6 +26,7 @@ #include "lib/socket/netif.h" #include "lib/util/debug.h" #include "librpc/ndr/ndr_private.h" +#include "lib/cmdline/cmdline.h" void init_glue(void); static PyObject *PyExc_NTSTATUSError; @@ -462,6 +463,62 @@ static PyObject *py_strstr_m(PyObject *self, PyObject *args) return result; } +static PyObject *py_get_burnt_commandline(PyObject *self, PyObject *args) +{ + PyObject *cmdline_as_list, *ret; + char *burnt_cmdline = NULL; + Py_ssize_t i, argc; + char **argv = NULL; + TALLOC_CTX *frame = talloc_stackframe(); + bool burnt; + + if (!PyArg_ParseTuple(args, "O!", &PyList_Type, &cmdline_as_list)) + { + TALLOC_FREE(frame); + return NULL; + } + + argc = PyList_GET_SIZE(cmdline_as_list); + + if (argc == 0) { + TALLOC_FREE(frame); + Py_RETURN_NONE; + } + + argv = PyList_AsStringList(frame, cmdline_as_list, "sys.argv"); + if (argv == NULL) { + return NULL; + } + + burnt = samba_cmdline_burn(argc, argv); + if (!burnt) { + TALLOC_FREE(frame); + Py_RETURN_NONE; + } + + for (i = 0; i < argc; i++) { + if (i == 0) { + burnt_cmdline = talloc_strdup(frame, + argv[i]); + } else { + burnt_cmdline + = talloc_asprintf_append(burnt_cmdline, + " %s", + argv[i]); + } + if (burnt_cmdline == NULL) { + PyErr_NoMemory(); + TALLOC_FREE(frame); + return NULL; + } + } + + ret = PyUnicode_FromString(burnt_cmdline); + TALLOC_FREE(frame); + + return ret; +} + static PyMethodDef py_misc_methods[] = { { "generate_random_str", (PyCFunction)py_generate_random_str, METH_VARARGS, "generate_random_str(len) -> string\n" @@ -521,6 +578,8 @@ static PyMethodDef py_misc_methods[] = { METH_NOARGS, "is Samba built with selftest enabled?" }, { "ndr_token_max_list_size", (PyCFunction)py_ndr_token_max_list_size, METH_NOARGS, "How many NDR internal tokens is too many for this build?" }, + { "get_burnt_commandline", (PyCFunction)py_get_burnt_commandline, + METH_VARARGS, "Return a redacted commandline to feed to setproctitle (None if no redaction required)" }, {0} }; diff --git a/python/wscript b/python/wscript index abe88b00129..a4573c57c6e 100644 --- a/python/wscript +++ b/python/wscript @@ -117,6 +117,7 @@ def build(bld): samba-util netif ndr + cmdline %s ''' % (pyparam_util, pytalloc_util), realname='samba/_glue.so') -- 2.25.1 From ab455f794f774cda6fa6d7935df32a0867713752 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 13:30:39 +1200 Subject: [PATCH 5/7] samba-tool: Use samba.glue.get_burnt_cmdline rather than regex This use avoids having two different methods to match on command-line passwords. We already have a dependency on the setproctitle python module, and this does not change as the (C) libbsd setproctitle() can't be run from within a python module. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit a53ebc288f47329c997d52325eeeb5e91ce43b75) --- python/samba/getopt.py | 69 ++++++++++------------------------ python/samba/tests/cred_opt.py | 14 +++++-- 2 files changed, 30 insertions(+), 53 deletions(-) diff --git a/python/samba/getopt.py b/python/samba/getopt.py index ff8aead3f8d..e9ff3de5b34 100644 --- a/python/samba/getopt.py +++ b/python/samba/getopt.py @@ -29,7 +29,7 @@ from samba.credentials import ( MUST_USE_KERBEROS, ) import sys - +from samba._glue import get_burnt_commandline OptionError = optparse.OptionValueError @@ -40,6 +40,25 @@ class SambaOptions(optparse.OptionGroup): def __init__(self, parser): from samba import fault_setup fault_setup() + + # This removes passwords from the commandline via + # setproctitle() but makes no change to python sys.argv so we + # can continue to process as normal + # + # get_burnt_commandline returns None if no change is needed + new_proctitle = get_burnt_commandline(sys.argv) + if new_proctitle is not None: + try: + import setproctitle + setproctitle.setproctitle(new_proctitle) + + except ModuleNotFoundError: + msg = ("WARNING: Using passwords on command line is insecure. " + "Installing the setproctitle python module will hide " + "these from shortly after program start.\n") + sys.stderr.write(msg) + sys.stderr.flush() + from samba.param import LoadParm optparse.OptionGroup.__init__(self, parser, "Samba Common Options") self.add_option("-s", "--configfile", action="callback", @@ -203,53 +222,6 @@ class CredentialsOptions(optparse.OptionGroup): help="DEPRECATED: Migrate to --use-kerberos", callback=self._set_kerberos_legacy) self.creds = Credentials() - def _ensure_secure_proctitle(self, opt_str, secret_data, data_type="password"): - """ Make sure no sensitive data (e.g. password) resides in proctitle. """ - import re - try: - import setproctitle - except ModuleNotFoundError: - msg = ("WARNING: Using %s on command line is insecure. " - "Please install the setproctitle python module.\n" - % data_type) - sys.stderr.write(msg) - sys.stderr.flush() - return False - # Regex to search and replace secret data + option with. - # .*[ ]+ -> Before the option must be one or more spaces. - # [= ] -> The option and the secret data might be separated by space - # or equal sign. - # [ ]*.* -> After the secret data might be one, many or no space. - pass_opt_re_str = "(.*[ ]+)(%s[= ]%s)([ ]*.*)" % (opt_str, secret_data) - pass_opt_re = re.compile(pass_opt_re_str) - # Get current proctitle. - cur_proctitle = setproctitle.getproctitle() - # Make sure we build the correct regex. - if not pass_opt_re.match(cur_proctitle): - msg = ("Unable to hide %s in proctitle. This is most likely " - "a bug!\n" % data_type) - sys.stderr.write(msg) - sys.stderr.flush() - return False - # String to replace secret data with. - secret_data_replacer = "xxx" - # Build string to replace secret data and option with. And as we dont - # want to change anything else than the secret data within the proctitle - # we have to check if the option was passed with space or equal sign as - # separator. - opt_pass_with_eq = "%s=%s" % (opt_str, secret_data) - opt_pass_part = re.sub(pass_opt_re_str, r'\2', cur_proctitle) - if opt_pass_part == opt_pass_with_eq: - replace_str = "%s=%s" % (opt_str, secret_data_replacer) - else: - replace_str = "%s %s" % (opt_str, secret_data_replacer) - # Build new proctitle: - new_proctitle = re.sub(pass_opt_re_str, - r'\1' + replace_str + r'\3', - cur_proctitle) - # Set new proctitle. - setproctitle.setproctitle(new_proctitle) - def _add_option(self, *args1, **kwargs): if self.special_name is None: return self.add_option(*args1, **kwargs) @@ -269,7 +241,6 @@ class CredentialsOptions(optparse.OptionGroup): self.creds.set_domain(arg) def _set_password(self, option, opt_str, arg, parser): - self._ensure_secure_proctitle(opt_str, arg, "password") self.creds.set_password(arg) self.ask_for_password = False self.machine_pass = False diff --git a/python/samba/tests/cred_opt.py b/python/samba/tests/cred_opt.py index 82c6434d9c8..02019b5022a 100644 --- a/python/samba/tests/cred_opt.py +++ b/python/samba/tests/cred_opt.py @@ -22,13 +22,13 @@ import optparse import os from contextlib import contextmanager -from samba.getopt import CredentialsOptions +from samba.getopt import CredentialsOptions, SambaOptions import samba.tests import setproctitle import sys password_opt = '--password=super_secret_password' -clear_password_opt = '--password=xxx' +clear_password_opt = '--password ' @contextmanager def auth_fle_opt(auth_file_path, long_opt=True): @@ -48,11 +48,17 @@ class CredentialsOptionsTests(samba.tests.TestCase): def setUp(self): super(samba.tests.TestCase, self).setUp() self.old_proctitle = setproctitle.getproctitle() - setproctitle.setproctitle('%s %s' % (self.old_proctitle, password_opt)) - sys.argv.append(password_opt) + + # We must append two options to get the " " we look for in the + # test after the redacted password + sys.argv.extend([password_opt, "--realm=samba.org"]) def test_clear_proctitle_password(self): parser = optparse.OptionParser() + + # The password burning is on the SambaOptions __init__() + sambaopts = SambaOptions(parser) + parser.add_option_group(sambaopts) credopts = CredentialsOptions(parser) parser.add_option_group(credopts) (opts, args) = parser.parse_args() -- 2.25.1 From 45fd3f72bcdf3ad65d2a370569aeb321b65e8e23 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 14:35:20 +1200 Subject: [PATCH 6/7] lib/cmdline: Also burn the --password2 parameter if given BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 414b3803bb6a1b12c44b52ab1ff64a8b7f61fd03) --- lib/cmdline/cmdline.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index 2f11804fe5e..d11ca46d389 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -149,6 +149,10 @@ bool samba_cmdline_burn(int argc, char *argv[]) return false; } + /* + * Take care that this list must be in longest-match + * first order + */ if (strncmp(p, "-U", 2) == 0) { ulen = 2; found = true; @@ -157,6 +161,9 @@ bool samba_cmdline_burn(int argc, char *argv[]) ulen = 6; found = true; is_user = true; + } else if (strncmp(p, "--password2", 11) == 0) { + ulen = 11; + found = true; } else if (strncmp(p, "--password", 10) == 0) { ulen = 10; found = true; -- 2.25.1 From 8814187cfe5f2004aed9541c5886193715ebc948 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 21 Jul 2023 15:39:28 +1200 Subject: [PATCH 7/7] lib/cmdline: Also redact --newpassword in samba_cmdline_burn() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15289 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Fri Jul 21 06:16:30 UTC 2023 on atb-devel-224 (cherry picked from commit 76ad44f446c42832e87b2c60a4731a8de3a0018f) RN: post-exec password redaction for samba-tool is more reliable for fully random passwords as it no longer uses regular expressions containing the password value itself. --- lib/cmdline/cmdline.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c index d11ca46d389..aade4ca365e 100644 --- a/lib/cmdline/cmdline.c +++ b/lib/cmdline/cmdline.c @@ -167,6 +167,9 @@ bool samba_cmdline_burn(int argc, char *argv[]) } else if (strncmp(p, "--password", 10) == 0) { ulen = 10; found = true; + } else if (strncmp(p, "--newpassword", 13) == 0) { + ulen = 13; + found = true; } if (found) { -- 2.25.1