From 522c025f9090b60d6a88319deb222126721c7c18 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 25 Jan 2021 11:46:30 +0100 Subject: [PATCH 1/4] vfs_error_inject: add unlinkat hook Note that a failure is only injected if the owner of the parent directory is not the same as the current user. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14617 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit c44dad3ac2eb36fc5eb5a9f80a9ef97183be26ef) --- source3/modules/vfs_error_inject.c | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/source3/modules/vfs_error_inject.c b/source3/modules/vfs_error_inject.c index 04880ffd5ab..6cbcf8314ce 100644 --- a/source3/modules/vfs_error_inject.c +++ b/source3/modules/vfs_error_inject.c @@ -30,6 +30,7 @@ struct unix_error_map { { "ESTALE", ESTALE }, { "EBADF", EBADF }, { "EINTR", EINTR }, + { "EACCES", EACCES }, }; static int find_unix_error_from_string(const char *err_str) @@ -122,10 +123,53 @@ static int vfs_error_inject_openat(struct vfs_handle_struct *handle, return SMB_VFS_NEXT_OPENAT(handle, dirfsp, smb_fname, fsp, flags, mode); } +static int vfs_error_inject_unlinkat(struct vfs_handle_struct *handle, + struct files_struct *dirfsp, + const struct smb_filename *smb_fname, + int flags) +{ + struct smb_filename *full_fname = NULL; + struct smb_filename *parent_fname = NULL; + int error = inject_unix_error("unlinkat", handle); + int ret; + bool ok; + + if (error == 0) { + return SMB_VFS_NEXT_UNLINKAT(handle, dirfsp, smb_fname, flags); + } + + full_fname = full_path_from_dirfsp_atname(talloc_tos(), + dirfsp, + smb_fname); + if (full_fname == NULL) { + return -1; + } + + ok = parent_smb_fname(full_fname, full_fname, &parent_fname, NULL); + if (!ok) { + TALLOC_FREE(full_fname); + return -1; + } + + ret = SMB_VFS_STAT(handle->conn, parent_fname); + if (ret != 0) { + TALLOC_FREE(full_fname); + return -1; + } + + if (parent_fname->st.st_ex_uid == get_current_uid(dirfsp->conn)) { + return SMB_VFS_NEXT_UNLINKAT(handle, dirfsp, smb_fname, flags); + } + + errno = error; + return -1; +} + static struct vfs_fn_pointers vfs_error_inject_fns = { .chdir_fn = vfs_error_inject_chdir, .pwrite_fn = vfs_error_inject_pwrite, .openat_fn = vfs_error_inject_openat, + .unlinkat_fn = vfs_error_inject_unlinkat, }; static_decl_vfs; -- 2.27.0 From e14ac295de6ec2628e29bf7eaf81a6f55b55352a Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 25 Jan 2021 11:47:45 +0100 Subject: [PATCH 2/4] selftest: add force_user_error_inject share in maptoguest env BUG: https://bugzilla.samba.org/show_bug.cgi?id=14617 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit f3f8fdfbf10f690bc8d972a13d6f74f1fb0fb375) --- selftest/target/Samba3.pm | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index bb32c93736d..c15057fa80b 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -1731,12 +1731,22 @@ $ret->{USERNAME} = KTEST\\Administrator sub setup_maptoguest { my ($self, $path) = @_; + my $prefix_abs = abs_path($path); + my $libdir="$prefix_abs/lib"; + my $share_dir="$prefix_abs/share"; + my $errorinjectconf="$libdir/error_inject.conf"; print "PROVISIONING maptoguest..."; my $options = " map to guest = bad user ntlm auth = yes + +[force_user_error_inject] + path = $share_dir + vfs objects = acl_xattr fake_acls xattr_tdb error_inject + force user = user1 + include = $errorinjectconf "; my $vars = $self->provision( -- 2.27.0 From c4da687c8cea3ffeb88b9de05a97e3a3dfe48fff Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 25 Jan 2021 11:48:32 +0100 Subject: [PATCH 3/4] selftest: add a test that verifies unlink works when "force user" is set BUG: https://bugzilla.samba.org/show_bug.cgi?id=14617 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit aa1f09cda0a097617e34dd0a8b1b0acc7a37bca8) --- .../samba3.blackbox.force-user-unlink | 1 + .../script/tests/test_force_user_unlink.sh | 40 +++++++++++++++++++ source3/selftest/tests.py | 5 +++ 3 files changed, 46 insertions(+) create mode 100644 selftest/knownfail.d/samba3.blackbox.force-user-unlink create mode 100755 source3/script/tests/test_force_user_unlink.sh diff --git a/selftest/knownfail.d/samba3.blackbox.force-user-unlink b/selftest/knownfail.d/samba3.blackbox.force-user-unlink new file mode 100644 index 00000000000..6761bd8cb61 --- /dev/null +++ b/selftest/knownfail.d/samba3.blackbox.force-user-unlink @@ -0,0 +1 @@ +^samba3.blackbox.force-user-unlink.test_forced_user_can_delete\(maptoguest:local\) diff --git a/source3/script/tests/test_force_user_unlink.sh b/source3/script/tests/test_force_user_unlink.sh new file mode 100755 index 00000000000..86076535497 --- /dev/null +++ b/source3/script/tests/test_force_user_unlink.sh @@ -0,0 +1,40 @@ +#!/bin/sh +# +# Test unlink on share with "force user" +# +# Copyright (C) 2021 Ralph Boehme + +incdir=$(dirname $0)/../../../testprogs/blackbox +. $incdir/subunit.sh +. $incdir/common_test_fns.inc + +smbclient="$BINDIR/smbclient" +error_inject_conf=$(dirname ${SMB_CONF_PATH})/error_inject.conf +failed=0 + +test_forced_user_can_delete() { + out=$($smbclient -U $DOMAIN/$USERNAME%$PASSWORD //$SERVER_IP/force_user_error_inject -c "rm dir/file") + if [ $? -ne 0 ] ; then + echo $out + return 1 + fi + tmp=$(echo $out | grep NT_STATUS_ ) + if [ $? -eq 0 ] ; then + return 1 + fi + return 0 +} + +echo "error_inject:unlinkat = EACCES" > ${error_inject_conf} + +$smbclient -U $DOMAIN/$USERNAME%$PASSWORD //$SERVER_IP/force_user_error_inject -c "mkdir dir" || failed=`expr $failed + 1` +$smbclient -U $DOMAIN/$USERNAME%$PASSWORD //$SERVER_IP/force_user_error_inject -c "put WHATSNEW.txt dir/file" || failed=`expr $failed + 1` + +testit "test_forced_user_can_delete" test_forced_user_can_delete || failed=`expr $failed + 1` + +rm ${error_inject_conf} + +# Clean up after ourselves. +$smbclient -U $DOMAIN/$USERNAME%$PASSWORD //$SERVER_IP/force_user_error_inject -c "del dir/file; rmdir dir" + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index edd86e2a1fc..bbd249c3ccb 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1125,6 +1125,11 @@ plantestsuite( "", "-b $PREFIX/clusteredmember_smb1/unclists/tmp.txt -N 5 -o 10"]) +plantestsuite("samba3.blackbox.force-user-unlink", + "maptoguest:local", + [os.path.join(samba3srcdir, + "script/tests/test_force_user_unlink.sh")]) + def planclusteredmembertestsuite(tname, prefix): '''Define a clustered test for the clusteredmember environment''' -- 2.27.0 From 00c5091c19629a573822ffcc9e19a2f0c78fd6df Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 23 Jan 2021 18:36:23 +0100 Subject: [PATCH 4/4] smbd: use fsp->conn->session_info for the initial delete-on-close token There's a correctly set up session_info at fsp->conn->session_info, we can just use that. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14617 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Tue Jan 26 04:04:14 UTC 2021 on sn-devel-184 (cherry picked from commit e06f86bbd93d024c70016e1adcf833db85742aca) --- .../samba3.blackbox.force-user-unlink | 1 - source3/smbd/close.c | 25 +++---------------- 2 files changed, 4 insertions(+), 22 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.blackbox.force-user-unlink diff --git a/selftest/knownfail.d/samba3.blackbox.force-user-unlink b/selftest/knownfail.d/samba3.blackbox.force-user-unlink deleted file mode 100644 index 6761bd8cb61..00000000000 --- a/selftest/knownfail.d/samba3.blackbox.force-user-unlink +++ /dev/null @@ -1 +0,0 @@ -^samba3.blackbox.force-user-unlink.test_forced_user_can_delete\(maptoguest:local\) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 9974877edc2..43762555b35 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -341,21 +341,13 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp, if (fsp->fsp_flags.initial_delete_on_close && !is_delete_on_close_set(lck, fsp->name_hash)) { - struct auth_session_info *session_info = NULL; - /* Initial delete on close was set and no one else * wrote a real delete on close. */ - status = smbXsrv_session_info_lookup(conn->sconn->client, - fsp->vuid, - &session_info); - if (!NT_STATUS_IS_OK(status)) { - return NT_STATUS_INTERNAL_ERROR; - } fsp->fsp_flags.delete_on_close = true; set_delete_on_close_lck(fsp, lck, - session_info->security_token, - session_info->unix_token); + fsp->conn->session_info->security_token, + fsp->conn->session_info->unix_token); } delete_file = is_delete_on_close_set(lck, fsp->name_hash) && @@ -1176,24 +1168,15 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, } if (fsp->fsp_flags.initial_delete_on_close) { - struct auth_session_info *session_info = NULL; - /* Initial delete on close was set - for * directories we don't care if anyone else * wrote a real delete on close. */ - status = smbXsrv_session_info_lookup(fsp->conn->sconn->client, - fsp->vuid, - &session_info); - if (!NT_STATUS_IS_OK(status)) { - return NT_STATUS_INTERNAL_ERROR; - } - send_stat_cache_delete_message(fsp->conn->sconn->msg_ctx, fsp->fsp_name->base_name); set_delete_on_close_lck(fsp, lck, - session_info->security_token, - session_info->unix_token); + fsp->conn->session_info->security_token, + fsp->conn->session_info->unix_token); fsp->fsp_flags.delete_on_close = true; } -- 2.27.0