From ce475c1395b1917f3f96702ddda25b151ee76b25 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 24 Jan 2019 10:15:56 -0800 Subject: [PATCH 1/2] 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 Reviewed-by: Ralph Boehme (cherry picked from commit 7b21b4c1f538650f23ec77fb3c02fe1e224d89aa) --- selftest/knownfail | 2 + selftest/selftesthelpers.py | 1 + selftest/target/Samba3.pm | 6 ++ .../script/tests/test_force_group_change.sh | 73 +++++++++++++++++++ source3/selftest/tests.py | 3 + 5 files changed, 85 insertions(+) create mode 100755 source3/script/tests/test_force_group_change.sh diff --git a/selftest/knownfail b/selftest/knownfail index baf3d57a31a..ca2f79d768c 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -349,3 +349,5 @@ # Disabling NTLM means you can't use samr to change the password ^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\) +# BUG:https://bugzilla.samba.org/show_bug.cgi?id=13690 +^samba3.blackbox.force_group_change.* diff --git a/selftest/selftesthelpers.py b/selftest/selftesthelpers.py index 0d8014c7d13..6e73f9f2673 100644 --- a/selftest/selftesthelpers.py +++ b/selftest/selftesthelpers.py @@ -197,3 +197,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 314aae55bc5..47b9c8cbc48 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -904,6 +904,12 @@ sub setup_fileserver force group = everyone write list = force_user +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=13690 +[force_group_test] + path = $share_dir + comment = force group test +# force group = everyone + [smbget] path = $smbget_sharedir comment = smb username is [%U] 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 +# 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 < Date: Fri, 18 Jan 2019 14:24:30 -0800 Subject: [PATCH 2/2] smbd: uid: Don't crash if 'force group' is added to an existing share connection. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ralph Boehme Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Fri Jan 25 16:31:27 CET 2019 on sn-devel-144 (cherry picked from commit e37f9956c1f2416408bad048a4618f6366086b6a) --- selftest/knownfail | 2 -- source3/smbd/uid.c | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/selftest/knownfail b/selftest/knownfail index ca2f79d768c..baf3d57a31a 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -349,5 +349,3 @@ # Disabling NTLM means you can't use samr to change the password ^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\) -# BUG:https://bugzilla.samba.org/show_bug.cgi?id=13690 -^samba3.blackbox.force_group_change.* diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c index 9d5321cf4cc..ced2d450f8e 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -296,6 +296,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; @@ -335,9 +336,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; -- 2.20.1.495.gaa96b0ce6b-goog