[PATCH] for bug 11647 - fix access denied when path = "/"

Michael Adam obnox at samba.org
Wed Dec 23 19:01:16 UTC 2015


attachment...

(See the patch with git show -w)

On 2015-12-23 at 19:55 +0100, Michael Adam wrote:
> Hi,
> 
> please see attached patch for
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11647
> 
> José and I are in the process of verifying that it fixes
> all the issues we have seen.
> 
> Thanks - Michael


-------------- next part --------------
From 18be79b6e7192350d26f2abdc5f27c95563d7c9f Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Dec 2015 18:01:23 +0100
Subject: [PATCH] s3:smbd: fix a corner case of the symlink verification

Commit 7606c0db257b3f9d84da5b2bf5fbb4034cc8d77d fixes the
path checks in check_reduced_name[_with_privilege]() to
prevent unintended access via wide links.

The fix fails to correctly treat a corner case where the share
path is "/". This case is important for some real world
scenarios, notably the use of the glusterfs VFS module:

For the share path "/", the newly introduced checks deny all
operations in the share.

This change fixes the checks for the corner case.
The point is that the assumptions on which the original
checks are based are not true for the rootdir "/" case.
This is the case where the rootdir starts _and ends_ with
a slash. Hence a subdirectory does not continue with a
slash after the rootdir, since the candidate path has
been normalized.

This fix just omits the string comparison and the
next character checks in the case of rootdir "/",
which is correct because we know that the candidate
path is normalized and hence starts with a '/'.

The patch is fairly minimal, but changes indentation,
hence best viewed with 'git show -w'.

A side effect is that the rootdir="/" case needs
one strncmp less.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11647

Pair-Programmed-With: Jose A. Rivera <jarrpa at samba.org>

Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Jose A. Rivera <jarrpa at samba.org>
---
 source3/smbd/vfs.c | 78 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 27b38d6..1a2ee3d 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -982,7 +982,6 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
 	struct smb_filename *smb_fname_cwd = NULL;
 	struct privilege_paths *priv_paths = NULL;
 	int ret;
-	bool matched;
 
 	DEBUG(3,("check_reduced_name_with_privilege [%s] [%s]\n",
 			fname,
@@ -1077,18 +1076,32 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
 	}
 
 	rootdir_len = strlen(conn_rootdir);
-	matched = (strncmp(conn_rootdir, resolved_name, rootdir_len) == 0);
-
-	if (!matched || (resolved_name[rootdir_len] != '/' &&
-			 resolved_name[rootdir_len] != '\0')) {
-		DEBUG(2, ("check_reduced_name_with_privilege: Bad access "
-			"attempt: %s is a symlink outside the "
-			"share path\n",
-			dir_name));
-		DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
-		DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
-		status = NT_STATUS_ACCESS_DENIED;
-		goto err;
+
+	/*
+	 * In the case of rootdir_len == 1, we know that conn_rootdir is
+	 * "/", and we also know that resolved_name starts with a slash.
+	 * So, in this corner case, resolved_name is automatically a
+	 * sub-directory of the conn_rootdir. Thus we can skip the string
+	 * comparison and the next character checks (which are even
+	 * wrong in this case).
+	 */
+	if (rootdir_len != 1) {
+		bool matched;
+
+		matched = (strncmp(conn_rootdir, resolved_name,
+				rootdir_len) == 0);
+
+		if (!matched || (resolved_name[rootdir_len] != '/' &&
+				 resolved_name[rootdir_len] != '\0')) {
+			DEBUG(2, ("check_reduced_name_with_privilege: Bad "
+				"access attempt: %s is a symlink outside the "
+				"share path\n",
+				dir_name));
+			DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
+			DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
+			status = NT_STATUS_ACCESS_DENIED;
+			goto err;
+		}
 	}
 
 	/* Now ensure that the last component either doesn't
@@ -1220,7 +1233,6 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
 	if (!allow_widelinks || !allow_symlinks) {
 		const char *conn_rootdir;
 		size_t rootdir_len;
-		bool matched;
 
 		conn_rootdir = SMB_VFS_CONNECTPATH(conn, fname);
 		if (conn_rootdir == NULL) {
@@ -1231,17 +1243,33 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
 		}
 
 		rootdir_len = strlen(conn_rootdir);
-		matched = (strncmp(conn_rootdir, resolved_name,
-				rootdir_len) == 0);
-		if (!matched || (resolved_name[rootdir_len] != '/' &&
-				 resolved_name[rootdir_len] != '\0')) {
-			DEBUG(2, ("check_reduced_name: Bad access "
-				"attempt: %s is a symlink outside the "
-				"share path\n", fname));
-			DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
-			DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
-			SAFE_FREE(resolved_name);
-			return NT_STATUS_ACCESS_DENIED;
+
+		/*
+		 * In the case of rootdir_len == 1, we know that
+		 * conn_rootdir is "/", and we also know that
+		 * resolved_name starts with a slash.  So, in this
+		 * corner case, resolved_name is automatically a
+		 * sub-directory of the conn_rootdir. Thus we can skip
+		 * the string comparison and the next character checks
+		 * (which are even wrong in this case).
+		 */
+		if (rootdir_len != 1) {
+			bool matched;
+
+			matched = (strncmp(conn_rootdir, resolved_name,
+					rootdir_len) == 0);
+			if (!matched || (resolved_name[rootdir_len] != '/' &&
+					 resolved_name[rootdir_len] != '\0')) {
+				DEBUG(2, ("check_reduced_name: Bad access "
+					"attempt: %s is a symlink outside the "
+					"share path\n", fname));
+				DEBUGADD(2, ("conn_rootdir =%s\n",
+					     conn_rootdir));
+				DEBUGADD(2, ("resolved_name=%s\n",
+					     resolved_name));
+				SAFE_FREE(resolved_name);
+				return NT_STATUS_ACCESS_DENIED;
+			}
 		}
 
 		/* Extra checks if all symlinks are disallowed. */
-- 
2.5.0

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151223/a0cd64ea/signature.sig>


More information about the samba-technical mailing list