[RFC] Creation of files in directories with delete-on-close set

Ralph Böhme slow at samba.org
Mon Jan 15 15:46:57 UTC 2018


Hi folks,

I just came across the fact that smbd does not enforce set delete-on-close on
parent directory on file (or directory) creation.

Windows server do this check and the s4 ntvfs server does as well.

Adding such a check is trivial (cf attached patch), but this oviously has
performance implications as it requires an additional record fetch from
locking.tdb per create.

For protocol completeness, we could add the check but guard it by a default off
option.

Thoughts?

-slow

-- 
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
-------------- next part --------------
From c3e12b1846e4fdea27e3a0419f2ea3b94c8bdc2a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 12 Jan 2018 17:32:44 +0100
Subject: [PATCH 1/2] selftest: run deltest20 against s3 as well

This marks the test as knownfail, the next commit fixes it.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 selftest/knownfail.d/samba3.base.delete | 2 ++
 source4/torture/basic/delete.c          | 4 ----
 2 files changed, 2 insertions(+), 4 deletions(-)
 create mode 100644 selftest/knownfail.d/samba3.base.delete

diff --git a/selftest/knownfail.d/samba3.base.delete b/selftest/knownfail.d/samba3.base.delete
new file mode 100644
index 00000000000..6129ca7a66c
--- /dev/null
+++ b/selftest/knownfail.d/samba3.base.delete
@@ -0,0 +1,2 @@
+samba3.base.delete.deltest20\(nt4_dc\)
+samba3.base.delete.deltest20\(ad_dc\)
diff --git a/source4/torture/basic/delete.c b/source4/torture/basic/delete.c
index d74063c310f..54815b95512 100644
--- a/source4/torture/basic/delete.c
+++ b/source4/torture/basic/delete.c
@@ -1766,10 +1766,6 @@ static bool deltest20(struct torture_context *tctx, struct smbcli_state *cli1, s
 
 	/* Test 20 -- non-empty directory hardest to get right... */
 
-	if (torture_setting_bool(tctx, "samba3", false)) {
-		return true;
-	}
-
 	smbcli_deltree(cli1->tree, dname);
 
 	dnum1 = smbcli_nt_create_full(cli1->tree, dname, 0,
-- 
2.13.6


From f40028b6de0cb3c35906b4b2971ad10113b492cf Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 11 Jan 2018 17:52:06 +0100
Subject: [PATCH 2/2] s3/smbd: fix handling of delete-on-close on directories

This implements a check to test the delete-on-close flag of a directory
for requests to create files in this directory.

Windows server implement this check, Samba doesn't as it has performance
implications.

This commit implements the check and a new option to control it. By
default the check is skipped, setting "check parent directory delete on
close = yes" enables it.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 .../tuning/checkparentdirectorydeleteonclose.xml   | 13 +++++++
 lib/param/loadparm.c                               |  2 +
 selftest/knownfail.d/samba3.base.delete            |  2 -
 selftest/target/Samba3.pm                          |  1 +
 selftest/target/Samba4.pm                          |  1 +
 source3/param/loadparm.c                           |  1 +
 source3/smbd/open.c                                | 45 +++++++++++++++++++++-
 7 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
 delete mode 100644 selftest/knownfail.d/samba3.base.delete

diff --git a/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml b/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
new file mode 100644
index 00000000000..1de06091dd7
--- /dev/null
+++ b/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
@@ -0,0 +1,13 @@
+<samba:parameter name="check parent directory delete on close"
+                 context="S"
+		 type="boolean"
+                 xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
+<description>
+    <para>A Windows SMB server prevents the client from creating files in a
+    directory that has the delete-on-close flag set. By default Samba doesn't
+    perform this check as this check is a quite expensive operation in Samba.
+    </para>
+</description>
+
+<value type="default">no</value>
+</samba:parameter>
diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
index 7854f57a158..efad4a10f03 100644
--- a/lib/param/loadparm.c
+++ b/lib/param/loadparm.c
@@ -2998,6 +2998,8 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)
 
 	lpcfg_do_global_parameter(lp_ctx, "prefork children", "1");
 
+	lpcfg_do_global_parameter(lp_ctx, "check parent directory delete on close", "no");
+
 	for (i = 0; parm_table[i].label; i++) {
 		if (!(lp_ctx->flags[i] & FLAG_CMDLINE)) {
 			lp_ctx->flags[i] |= FLAG_DEFAULT;
diff --git a/selftest/knownfail.d/samba3.base.delete b/selftest/knownfail.d/samba3.base.delete
deleted file mode 100644
index 6129ca7a66c..00000000000
--- a/selftest/knownfail.d/samba3.base.delete
+++ /dev/null
@@ -1,2 +0,0 @@
-samba3.base.delete.deltest20\(nt4_dc\)
-samba3.base.delete.deltest20\(ad_dc\)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index f4ae0f3c02f..43dbfe88856 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -231,6 +231,7 @@ sub setup_nt4_dc($$)
 	rpc_daemon:lsasd = fork
 	rpc_daemon:fssd = fork
 	fss: sequence timeout = 1
+	check parent directory delete on close = yes
 ";
 
 	my $vars = $self->provision($path, "SAMBA-TEST",
diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
index c161ee082a0..123bf6c3a91 100755
--- a/selftest/target/Samba4.pm
+++ b/selftest/target/Samba4.pm
@@ -1815,6 +1815,7 @@ sub provision_ad_dc($$$$$$)
 	smbd:writetimeupdatedelay = 500000
 	create mask = 755
 	dos filemode = yes
+	check parent directory delete on close = yes
 
         dcerpc endpoint servers = -winreg -srvsvc
 
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 096c23f4fb3..b1b502c117e 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -244,6 +244,7 @@ static const struct loadparm_service _sDefault =
 	.smb_encrypt = SMB_SIGNING_DEFAULT,
 	.kernel_share_modes = true,
 	.durable_handles = true,
+	.check_parent_directory_delete_on_close = false,
 	.param_opt = NULL,
 	.dummy = ""
 };
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index e55c3941092..5817bdbe9ae 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -253,6 +253,11 @@ NTSTATUS check_parent_access(struct connection_struct *conn,
 	struct security_descriptor *parent_sd = NULL;
 	uint32_t access_granted = 0;
 	struct smb_filename *parent_smb_fname = NULL;
+	struct share_mode_lock *lck = NULL;
+	struct file_id id = {0};
+	uint32_t name_hash;
+	bool delete_on_close_set;
+	int ret;
 
 	if (!parent_dirname(talloc_tos(),
 				smb_fname->base_name,
@@ -320,7 +325,45 @@ NTSTATUS check_parent_access(struct connection_struct *conn,
 		return status;
 	}
 
-	return NT_STATUS_OK;
+	if (!(access_mask & (SEC_DIR_ADD_FILE | SEC_DIR_ADD_SUBDIR))) {
+		return NT_STATUS_OK;
+	}
+	if (!lp_check_parent_directory_delete_on_close(SNUM(conn))) {
+		return NT_STATUS_OK;
+	}
+
+	/* Check if the directory has delete-on-close set */
+	ret = SMB_VFS_STAT(conn, parent_smb_fname);
+	if (ret != 0) {
+		status = map_nt_error_from_unix(errno);
+		goto out;
+	}
+
+	id = SMB_VFS_FILE_ID_CREATE(conn, &parent_smb_fname->st);
+
+	status = file_name_hash(conn, parent_smb_fname->base_name, &name_hash);
+	if (!NT_STATUS_IS_OK(status)) {
+		goto out;
+	}
+
+	lck = get_existing_share_mode_lock(talloc_tos(), id);
+	if (lck == NULL) {
+		status = NT_STATUS_OK;
+		goto out;
+	}
+
+	delete_on_close_set = is_delete_on_close_set(lck, name_hash);
+	if (delete_on_close_set) {
+		status = NT_STATUS_DELETE_PENDING;
+		goto out;
+	}
+
+	status = NT_STATUS_OK;
+
+out:
+	TALLOC_FREE(lck);
+	TALLOC_FREE(parent_smb_fname);
+	return status;
 }
 
 /****************************************************************************
-- 
2.13.6



More information about the samba-technical mailing list