The branch, master has been updated via e37f9956c1f smbd: uid: Don't crash if 'force group' is added to an existing share connection. via 7b21b4c1f53 s3: tests: Add regression test for smbd crash on share force group change with existing connection. via e903d37ea4a s3:rpclient: rpclient help is not very helpful from 67fc683a3ee CI: move target "build_nt4" to private gitlab runners
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit e37f9956c1f2416408bad048a4618f6366086b6a Author: Jeremy Allison <j...@samba.org> Date: Fri Jan 18 14:24:30 2019 -0800 smbd: uid: Don't crash if 'force group' is added to an existing share connection. smbd could crash if "force group" is added to a share definition whilst an existing connection to that share exists. In that case, don't change the existing credentials for force group, only do so for new connections. Remove knownfail from regression test. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> Autobuild-User(master): Ralph Böhme <s...@samba.org> Autobuild-Date(master): Fri Jan 25 16:31:27 CET 2019 on sn-devel-144 commit 7b21b4c1f538650f23ec77fb3c02fe1e224d89aa Author: Jeremy Allison <j...@samba.org> Date: Thu Jan 24 10:15:56 2019 -0800 s3: tests: Add regression test for smbd crash on share force group change with existing connection. Mark as known fail for now. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> commit e903d37ea4a8372f819f8bdbec7dfeef1799f612 Author: Tim Beale <timbe...@catalyst.net.nz> Date: Wed Jan 23 11:07:42 2019 +1300 s3:rpclient: rpclient help is not very helpful The help was not telling me that there was a mandatory 'server' argument that I needed to specify. After trying several different combinations of parameters, I eventually had to run the tool in gdb to work out why it was complaining. This is the output I was getting: bin/rpcclient -U$USERNAME%$PASSWORD -I $SERVER_IP Usage: rpcclient [OPTION...] -c, --command=COMMANDS Execute semicolon separated cmds -I, --dest-ip=IP Specify destination IP address -p, --port=PORT Specify port number ... New help output is: Usage: rpcclient [OPTION...] <server> Options: -c, --command=COMMANDS Execute semicolon separated cmds ... Signed-off-by: Tim Beale <timbe...@catalyst.net.nz> Reviewed-by: Ralph Boehme <s...@samba.org> ----------------------------------------------------------------------- Summary of changes: selftest/selftesthelpers.py | 1 + selftest/target/Samba3.pm | 5 ++ source3/rpcclient/rpcclient.c | 2 + source3/script/tests/test_force_group_change.sh | 73 +++++++++++++++++++++++++ source3/selftest/tests.py | 4 ++ source3/smbd/uid.c | 35 +++++++++++- 6 files changed, 118 insertions(+), 2 deletions(-) create mode 100755 source3/script/tests/test_force_group_change.sh Changeset truncated at 500 lines: diff --git a/selftest/selftesthelpers.py b/selftest/selftesthelpers.py index ebdae12866a..acce6d24cce 100644 --- a/selftest/selftesthelpers.py +++ b/selftest/selftesthelpers.py @@ -207,3 +207,4 @@ smbcquotas = binpath('smbcquotas') smbget = binpath('smbget') rpcclient = binpath('rpcclient') smbcacls = binpath('smbcacls') +smbcontrol = binpath('smbcontrol') diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index 49bdd2ac885..f11bb9312df 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -984,6 +984,11 @@ sub setup_fileserver comment = inherit only unix owner inherit owner = unix only acl_xattr:ignore system acls = yes +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 +[force_group_test] + path = $share_dir + comment = force group test +# force group = everyone [homes] comment = Home directories browseable = No diff --git a/source3/rpcclient/rpcclient.c b/source3/rpcclient/rpcclient.c index 9f95f1a7a8c..6cfacb19cfe 100644 --- a/source3/rpcclient/rpcclient.c +++ b/source3/rpcclient/rpcclient.c @@ -982,6 +982,8 @@ out_free: pc = poptGetContext("rpcclient", argc, const_argv, long_options, 0); + poptSetOtherOptionHelp(pc, "[OPTION...] <server>\nOptions:"); + if (argc == 1) { poptPrintHelp(pc, stderr, 0); goto done; diff --git a/source3/script/tests/test_force_group_change.sh b/source3/script/tests/test_force_group_change.sh new file mode 100755 index 00000000000..6cb1ab4e048 --- /dev/null +++ b/source3/script/tests/test_force_group_change.sh @@ -0,0 +1,73 @@ +#!/bin/sh + +# Copyright (c) Jeremy Allison <j...@samba.org> +# License: GPLv3 +# Regression test for BUG:https://bugzilla.samba.org/show_bug.cgi?id=13690 + +if [ $# -lt 6 ]; then + echo "Usage: test_force_group_change.sh SERVER USERNAME PASSWORD LOCAL_PATH SMBCLIENT SMBCONTROL" + exit 1 +fi + +SERVER="${1}" +USERNAME="${2}" +PASSWORD="${3}" +LOCAL_PATH="${4}" +SMBCLIENT="${5}" +SMBCONTROL="${6}" +shift 6 + +incdir=`dirname $0`/../../../testprogs/blackbox +. $incdir/subunit.sh + +failed=0 + +test_force_group_change() +{ +# +# A SMB_CONF variable passed in here is the client smb.conf. +# We need to convert to the server.conf file from +# the LOCAL_PATH variable. +# +SERVER_CONFIG=`dirname $LOCAL_PATH`/lib/server.conf +SERVER_CONFIG_SAVE=${SERVER_CONFIG}.bak +SERVER_CONFIG_NEW=${SERVER_CONFIG}.new +cp $SERVER_CONFIG $SERVER_CONFIG_SAVE + +sed -e 's/#\tforce group = everyone/\tforce group = everyone/' <${SERVER_CONFIG} >${SERVER_CONFIG_NEW} + + tmpfile=$PREFIX/smbclient_force_group_change_commands + cat > $tmpfile <<EOF +ls +!cp ${SERVER_CONFIG_NEW} ${SERVER_CONFIG} +!${SMBCONTROL} --configfile=${SERVER_CONFIG} all reload-config +ls +!cp ${SERVER_CONFIG_SAVE} ${SERVER_CONFIG} +!${SMBCONTROL} --configfile=${SERVER_CONFIG} all reload-config +quit +EOF + + cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/force_group_test $CONFIGURATION < $tmpfile 2>&1' + eval echo "$cmd" + out=$(eval $cmd) + ret=$? + rm -f $tmpfile + rm -f $SERVER_CONFIG_SAVE + rm -f $SERVER_CONFIG_NEW + + echo "$out" | grep 'NT_STATUS_CONNECTION_DISCONNECTED' + ret=$? + if [ $ret -eq 0 ] ; then + # Client was disconnected as server crashed. + echo "$out" + return 1 + fi + + return 0 +} + +testit "test force group change" \ + test_force_group_change || \ + failed=`expr $failed + 1` + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 46f078759e1..30a93a2ee42 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -29,6 +29,7 @@ from selftesthelpers import net, wbinfo, dbwrap_tool, rpcclient, python from selftesthelpers import smbget, smbcacls, smbcquotas, ntlm_auth3 from selftesthelpers import valgrindify, smbtorture4_testsuites from selftesthelpers import smbtorture4_options +from selftesthelpers import smbcontrol smbtorture4_options.extend([ '--option=torture:sharedelay=100000', '--option=torture:writetimeupdatedelay=500000', @@ -327,6 +328,9 @@ for env in ["fileserver"]: plantestsuite("samba3.blackbox.large_acl.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls, '-m', 'SMB3']) plantestsuite("samba3.blackbox.give_owner", env, [os.path.join(samba3srcdir, "script/tests/test_give_owner.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'tmp']) plantestsuite("samba3.blackbox.homes", env, [os.path.join(samba3srcdir, "script/tests/test_homes.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', '$PREFIX', smbclient3, configuration]) + plantestsuite("samba3.blackbox.force_group_change", env, + [os.path.join(samba3srcdir, "script/tests/test_force_group_change.sh"), + '$SERVER', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3, smbcontrol]) # # tar command tests diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c index 7aecea5f857..a4bcb747d37 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -291,6 +291,7 @@ static bool change_to_user_internal(connection_struct *conn, int snum; gid_t gid; uid_t uid; + const char *force_group_name; char group_c; int num_groups = 0; gid_t *group_list = NULL; @@ -330,9 +331,39 @@ static bool change_to_user_internal(connection_struct *conn, * See if we should force group for this service. If so this overrides * any group set in the force user code. */ - if((group_c = *lp_force_group(talloc_tos(), snum))) { + force_group_name = lp_force_group(talloc_tos(), snum); + group_c = *force_group_name; - SMB_ASSERT(conn->force_group_gid != (gid_t)-1); + if ((group_c != '\0') && (conn->force_group_gid == (gid_t)-1)) { + /* + * This can happen if "force group" is added to a + * share definition whilst an existing connection + * to that share exists. In that case, don't change + * the existing credentials for force group, only + * do so for new connections. + * + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 + */ + DBG_INFO("Not forcing group %s on existing connection to " + "share %s for SMB user %s (unix user %s)\n", + force_group_name, + lp_const_servicename(snum), + session_info->unix_info->sanitized_username, + session_info->unix_info->unix_name); + } + + if((group_c != '\0') && (conn->force_group_gid != (gid_t)-1)) { + /* + * Only force group for connections where + * conn->force_group_gid has already been set + * to the correct value (i.e. the connection + * happened after the 'force group' definition + * was added to the share definition. Connections + * that were made before force group was added + * should stay with their existing credentials. + * + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 + */ if (group_c == '+') { int i; -- Samba Shared Repository