[PATCH] Fix bug 12721 - CVE-2017-2619 regression with "follow symlinks = no"
Jeremy Allison
jra at samba.org
Tue Mar 28 00:13:44 UTC 2017
On Mon, Mar 27, 2017 at 11:44:19PM +0300, Uri Simchoni via samba-technical wrote:
> On 03/27/2017 10:23 PM, Jeremy Allison wrote:
> > This one was really interesting. It actually isn't
> > related (directly) to the CVE-2017-2619 fix. That
> > fix exposed an incorrect assumption that Samba has
> > had for a few years. Quite simply, on a POSIX
> > filesystem the directory entries "." and ".."
> > by design can *NEVER* be symlinks - and we were treating
> > them as if they could be.
> >
> > Fix and regression test attached.
> >
> > Please review and push if happy, and I'll back-port
> > for the production releases.
> >
> > Jeremy.
> >
> Pushed.
There's a second part needed to this bugfix (see
Ralph's comment on bug:
https://bugzilla.samba.org/show_bug.cgi?id=12721
Attached is my proposed patch. Works, but please
don't push yet until I finish writing the test
case for it).
As I changed the interface to check_reduced_name()
slightly (added a parameter) I pushed as two separate
chunks, #1 changes the interface and callers but
doesn't add the logic change, #2 adds the logic
change in check_reduced_name() that uses the extra
parameter. I can squash these if you think it's
easier to understand as one chunk. Let me know !
Jeremy.
-------------- next part --------------
>From 9b99f6bdbb3b0fdd0211dc618a5ce398c268943b Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 17:04:58 -0700
Subject: [PATCH 1/2] s3: smbd: Fix "follow symlink = no" regression part 2.
Add an extra paramter to cwd_name to check_reduced_name().
If cwd_name == NULL then fname is a client given path relative
to the base path of the share.
If cwd_name != NULL then fname is a client given path relative
to cwd_name, which is relative to the root of the share.
Not yet used, logic added in the next commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/filename.c | 2 +-
source3/smbd/open.c | 2 +-
source3/smbd/proto.h | 4 +++-
source3/smbd/vfs.c | 10 +++++++++-
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
index efe52a0..2d85e8d 100644
--- a/source3/smbd/filename.c
+++ b/source3/smbd/filename.c
@@ -1242,7 +1242,7 @@ NTSTATUS check_name(connection_struct *conn, const char *name)
}
if (!lp_widelinks(SNUM(conn)) || !lp_follow_symlinks(SNUM(conn))) {
- status = check_reduced_name(conn,name);
+ status = check_reduced_name(conn, NULL, name);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(5,("check_name: name %s failed with %s\n",name,
nt_errstr(status)));
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index a6e61e7..50f8a5e 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -558,7 +558,7 @@ static int non_widelink_open(struct connection_struct *conn,
}
/* Ensure the relative path is below the share. */
- status = check_reduced_name(conn, final_component);
+ status = check_reduced_name(conn, parent_dir, final_component);
if (!NT_STATUS_IS_OK(status)) {
saved_errno = map_errno_from_nt_status(status);
goto out;
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 2e8df29..841a095 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -1226,7 +1226,9 @@ const char *vfs_readdirname(connection_struct *conn, void *p,
SMB_STRUCT_STAT *sbuf, char **talloced);
int vfs_ChDir(connection_struct *conn, const char *path);
char *vfs_GetWd(TALLOC_CTX *ctx, connection_struct *conn);
-NTSTATUS check_reduced_name(connection_struct *conn, const char *fname);
+NTSTATUS check_reduced_name(connection_struct *conn,
+ const char *cwd_name,
+ const char *fname);
NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
const char *fname,
struct smb_request *smbreq);
diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 5133fe5..b05e4b2 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1179,9 +1179,17 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
/*******************************************************************
Reduce a file name, removing .. elements and checking that
it is below dir in the heirachy. This uses realpath.
+
+ If cwd_name == NULL then fname is a client given path relative
+ to the base path of the share.
+
+ If cwd_name != NULL then fname is a client given path relative
+ to cwd_name, which is relative to the root of the share.
********************************************************************/
-NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
+NTSTATUS check_reduced_name(connection_struct *conn,
+ const char *cwd_name,
+ const char *fname)
{
char *resolved_name = NULL;
bool allow_symlinks = true;
--
2.7.4
>From 8fe2308c002f04063f6c5117626467f06845c000 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 17:09:38 -0700
Subject: [PATCH 2/2] s3: smbd: Fix "follow symlink = no" regression part 2.
Use the cwd_name parameter to reconstruct the original
client name for symlink testing.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/vfs.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index b05e4b2..ca54ce9 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1333,6 +1333,25 @@ NTSTATUS check_reduced_name(connection_struct *conn,
}
p++;
+
+ /*
+ * If cwd_name is present and not ".",
+ * then fname is relative to that, not
+ * the root of the share. Make sure the
+ * path we check is the one the client
+ * sent (cwd_name+fname).
+ */
+ if (cwd_name != NULL && !ISDOT(cwd_name)) {
+ fname = talloc_asprintf(talloc_tos(),
+ "%s/%s",
+ cwd_name,
+ fname);
+ if (fname == NULL) {
+ SAFE_FREE(resolved_name);
+ return NT_STATUS_NO_MEMORY;
+ }
+ }
+
if (strcmp(fname, p)!=0) {
DEBUG(2, ("check_reduced_name: Bad access "
"attempt: %s is a symlink to %s\n",
--
2.7.4
More information about the samba-technical
mailing list