change in behaviour regarding "open for execute" from 3.6 to 4.0

Michael Adam obnox at samba.org
Mon Sep 2 09:56:07 MDT 2013


Hi,

in Samba 3.6, "open for execution" was successful even if the
user had no execute permissions.

In Samba 4.0 this was fixed by doing a proper ACL-check against
the provided access_mask.

While this is correct, it is a potential problem for those migrating
their fileserver from Samba 3.6 (or older) to Samba 4.0 (or newer),
since they need to audit their files for missing x-bits in
acls/permissions...

Because quite a number of people have already stumbled across
this, the attached patchset introduces a new smb.conf-parameter

  "acl execute compatibility mode"

Which (when set to True) re-establishes the old behaviour.
It is meant as a workaround for a transition period until
the file permissions have been fixed..

Comment / review / push appreciated..

Cheers - Michael

PS: For 4.0. there is one difference in context, so for
convenience, also attached a patchset that works against
v4-0-test.
-------------- next part --------------
From 104a8513a5bd46ff603c58409b7f7c4e03b03852 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 17:36:59 +0200
Subject: [PATCH 1/3] loadparm: add new parameter "acl execute compatibility
 mode"

Signed-off-by: Michael Adam <obnox at samba.org>
---
 lib/param/param_functions.c |    1 +
 lib/param/param_table.c     |   10 ++++++++++
 source3/include/proto.h     |    1 +
 source3/param/loadparm.c    |    1 +
 4 files changed, 13 insertions(+)

diff --git a/lib/param/param_functions.c b/lib/param/param_functions.c
index fed2e95..511a8bc 100644
--- a/lib/param/param_functions.c
+++ b/lib/param/param_functions.c
@@ -132,6 +132,7 @@ FN_LOCAL_BOOL(afs_share, bAfs_Share)
 FN_LOCAL_BOOL(acl_check_permissions, bAclCheckPermissions)
 FN_LOCAL_BOOL(acl_group_control, bAclGroupControl)
 FN_LOCAL_BOOL(acl_map_full_control, bAclMapFullControl)
+FN_LOCAL_BOOL(acl_execute_compatibility_mode, bAclExecuteCompatibilityMode)
 FN_LOCAL_INTEGER(defaultcase, iDefaultCase)
 FN_LOCAL_INTEGER(minprintspace, iMinPrintSpace)
 FN_LOCAL_INTEGER(printing, iPrinting)
diff --git a/lib/param/param_table.c b/lib/param/param_table.c
index 1b1497c..b996763 100644
--- a/lib/param/param_table.c
+++ b/lib/param/param_table.c
@@ -904,6 +904,16 @@ static struct parm_struct parm_table[] = {
 		.flags		= FLAG_ADVANCED | FLAG_GLOBAL | FLAG_SHARE,
 	},
 	{
+		.label		= "acl execute compatibility mode",
+		.type		= P_BOOL,
+		.p_class	= P_LOCAL,
+		.offset		= LOCAL_VAR(bAclExecuteCompatibilityMode),
+		.special	= NULL,
+		.enum_list	= NULL,
+		.flags		= FLAG_ADVANCED | FLAG_GLOBAL | FLAG_SHARE,
+	},
+
+	{
 		.label		= "create mask",
 		.type		= P_OCTAL,
 		.p_class	= P_LOCAL,
diff --git a/source3/include/proto.h b/source3/include/proto.h
index 078d039..448d098 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -1328,6 +1328,7 @@ bool lp_afs_share(int );
 bool lp_acl_check_permissions(int );
 bool lp_acl_group_control(int );
 bool lp_acl_map_full_control(int );
+bool lp_acl_execute_compatibility_mode(int);
 bool lp_durable_handles(int);
 int lp_create_mask(int );
 int lp_force_create_mode(int );
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 229ebd8..b47bd1c 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -264,6 +264,7 @@ static struct loadparm_service sDefault =
 	.bAclCheckPermissions = true,
 	.bAclMapFullControl = true,
 	.bAclGroupControl = false,
+	.bAclExecuteCompatibilityMode = false,
 	.bChangeNotify = true,
 	.bKernelChangeNotify = true,
 	.iallocation_roundup_size = SMB_ROUNDUP_ALLOCATION_SIZE,
-- 
1.7.9.5


From 3e88d073b638c27ba8158704c1f154fb3ede7c0b Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 17:37:50 +0200
Subject: [PATCH 2/3] s3:smbd: ease file server upgrades from 3.6 and earlier
 with "acl execute compatibility mode"

3.6 and earlier allowed open for execution when execute permissions are
not present on a file. This has been fixed in Samba 4.0.

This patch changes smbd to skip the execute bit from the ACL check
in the open code if "acl execute compatibility mode = yes", hence
re-establishing the old behaviour in this case.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/smbd/open.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 53f8b8e..e31f96d 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -76,6 +76,7 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
 	struct security_descriptor *sd = NULL;
 	uint32_t rejected_share_access;
 	uint32_t rejected_mask = access_mask;
+	uint32_t do_not_check_mask = 0;
 
 	rejected_share_access = access_mask & ~(conn->share_access);
 
@@ -143,10 +144,23 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
 	 * se_file_access_check() also takes care of
 	 * owner WRITE_DAC and READ_CONTROL.
 	 */
+	do_not_check_mask = FILE_READ_ATTRIBUTES;
+
+	/*
+	 * Samba 3.6 and earlier granted execute access even
+	 * if the ACL did not contain execute rights.
+	 * Samba 4.0 is more correct and checks it.
+	 * The compatibilty mode allows to skip this check
+	 * to smoothen upgrades.
+	 */
+	if (lp_acl_execute_compatibility_mode(SNUM(conn))) {
+		do_not_check_mask |= FILE_EXECUTE;
+	}
+
 	status = se_file_access_check(sd,
 				get_current_nttok(conn),
 				use_privs,
-				(access_mask & ~FILE_READ_ATTRIBUTES),
+				(access_mask & ~do_not_check_mask),
 				&rejected_mask);
 
 	DEBUG(10,("smbd_check_access_rights: file %s requesting "
-- 
1.7.9.5


From bbf0a68ad8063847f938375ab4c180b0e9ada67f Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 16:54:15 +0200
Subject: [PATCH 3/3] docs: document "acl execute compatibility mode"

Signed-off-by: Michael Adam <obnox at samba.org>
---
 .../protocol/aclexecutecompatibilitymode.xml       |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml

diff --git a/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml b/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml
new file mode 100644
index 0000000..53b1747
--- /dev/null
+++ b/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml
@@ -0,0 +1,26 @@
+<samba:parameter name="acl execute compatibility mode"
+                 context="S"
+                 type="boolean"
+                 advanced="1" wizard="1"
+                 xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
+<description>
+    <para>
+    This boolean parameter controls the behaviour of <citerefentry><refentrytitle>smbd</refentrytitle>
+    <manvolnum>8</manvolnum></citerefentry> when receiving a protocol request of "open for execution"
+    from a Windows client.
+    With Samba 3.6 and older, the execution right in the ACL was not checked, so a client
+    could execute a file even if it did not have execute rights on the file. In Samba 4.0,
+    this has been fixed, so that by default, i.e. when this parameter is set to "False",
+    open for execution is now denied when execution permissions are not present.
+    </para>
+    <para>
+    If this parameter is set to "True", Samba does not check execute permissions on
+    "open for execution, thus re-establishing the behaviour of Samba 3.6.
+    This can be useful to smoothen upgrades from older Samba versions to 4.0 and newer.
+    This setting is not not meant to be used as a permanent setting, but as a temporary relief:
+    It is recommended to fix the permissions in the ACLs and reset this parameter to the
+    default after a ceratain transition period.
+    </para>
+</description>
+<value type="default">False</value>
+</samba:parameter>
-- 
1.7.9.5

-------------- next part --------------
From 3a347178b68d7afde2409f195b27e28532684448 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 17:36:59 +0200
Subject: [PATCH 1/3] loadparm: add new parameter "acl execute compatibility
 mode"

Signed-off-by: Michael Adam <obnox at samba.org>
---
 lib/param/param_functions.c |    1 +
 lib/param/param_table.c     |   10 ++++++++++
 source3/include/proto.h     |    1 +
 source3/param/loadparm.c    |    1 +
 4 files changed, 13 insertions(+)

diff --git a/lib/param/param_functions.c b/lib/param/param_functions.c
index 94652fa..9e61003 100644
--- a/lib/param/param_functions.c
+++ b/lib/param/param_functions.c
@@ -134,6 +134,7 @@ FN_LOCAL_BOOL(afs_share, bAfs_Share)
 FN_LOCAL_BOOL(acl_check_permissions, bAclCheckPermissions)
 FN_LOCAL_BOOL(acl_group_control, bAclGroupControl)
 FN_LOCAL_BOOL(acl_map_full_control, bAclMapFullControl)
+FN_LOCAL_BOOL(acl_execute_compatibility_mode, bAclExecuteCompatibilityMode)
 FN_LOCAL_INTEGER(defaultcase, iDefaultCase)
 FN_LOCAL_INTEGER(minprintspace, iMinPrintSpace)
 FN_LOCAL_INTEGER(printing, iPrinting)
diff --git a/lib/param/param_table.c b/lib/param/param_table.c
index a73cd96..090695b 100644
--- a/lib/param/param_table.c
+++ b/lib/param/param_table.c
@@ -920,6 +920,16 @@ static struct parm_struct parm_table[] = {
 		.flags		= FLAG_ADVANCED | FLAG_GLOBAL | FLAG_SHARE,
 	},
 	{
+		.label		= "acl execute compatibility mode",
+		.type		= P_BOOL,
+		.p_class	= P_LOCAL,
+		.offset		= LOCAL_VAR(bAclExecuteCompatibilityMode),
+		.special	= NULL,
+		.enum_list	= NULL,
+		.flags		= FLAG_ADVANCED | FLAG_GLOBAL | FLAG_SHARE,
+	},
+
+	{
 		.label		= "create mask",
 		.type		= P_OCTAL,
 		.p_class	= P_LOCAL,
diff --git a/source3/include/proto.h b/source3/include/proto.h
index 5f34193..69d350f 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -1326,6 +1326,7 @@ bool lp_afs_share(int );
 bool lp_acl_check_permissions(int );
 bool lp_acl_group_control(int );
 bool lp_acl_map_full_control(int );
+bool lp_acl_execute_compatibility_mode(int);
 bool lp_durable_handles(int);
 int lp_create_mask(int );
 int lp_force_create_mode(int );
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index e09c2bf..6496fd8 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -264,6 +264,7 @@ static struct loadparm_service sDefault =
 	.bAclCheckPermissions = true,
 	.bAclMapFullControl = true,
 	.bAclGroupControl = false,
+	.bAclExecuteCompatibilityMode = false,
 	.bChangeNotify = true,
 	.bKernelChangeNotify = true,
 	.iallocation_roundup_size = SMB_ROUNDUP_ALLOCATION_SIZE,
-- 
1.7.9.5


From ef25b0496c40543230d63cc3f0fc7428d5d7049e Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 17:37:50 +0200
Subject: [PATCH 2/3] s3:smbd: ease file server upgrades from 3.6 and earlier
 with "acl execute compatibility mode"

3.6 and earlier allowed open for execution when execute permissions are
not present on a file. This has been fixed in Samba 4.0.

This patch changes smbd to skip the execute bit from the ACL check
in the open code if "acl execute compatibility mode = yes", hence
re-establishing the old behaviour in this case.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/smbd/open.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 16ca34a..ea6b1be 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -74,6 +74,7 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
 	struct security_descriptor *sd = NULL;
 	uint32_t rejected_share_access;
 	uint32_t rejected_mask = access_mask;
+	uint32_t do_not_check_mask = 0;
 
 	rejected_share_access = access_mask & ~(conn->share_access);
 
@@ -141,10 +142,23 @@ NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
 	 * se_file_access_check() also takes care of
 	 * owner WRITE_DAC and READ_CONTROL.
 	 */
+	do_not_check_mask = FILE_READ_ATTRIBUTES;
+
+	/*
+	 * Samba 3.6 and earlier granted execute access even
+	 * if the ACL did not contain execute rights.
+	 * Samba 4.0 is more correct and checks it.
+	 * The compatibilty mode allows to skip this check
+	 * to smoothen upgrades.
+	 */
+	if (lp_acl_execute_compatibility_mode(SNUM(conn))) {
+		do_not_check_mask |= FILE_EXECUTE;
+	}
+
 	status = se_file_access_check(sd,
 				get_current_nttok(conn),
 				false,
-				(access_mask & ~FILE_READ_ATTRIBUTES),
+				(access_mask & ~do_not_check_mask),
 				&rejected_mask);
 
 	DEBUG(10,("smbd_check_access_rights: file %s requesting "
-- 
1.7.9.5


From b85ace219a0e2b6d77c4f060f246fabf94d963e3 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 2 Sep 2013 16:54:15 +0200
Subject: [PATCH 3/3] docs: document "acl execute compatibility mode"

Signed-off-by: Michael Adam <obnox at samba.org>
---
 .../protocol/aclexecutecompatibilitymode.xml       |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml

diff --git a/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml b/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml
new file mode 100644
index 0000000..53b1747
--- /dev/null
+++ b/docs-xml/smbdotconf/protocol/aclexecutecompatibilitymode.xml
@@ -0,0 +1,26 @@
+<samba:parameter name="acl execute compatibility mode"
+                 context="S"
+                 type="boolean"
+                 advanced="1" wizard="1"
+                 xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
+<description>
+    <para>
+    This boolean parameter controls the behaviour of <citerefentry><refentrytitle>smbd</refentrytitle>
+    <manvolnum>8</manvolnum></citerefentry> when receiving a protocol request of "open for execution"
+    from a Windows client.
+    With Samba 3.6 and older, the execution right in the ACL was not checked, so a client
+    could execute a file even if it did not have execute rights on the file. In Samba 4.0,
+    this has been fixed, so that by default, i.e. when this parameter is set to "False",
+    open for execution is now denied when execution permissions are not present.
+    </para>
+    <para>
+    If this parameter is set to "True", Samba does not check execute permissions on
+    "open for execution, thus re-establishing the behaviour of Samba 3.6.
+    This can be useful to smoothen upgrades from older Samba versions to 4.0 and newer.
+    This setting is not not meant to be used as a permanent setting, but as a temporary relief:
+    It is recommended to fix the permissions in the ACLs and reset this parameter to the
+    default after a ceratain transition period.
+    </para>
+</description>
+<value type="default">False</value>
+</samba:parameter>
-- 
1.7.9.5

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130902/c91aba1f/attachment.pgp>


More information about the samba-technical mailing list