RFC: CVE-2017-2619 fix breaks accessing previous versions of directories with snapshots in subdirectories of the share

Ralph Böhme slow at samba.org
Fri Jul 7 12:12:53 UTC 2017


Hi!

As explained in <https://bugzilla.samba.org/show_bug.cgi?id=12885>:

With shadow:snapdirseverywhere=true and a snapshot directory that

* is a subdirectory of a share

* and that contains a snapshot directory

we fail the symlink check in the new function non_widelink_open() because
parent_dirname() cuts off the subdirectory name leaving only the @GMT stanza
which is then interpreted by the called functions as being relative to the
parent directory which it isn't.

The simplest fix as far as I can see is to leverage the fact that (given the
system defines O_DIRECTORY) we know when we're called for a directory, so we can
just directly chdir() into the path passed by the caller.

Can we rely here on O_DIRECTORY? Linux has it, FreeBSD has it, Solaris has it
and we probably don't care about the rest.

The subsequent security check done in check_reduced_name() should continue to
work with this change.

I've just fire of a private autobuild with the patchset and will follow up with
the results (fingers crossed :) ).

Cheerio!
-slow
-------------- next part --------------
From 48211a8b2d01b22064c16093be0a1be95f9b9ddb Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 7 Jul 2017 12:57:57 +0200
Subject: [PATCH 1/2] s3/smbd: let non_widelink_open() chdir() to directories
 directly
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If the caller passes O_DIRECTORY we just try to chdir() to smb_fname
directly, not to the parent directory.

The security check in check_reduced_name() will continue to work, but
this fixes the case of an open() for a previous version of a
subdirectory that contains snapshopt.

Eg:

[share]
    path = /shares/test
    vfs objects = shadow_copy2
    shadow:snapdir = .snapshots
    shadow:snapdirseverywhere = yes

Directory tree with fake snapshots:

$ tree -a /shares/test/
/shares/test/
├── dir
│   ├── file
│   └── .snapshots
│       └── @GMT-2017.07.04-04.30.12
│           └── file
├── dir2
│   └── file
├── file
├── .snapshots
│   └── @GMT-2001.01.01-00.00.00
│       ├── dir2
│       │   └── file
│       └── file
└── testfsctl.dat

./bin/smbclient -U slow%x //localhost/share -c 'ls @GMT-2017.07.04-04.30.12/dir/*'
NT_STATUS_OBJECT_NAME_NOT_FOUND listing \@GMT-2017.07.04-04.30.12\dir\*

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12885

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/smbd/open.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 3ccee36..7781a6f 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -550,12 +550,32 @@ static int non_widelink_open(struct connection_struct *conn,
 	char *parent_dir = NULL;
 	struct smb_filename parent_dir_fname = {0};
 	const char *final_component = NULL;
+	bool is_directory = false;
+	bool ok;
 
-	if (!parent_dirname(talloc_tos(),
-			smb_fname->base_name,
-			&parent_dir,
-			&final_component)) {
-		goto out;
+#ifdef O_DIRECTORY
+	if (flags & O_DIRECTORY) {
+		is_directory = true;
+	}
+#endif
+
+	if (is_directory) {
+		parent_dir = talloc_strdup(talloc_tos(), smb_fname->base_name);
+		if (parent_dir == NULL) {
+			saved_errno = errno;
+			goto out;
+		}
+
+		final_component = ".";
+	} else {
+		ok = parent_dirname(talloc_tos(),
+				    smb_fname->base_name,
+				    &parent_dir,
+				    &final_component);
+		if (!ok) {
+			saved_errno = errno;
+			goto out;
+		}
 	}
 
 	parent_dir_fname = (struct smb_filename) { .base_name = parent_dir };
-- 
2.9.4


From eedf92df6c26ed29ed5a93cdb34e6da22982b6a3 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 7 Jul 2017 13:12:19 +0200
Subject: [PATCH 2/2] selftest: add a test for accessing previous version of
 directories with snapdirseverywhere

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12885

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/script/tests/test_shadow_copy.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index 783e7f32..eba873f 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -221,6 +221,26 @@ test_fetch_snap_file()
         -c "get ${SNAPSHOTS[$snapidx]}/$path $WORKDIR/foo"
 }
 
+# Test fetching a previous version of a file
+test_fetch_snap_dir()
+{
+    local share
+    local path
+    local snapidx
+
+    share=$1
+    path=$2
+    snapidx=$3
+
+    # This first command is not strictly needed, but it causes the snapshots to
+    # appear in a network trace which helps debugging...
+    $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP \
+        -c "allinfo $path"
+
+    $SMBCLIENT -U$USERNAME%$PASSWORD "//$SERVER/$share" -I $SERVER_IP \
+        -c "ls ${SNAPSHOTS[$snapidx]}/$path/*"
+}
+
 test_shadow_copy_fixed()
 {
     local share     #share to contact
@@ -329,6 +349,9 @@ test_shadow_copy_everywhere()
         test_fetch_snap_file $share "bar/lfoo" 3 || \
         failed=`expr $failed + 1`
 
+    testit "list a previous version directory" \
+        test_fetch_snap_dir $share "bar" 6 || \
+        failed=`expr $failed + 1`
 }
 
 test_shadow_copy_format()
-- 
2.9.4



More information about the samba-technical mailing list