Attachment 17582 Details for Bug 15207 – CVE-2022-3592 patch for 4.17

[patch] CVE-2022-3592 patch for 4.17

CVE-2022-3592-4.17.2.patch (text/plain), 8.93 KB, created by Ralph Böhme on 2022-10-19 08:45:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2022-10-19 08:45:50 UTC

Size: 8.93 KB

patch

obsolete

>From 36fff1ba12176a26678a9efe7bfd8734e694e853 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/4] CVE-2022-3592 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 179c3e11a76b..9b0c902c0d4d 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);
> 			status = NT_STATUS_OBJECT_NAME_INVALID;
>-- 
>2.37.3
>
>
>From 83c11d8c6046e0586b24252e2dbe1f4bd932457c 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/4] CVE-2022-3592 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 971d53442161..5caee329baf3 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"
> 	ln -s "dir_inside_share_noperms" "symlink_dir_inside_share_noperms"
> 	mkdir "dir_inside_share_noperms/noperm_subdir_exists"
> 	touch "dir_inside_share_noperms/noperm_subdir_exists/noperm_subdir_file_exists"
>+
>+	# 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.37.3
>
>
>From dcf76926e107da16767b30ee743e4736e7e7ea20 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/4] CVE-2022-3592 lib: add subdir_of() to
> source3/lib/util_path.c
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=15207
>Signed-off-by: Volker Lendecke <vl@samba.org>
>---
> source3/lib/util_path.c | 51 +++++++++++++++++++++++++++++++++++++++++
> source3/lib/util_path.h |  4 ++++
> 2 files changed, 55 insertions(+)
>
>diff --git a/source3/lib/util_path.c b/source3/lib/util_path.c
>index 3591589cb8e1..b8dfec40b03e 100644
>--- a/source3/lib/util_path.c
>+++ b/source3/lib/util_path.c
>@@ -23,6 +23,7 @@
> 
> #include "replace.h"
> #include <talloc.h>
>+#include "lib/util/debug.h"
> #include "lib/util/samba_util.h"
> #include "lib/util_path.h"
> 
>@@ -204,3 +205,53 @@ char *canonicalize_absolute_path(TALLOC_CTX *ctx, const char *pathname_in)
> 	*p++ = '\0';
> 	return pathname;
> }
>+
>+/*
>+ * 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 3e7d04de5507..0ea508bf5bbc 100644
>--- a/source3/lib/util_path.h
>+++ b/source3/lib/util_path.h
>@@ -31,5 +31,9 @@ char *lock_path(TALLOC_CTX *mem_ctx, const char *name);
> char *state_path(TALLOC_CTX *mem_ctx, const char *name);
> char *cache_path(TALLOC_CTX *mem_ctx, const char *name);
> char *canonicalize_absolute_path(TALLOC_CTX *ctx, const char *abs_path);
>+bool subdir_of(const char *parent,
>+	       size_t parent_len,
>+	       const char *subdir,
>+	       const char **_relative);
> 
> #endif
>-- 
>2.37.3
>
>
>From a7755f5573a75673c2f652e60c89464443b9619b Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Sat, 15 Oct 2022 13:37:17 +0200
>Subject: [PATCH 4/4] CVE-2022-3592 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 5caee329baf3..1fb9c67420d9 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 0be8e320ffa0..e7873eb124f0 100644
>--- a/source3/smbd/filename.c
>+++ b/source3/smbd/filename.c
>@@ -1440,6 +1440,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;
>@@ -1512,17 +1513,17 @@ NTSTATUS filename_convert_dirfsp(
> 
> 	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.37.3
>

Flags:

vl: review+
slow: ci-passed+

Actions: View

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