From 58212b72715510c87b906ce906f0cf8ccb5044e8 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 | 5 ++ .../script/tests/test_force_group_change.sh | 73 +++++++++++++++++++ source3/selftest/tests.py | 4 + 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 abbbd889c71..9678883924e 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -363,3 +363,5 @@ ^samba.tests.ntlmdisabled.python\(ktest\).python2.ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python3.ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python2.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 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/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 9678883924e..abbbd889c71 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -363,5 +363,3 @@ ^samba.tests.ntlmdisabled.python\(ktest\).python2.ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python3.ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\) ^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).python2.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 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; -- 2.20.1.495.gaa96b0ce6b-goog