Attachment 17566 Details for Bug 15207 – Volker's patch for master.

[patch] Volker's patch for master.

15207.txt (text/plain), 10.85 KB, created by Jeremy Allison on 2022-10-17 20:33:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2022-10-17 20:33:26 UTC

Size: 10.85 KB

patch

obsolete

>From 01112000110c5770973771aac894fc0029e9c04a Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 17 Oct 2022 18:06:02 +0200
>Subject: [PATCH 1/5] smbd: No empty path components in
> openat_pathref_dirfsp_nosymlink()
>
>Upper layers must have filtered this, everything else is a bug
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/smbd/files.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/source3/smbd/files.c b/source3/smbd/files.c
>index 87c1f0f6301..64297f18773 100644
>--- a/source3/smbd/files.c
>+++ b/source3/smbd/files.c
>@@ -817,6 +817,12 @@ NTSTATUS openat_pathref_dirfsp_nosymlink(
> 
> 		next = strv_next(path, rel_fname.base_name);
> 
>+		/*
>+		 * Path sanitizing further up has cleaned or rejected
>+		 * empty path components. Assert this here.
>+		 */
>+		SMB_ASSERT(rel_fname.base_name[0] != '\0');
>+
> 		if (ISDOT(rel_fname.base_name) ||
> 		    ISDOTDOT(rel_fname.base_name)) {
> 			DBG_DEBUG("%s contains a dot\n", path_in);
>-- 
>2.30.2
>
>
>From d6c4e16f9f1061068ec4cdc5be5dbb656c7901ef Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Sat, 15 Oct 2022 14:09:55 +0200
>Subject: [PATCH 2/5] torture3: Show that our symlink traversal checks are
> insecure
>
>This test shows that we don't properly check whether symlink targets
>are inside the exported share. Linking to <share-root>a/etc makes us
>loop back into filename_convert_dirfsp_nosymlink() with /etc as a
>directory name.
>
>On Linux systems with openat2(RESOLVE_NO_SYMLINKS) we pass "/etc"
>directly into that call after some checks for "."/".." as invalid file
>name components. "/etc" is okay for openat2(), but this test must also
>succeed on systems without RESOLVE_NO_SYMLINKS (sn-devel-184 for
>example). On systems without RESOLVE_NO_SYMLINKS split up the path
>"/etc" into path components, in this case "" and "etc". So we pass ""
>down to openat(), which correctly fails with ENOENT.
>
>Summary: Only with RESOLVE_NO_SYMLINKS we're hit by bug 15207, and
>this test shows by expecting CONNECTION_DISCONNECTED that we violate
>the internal assumption of empty path components with an unexpected
>symlink target, making it testable on systems with and without
>RESOLVE_NO_SYMLINKS.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/script/tests/test_symlink_traversal_smb2.sh | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/source3/script/tests/test_symlink_traversal_smb2.sh b/source3/script/tests/test_symlink_traversal_smb2.sh
>index efd4353c533..08929f0962f 100755
>--- a/source3/script/tests/test_symlink_traversal_smb2.sh
>+++ b/source3/script/tests/test_symlink_traversal_smb2.sh
>@@ -144,6 +144,9 @@ chmod 0 "$dir_outside_share_noperms"
> 	mkdir "dir_inside_share_noperms/noperm_subdir_exists"
> 	touch "dir_inside_share_noperms/noperm_subdir_exists/noperm_subdir_file_exists"
> 	chmod 0 "dir_inside_share_noperms"
>+
>+	# Symlink pointing out of the share
>+	ln -s "$share_test_dir"a"/etc" x
> )
> 
> #
>@@ -345,6 +348,7 @@ test_symlink_traversal_SMB2()
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists" "" "NT_STATUS_FILE_IS_A_DIRECTORY" || return 1
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists/noexist1" "" "NT_STATUS_OBJECT_NAME_NOT_FOUND" || return 1
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists/noexist1/noexist2" "" "NT_STATUS_OBJECT_PATH_NOT_FOUND" || return 1
>+	smbclient_expect_error "get" "x/passwd" "passwd" "NT_STATUS_CONNECTION_DISCONNECTED" || return 1
> 
> 	#
> 	# Test paths within share with no permissions.
>-- 
>2.30.2
>
>
>From ce977b50aa90be1c8dc6a86c1292f734ad270a93 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Sat, 15 Oct 2022 13:26:48 +0200
>Subject: [PATCH 3/5] lib: lib/util/fault.h requires _SAMBA_DEBUG_H for
> SMB_ASSERT()
>
>fault.h has:
>
>which leads to SMB_ASSERT not being defined when you include
>samba_util.h (and thus fault.h) before debug.h.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/lib/util_path.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/lib/util_path.c b/source3/lib/util_path.c
>index 5b7c01b05e3..33827fcf894 100644
>--- a/source3/lib/util_path.c
>+++ b/source3/lib/util_path.c
>@@ -23,8 +23,8 @@
> 
> #include "replace.h"
> #include <talloc.h>
>-#include "lib/util/samba_util.h"
> #include "lib/util/debug.h"
>+#include "lib/util/samba_util.h"
> #include "lib/util_path.h"
> 
> struct loadparm_substitution;
>-- 
>2.30.2
>
>
>From 4b5273bd97c77d3b0600394f6beb9b20e440b411 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Sat, 15 Oct 2022 13:29:14 +0200
>Subject: [PATCH 4/5] lib: Move subdir_of() to source3/lib/util_path.c
>
>Make it available for other components
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/lib/util_path.c | 50 +++++++++++++++++++++++++++++++++++++++
> source3/lib/util_path.h |  4 ++++
> source3/smbd/open.c     | 52 -----------------------------------------
> 3 files changed, 54 insertions(+), 52 deletions(-)
>
>diff --git a/source3/lib/util_path.c b/source3/lib/util_path.c
>index 33827fcf894..5a94b391dd6 100644
>--- a/source3/lib/util_path.c
>+++ b/source3/lib/util_path.c
>@@ -304,3 +304,53 @@ bool extract_snapshot_token(char *fname, NTTIME *twrp)
> 
> 	return true;
> }
>+
>+/*
>+ * Take two absolute paths, figure out if "subdir" is a proper
>+ * subdirectory of "parent". Return the component relative to the
>+ * "parent" without the potential "/". Take care of "parent"
>+ * possibly ending in "/".
>+ */
>+bool subdir_of(const char *parent,
>+	       size_t parent_len,
>+	       const char *subdir,
>+	       const char **_relative)
>+{
>+	const char *relative = NULL;
>+	bool matched;
>+
>+	SMB_ASSERT(parent[0] == '/');
>+	SMB_ASSERT(subdir[0] == '/');
>+
>+	if (parent_len == 1) {
>+		/*
>+		 * Everything is below "/"
>+		 */
>+		*_relative = subdir+1;
>+		return true;
>+	}
>+
>+	if (parent[parent_len-1] == '/') {
>+		parent_len -= 1;
>+	}
>+
>+	matched = (strncmp(subdir, parent, parent_len) == 0);
>+	if (!matched) {
>+		return false;
>+	}
>+
>+	relative = &subdir[parent_len];
>+
>+	if (relative[0] == '\0') {
>+		*_relative = relative; /* nothing left */
>+		return true;
>+	}
>+
>+	if (relative[0] == '/') {
>+		/* End of parent must match a '/' in subdir. */
>+		*_relative = relative+1;
>+		return true;
>+	}
>+
>+	return false;
>+}
>diff --git a/source3/lib/util_path.h b/source3/lib/util_path.h
>index 33699da28c2..5b0a1f13e38 100644
>--- a/source3/lib/util_path.h
>+++ b/source3/lib/util_path.h
>@@ -49,5 +49,9 @@ bool clistr_is_previous_version_path(const char *path,
> 				     const char **startp,
> 				     const char **endp,
> 				     NTTIME *ptwrp);
>+bool subdir_of(const char *parent,
>+	       size_t parent_len,
>+	       const char *subdir,
>+	       const char **_relative);
> 
> #endif
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index 9d85b0d8ef8..d4a679a4cb0 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -475,58 +475,6 @@ static NTSTATUS check_base_file_access(struct files_struct *fsp,
> 					access_mask);
> }
> 
>-/*
>- * Take two absolute paths, figure out if "subdir" is a proper
>- * subdirectory of "parent". Return the component relative to the
>- * "parent" without the potential "/". Take care of "parent"
>- * possibly ending in "/".
>- */
>-static bool subdir_of(
>-	const char *parent,
>-	size_t parent_len,
>-	const char *subdir,
>-	const char **_relative)
>-
>-{
>-	const char *relative = NULL;
>-	bool matched;
>-
>-	SMB_ASSERT(parent[0] == '/');
>-	SMB_ASSERT(subdir[0] == '/');
>-
>-	if (parent_len == 1) {
>-		/*
>-		 * Everything is below "/"
>-		 */
>-		*_relative = subdir+1;
>-		return true;
>-	}
>-
>-	if (parent[parent_len-1] == '/') {
>-		parent_len -= 1;
>-	}
>-
>-	matched = (strncmp(subdir, parent, parent_len) == 0);
>-	if (!matched) {
>-		return false;
>-	}
>-
>-	relative = &subdir[parent_len];
>-
>-	if (relative[0] == '\0') {
>-		*_relative = relative; /* nothing left */
>-		return true;
>-	}
>-
>-	if (relative[0] == '/') {
>-		/* End of parent must match a '/' in subdir. */
>-		*_relative = relative+1;
>-		return true;
>-	}
>-
>-	return false;
>-}
>-
> static NTSTATUS chdir_below_conn(
> 	TALLOC_CTX *mem_ctx,
> 	connection_struct *conn,
>-- 
>2.30.2
>
>
>From 0c5f940bb39164773b9b36febc6283c1e3099736 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Sat, 15 Oct 2022 13:37:17 +0200
>Subject: [PATCH 5/5] smbd: Slightly simplify filename_convert_dirfsp()
>
>subdir_of() calculates the share-relative rest for us, don't do the
>strlen(connectpath) calculation twice. subdir_of() also checks that
>the target properly ends on a directory. With just strncmp a symlink
>to x->/aa/etc would qualify as in share /a, so a "get x/passwd" leads to a
>pretty unfortunate result. This is the proper fix for bug 15207, so we
>need to change the expected error code to OBJECT_PATH_NOT_FOUND
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/script/tests/test_symlink_traversal_smb2.sh |  2 +-
> source3/smbd/filename.c                             | 11 ++++++-----
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/source3/script/tests/test_symlink_traversal_smb2.sh b/source3/script/tests/test_symlink_traversal_smb2.sh
>index 08929f0962f..38719cc8eae 100755
>--- a/source3/script/tests/test_symlink_traversal_smb2.sh
>+++ b/source3/script/tests/test_symlink_traversal_smb2.sh
>@@ -348,7 +348,7 @@ test_symlink_traversal_SMB2()
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists" "" "NT_STATUS_FILE_IS_A_DIRECTORY" || return 1
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists/noexist1" "" "NT_STATUS_OBJECT_NAME_NOT_FOUND" || return 1
> 	smbclient_expect_error "get" "symlink_to_dir_exists/subdir_exists/noexist1/noexist2" "" "NT_STATUS_OBJECT_PATH_NOT_FOUND" || return 1
>-	smbclient_expect_error "get" "x/passwd" "passwd" "NT_STATUS_CONNECTION_DISCONNECTED" || return 1
>+	smbclient_expect_error "get" "x/passwd" "passwd" "NT_STATUS_OBJECT_PATH_NOT_FOUND" || return 1
> 
> 	#
> 	# Test paths within share with no permissions.
>diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
>index 79bb7dffe99..5bfa4b2f1df 100644
>--- a/source3/smbd/filename.c
>+++ b/source3/smbd/filename.c
>@@ -1369,6 +1369,7 @@ NTSTATUS filename_convert_dirfsp(
> 	struct smb_filename **_smb_fname)
> {
> 	char *substitute = NULL;
>+	const char *relative = NULL;
> 	size_t unparsed = 0;
> 	NTSTATUS status;
> 	char *target = NULL;
>@@ -1441,17 +1442,17 @@ next:
> 
> 	DBG_DEBUG("abs_target_canon=%s\n", abs_target_canon);
> 
>-	in_share = strncmp(
>-		abs_target_canon,
>+	in_share = subdir_of(
> 		conn->connectpath,
>-		strlen(conn->connectpath)) == 0;
>+		strlen(conn->connectpath),
>+		abs_target_canon,
>+		&relative);
> 	if (!in_share) {
> 		DBG_DEBUG("wide link to %s\n", abs_target_canon);
> 		return NT_STATUS_OBJECT_PATH_NOT_FOUND;
> 	}
> 
>-	name_in = talloc_strdup(
>-		mem_ctx, abs_target_canon + strlen(conn->connectpath) + 1);
>+	name_in = talloc_strdup(mem_ctx, relative);
> 
> 	symlink_redirects += 1;
> 
>-- 
>2.30.2
>

Flags:

slow: review+

Actions: View

Attachments on bug 15207: 17566 | 17575 | 17576 | 17578 | 17579 | 17580 | 17581 | 17582 | 17584