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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
>
> On Jul 7, 2017, at 07:12, Ralph Böhme via samba-technical <[hidden email]> wrote:
>
> 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
> <look>

Out of curiosity, does this break MSDFS links since they're internally handled like symlinks IIRC?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 07:58:10AM -0500, Scott Lovenberg wrote:

> >
> > On Jul 7, 2017, at 07:12, Ralph Böhme via samba-technical <[hidden email]> wrote:
> >
> > 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
> > <look>
>
> Out of curiosity, does this break MSDFS links since they're internally handled
> like symlinks IIRC?

la, la, la, I can't hear you, la, la, la. ;)

No, I don't think so. The breakage with shadow_copy2 is triggered by the logic
that tries to figure out the corrent (tm) location of the .snapshots directory.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 02:12:53PM +0200, Ralph Böhme via samba-technical wrote:
> I've just fire of a private autobuild with the patchset and will follow up with
> the results (fingers crossed :) ).

just passed. *phew*

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 02:12:53PM +0200, Ralph Böhme wrote:

> 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 :) ).

Great catch Ralph. That's a really minimal fix with no disruption
to the security checks whatsoever !

Words fail me when I try and articulate how much I *HATE* the
shadow_copy2 code (even after I fixed up a lot of it :-).

Jeremy.

> From 48211a8b2d01b22064c16093be0a1be95f9b9ddb Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 09:30:17AM -0700, Jeremy Allison via samba-technical wrote:

> On Fri, Jul 07, 2017 at 02:12:53PM +0200, Ralph Böhme wrote:
> > 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 :) ).
>
> Great catch Ralph. That's a really minimal fix with no disruption
> to the security checks whatsoever !
>
> Words fail me when I try and articulate how much I *HATE* the
> shadow_copy2 code (even after I fixed up a lot of it :-).

In case it wasn't clear - RB+.

Thanks !

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 10:12:34AM -0700, Jeremy Allison wrote:
> In case it wasn't clear - RB+.

thanks! :) Will push.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Jul 07, 2017 at 09:30:17AM -0700, Jeremy Allison via samba-technical wrote:
> Words fail me when I try and articulate how much I *HATE* the
> shadow_copy2 code (even after I fixed up a lot of it :-).

Just take a look at the original shadow_copy2 code. I think everybody
has to get his/her hands dirty before this becomes fine.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Loading...