Bug 12531 - vfs_shadow_copy2 doesn't cope with server changing directories.
Summary: vfs_shadow_copy2 doesn't cope with server changing directories.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 12496
  Show dependency treegraph
 
Reported: 2017-01-20 17:04 UTC by Jeremy Allison
Modified: 2017-03-28 05:53 UTC (History)
4 users (show)

See Also:


Attachments
Basic fix for master in git-am format. (49.86 KB, patch)
2017-01-20 18:37 UTC, Jeremy Allison
no flags Details
Complete fix for master in git-am format. (60.53 KB, patch)
2017-01-23 19:04 UTC, Jeremy Allison
no flags Details
git-am fix for master. (62.51 KB, patch)
2017-01-25 18:33 UTC, Jeremy Allison
no flags Details
git-am fix for master (67.12 KB, patch)
2017-01-26 19:11 UTC, Jeremy Allison
no flags Details
git-am fix for master (69.94 KB, patch)
2017-01-27 00:23 UTC, Jeremy Allison
no flags Details
git-am fix for master. (74.81 KB, patch)
2017-01-27 01:28 UTC, Jeremy Allison
no flags Details
git-am fix for master. (74.81 KB, patch)
2017-01-27 19:25 UTC, Jeremy Allison
uri: review+
jra: review? (obnox)
Details
git-am fix for 4.6.next (77.10 KB, patch)
2017-01-31 17:19 UTC, Jeremy Allison
uri: review+
Details
git-am fix for 4.5.next (77.09 KB, patch)
2017-01-31 17:20 UTC, Jeremy Allison
uri: review+
Details
git-am fix for 4.4.next (77.28 KB, patch)
2017-01-31 18:29 UTC, Jeremy Allison
no flags Details
git-am fix for 4.4.next (76.73 KB, patch)
2017-02-03 16:44 UTC, Jeremy Allison
uri: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2017-01-20 17:04:23 UTC
When doing opens with backup intent, or as root, smbd can change directories within the exported share path. The code in vfs_shadow_copy2 does not cope well with this. Patches to follow.

Jeremy.
Comment 1 Jeremy Allison 2017-01-20 17:06:09 UTC
Adding uri to CC: list as he has been helping with this.
Comment 2 Jeremy Allison 2017-01-20 18:37:18 UTC
Created attachment 12844 [details]
Basic fix for master in git-am format.

I'm planning to split up the vfs_shadow_copy2 changes further, but wanted to keep a record of this version as the basis for me to work on.
Comment 3 Jeremy Allison 2017-01-23 19:04:34 UTC
Created attachment 12846 [details]
Complete fix for master in git-am format.

Uri, this is what I'd like to go with for master. Please review. Thanks a *LOT* for your help on this.
Comment 4 Uri Simchoni 2017-01-25 15:06:21 UTC
I "just" have 14/16 to cover, the rest RB+ me except for the following comments. Will (hopefully) get back to 14/16 later today.

[2/16]
IMHO a comment what "canonicalize" means is in order (for example, in the realpath(3) manpage, a canonicalized path is one with expanded symbolic links)

+				/* Go back one level... */
+				/*
+				 * Decrement d first as d points to
+				 * the *next* char to write into.
+				 */
+				for (d--; d > destname; d--) {
+					if (*d == '/') {
+						break;
+					}
+				}

If at the end of this loop we're back at destname, shouldn't we do *d++ = '/' ?

[15/16]
there's still a small leak, isn't there? the last one. Can be free'd with a talloc destructor (with an added patch)

[16/16]
Can you explain in the commit message why you're doing this? I suppose it's a speed thing, never came across a "try not to set errno sparingly" convention.
Comment 5 Jeremy Allison 2017-01-25 17:48:19 UTC
(In reply to Uri Simchoni from comment #4)

> [2/16]
> IMHO a comment what "canonicalize" means is in order (for example, in the > realpath(3) manpage, a canonicalized path is one with expanded symbolic links)

OK, will fix - thanks !

> +				/* Go back one level... */
> +				/*
> +				 * Decrement d first as d points to
> +				 * the *next* char to write into.
> +				 */
> +				for (d--; d > destname; d--) {
> +					if (*d == '/') {
> +						break;
> +					}
> +				}
>
> If at the end of this loop we're back at destname, shouldn't we do *d++ = '/' ?

Actually no. We can't get here with d == destname. The reason is
the case just above:


140                                 /*
141                                  * Are we at the start ?
142                                  * Can't go back further if so.
143                                  */
144                                 if (d <= destname) {
145                                         *d++ = '/'; /* Can't delete root */
146                                         continue;
147                                 }


Consider the degenerate case "/.."

We end up here:

126                 if (start_of_name_component) {
127                         if ((s[0] == '.') && (s[1] == '.') &&
128                                         (s[2] == '/' || s[2] == '\0')) {

as start_of_name_component==true and s[0]==. s[1]==. s[2]=='\0'

131                                 /* Go past the .. leaving us on the / or '\0' */
132                                 s += 2;

s now points to the '\0'

134                                 /* If  we just added a '/' - delete it */
135                                 if ((d > destname) && (*(d-1) == '/')) {
136                                         *(d-1) = '\0';
137                                         d--;
138                                 }

d == destname, and d[0] = '\0';

140                                 /*
141                                  * Are we at the start ?
142                                  * Can't go back further if so.
143                                  */
144                                 if (d <= destname) {
145                                         *d++ = '/'; /* Can't delete root */
146                                         continue;
147                                 }

Now destname = "/" (we re-add the /) and the continue stops us from getting to
the problematic case.

Once the patch:

PATCH 03/16] s3: lib: Fix old, old bug in set_conn_connectpath(),
 now in canonicalize_absolute_path().

is applied the code becomes functionally identical to source3/smbd/reply.c:check_path_syntax_internal(),
minus the NT_STATUS error returns and stream checking.

 128                         if ((s[0] == '.') && (s[1] == '.') && (IS_PATH_SEP(s[2],posix_path) || s[2] == '\0')) {
 129                                 /* Uh oh - "/../" or "\\..\\"  or "/..\0" or "\\..\0" ! */
 130 
 131                                 /*
 132                                  * No mb char starts with '.' so we're safe checking the directory separator here.
 133                                  */
 134 
 135                                 /* If  we just added a '/' - delete it */
 136                                 if ((d > path) && (*(d-1) == '/')) {
 137                                         *(d-1) = '\0';
 138                                         d--;
 139                                 }
 140 
 141                                 /* Are we at the start ? Can't go back further if so. */
 142                                 if (d <= path) {
 143                                         ret = NT_STATUS_OBJECT_PATH_SYNTAX_BAD;
 144                                         break;
 145                                 }
 146                                 /* Go back one level... */
 147                                 /* We know this is safe as '/' cannot be part of a mb sequence. */
 148                                 /* NOTE - if this assumption is invalid we are not in good shape... */
 149                                 /* Decrement d first as d points to the *next* char to write into. */
 150                                 for (d--; d > path; d--) {
 151                                         if (*d == '/')
 152                                                 break;
 153                                 }
 154                                 s += 2; /* Else go past the .. */
 155                                 /* We're still at the start of a name component, just the previous one. */
 156                                 continue;

The only difference is in source3/smbd/reply.c:check_path_syntax_internal()
we error out on the if (d <= destname) case (so don't need to increment the
s variable path pointer) whereas here we just reset destname to "/" and so
must increment s to keep walking the path.

> [15/16]
> there's still a small leak, isn't there? the last one. Can be free'd with a
> talloc destructor (with an added patch)

Yes, good catch - I'll add a destructor patch after here.

> [16/16]
> Can you explain in the commit message why you're doing this? I suppose it's a
>  speed thing, never came across a "try not to set errno sparingly" 
> convention.

No, it's actually a correctness issue. VFS calls must act like their POSIX equivalents, and the POSIX versions *only* set errno on a failure. There is actually code (or used to be :-) in the upper smbd layers that depends on errno being correct on a fail return from a VFS call.

For a compound VFS module like this, a common pattern is :

SMB_VFS_CALL_X()
{
      int ret;

      syscall1();
      ret = syscall2();
      syscall3();

      return ret;
}

Where if *any* of the contained syscallX()'s fail, they'll set errno. However, the actual errno we should return is *only* the one returned if syscall2() fails (the others are lstat's checking for existence etc.).

So what we should do to correctly return only the errno from syscall2() is:

SMB_VFS_CALL_X()
{
      int ret;
      int saved_errno = 0;

      syscall1()

      ret = syscall2();
      if (ret == -1) {
            saved_errno = errno;
      }
      syscall3()

      if (saved_errno != 0) {
           errno = saved_errno;
      }
      return ret;
}

Thanks *SO MUCH* for this review. Updated patch to follow !
Comment 6 Jeremy Allison 2017-01-25 18:33:15 UTC
Created attachment 12849 [details]
git-am fix for master.

Updated with fixes from Uri's previous comments.
Comment 7 Jeremy Allison 2017-01-25 23:57:52 UTC
Comment on attachment 12849 [details]
git-am fix for master.

(email from Uri):

Trying to wrap my head around [14/16] and how it all fits together -
that's *really* a nightmare :).

So I'm not sure I have it figured out yet, but one thing bugs me - we
set psnappath in a case where we know we have a converted path. But
there are cases where we have a converted path, and we don't know it.
That happens for example if shadow:format is set to something other than
default.

I'm refering to this code:
@@ -514,19 +539,43 @@ static bool
shadow_copy2_strip_snapshot_internal(TALLOC_CTX *mem_ctx,

                if (strncmp(parent_snapdir, snapdir, snapdirlen) == 0) {
                        DEBUG(10, ("name=%s is already converted\n", name));
-                       goto no_snapshot;
+                       if (psnappath) {
+                               /*
+                                * Need to return up to the next path
+                                * component after the @GMT-.
+                                * This will be used as the connectpath.
+                                */
+

This all runs if we've found @GMT, but if shadow:format is
SNAP-%d/%m/%Y-%H:%M:%S, then we don't find @GMT and this code doesn't run.

With the prior code this wasn't a problem because it wouldn't be
stripped, which is the desired result, end of story. But now, if we want
psnappath to be set to .snapshots/SNAP-%d/%m/%Y-%H:%M:%S, looks like it
wouldn't.

Like I said, I haven't fully figured it out, so I'm not sure this is a
real concern. shadow:format *is* tested to some extend so...
Comment 8 Jeremy Allison 2017-01-26 00:27:54 UTC
OK - I have a plan to fix this. Give me another day of coding or so. Thanks a *LOT* for the analysis.
Comment 9 Jeremy Allison 2017-01-26 01:39:06 UTC
(In reply to Jeremy Allison from comment #7)

FYI: I don't think the current code would cope with a shadow:format of
SNAP-%d/%m/%Y-%H:%M:%S, as shadow_copy2_snapshot_to_gmt() only looks at a single pathname element to find a snapshot (a name returned from READDIR()), so I'm pretty sure shadow:format can't contain '/' characters (and indeed we don't test for that as far as I can see).
Comment 10 Uri Simchoni 2017-01-26 06:21:13 UTC
(In reply to Jeremy Allison from comment #9)
Sure. Bad example. Should have used dashes or dots.
Comment 11 Jeremy Allison 2017-01-26 19:11:37 UTC
Created attachment 12850 [details]
git-am fix for master

w00t! This time I think I've nailed it Uri :-). I wouldn't have thought deeply enough about the problems without your help.

This version (in patches  [PATCH 15/18] and [PATCH 16/18]) refactors shadow_copy2_strip_snapshot() to actually make it comprehensible.

Changes from previous version:

[PATCH 14/18] adds a check to make sure shadow:format doesn't contain a /

[PATCH 15/18] adds a new function check_for_converted_path() that can correctly detect an already converted path.

[PATCH 16/18] mostly removes the crappy heuristics in shadow_copy2_strip_snapshot() that look for an already converted path, and makes it call check_for_converted_path() first, followed by the code to strip out any SMB @GMT-token part if the path isn't already converted.

Hopefully this is much clearer to review.

THANKS SO MUCH FOR YOUR HELP ON THIS ! :-).

Jeremy.
Comment 12 Uri Simchoni 2017-01-26 22:56:11 UTC
(In reply to Jeremy Allison from comment #11)
Wow! Suddenly I can comprehend this code even at 11PM! it's almost unbelievable that the code beforehand, with all the strict separation between "snapshotseverywhere" and non-snapshots-everywhere, did the same thing. I can't find "missing functionality". If Michael would review that refactoring too, it would be great.

That being said, I suspect there's still an issue with the patch set. When shadow_copy2_strip_snapshot_internal() returns a snappath instead of a stripped path, that path could be outside the share.

For example, if the share is at /vol/a/foo, and a snapshot has been taken of volume a and mounted at /snapshots/@GMT-whatever, then looking at /snapshots/@GMT-whatever/foo/bar, its connectpath needs to be /snapshots/@GMT-whatever/foo, not /snapshots/@GMT-whatever.

I imagine our tests don't cover the "already converted" case, and that's how they passed the previous patch. Without covering this, we don't really hit the snappath case. A comment in the pre-patch code said it happens with MxAc - I believe we need this comment back (unless it's incorrect)

About [2/16], and my comment:
> +				/* Go back one level... */
> +				/*
> +				 * Decrement d first as d points to
> +				 * the *next* char to write into.
> +				 */
> +				for (d--; d > destname; d--) {
> +					if (*d == '/') {
> +						break;
> +					}
> +				}
>
> If at the end of this loop we're back at destname, shouldn't we do *d++ = '/' ?

What if we have /foo/..? Doesn't it end with an empty string?

Beside those concerns all else LGTM.
Comment 13 Jeremy Allison 2017-01-26 23:35:13 UTC
(In reply to Uri Simchoni from comment #12)

> That being said, I suspect there's still an issue with the patch set. When 
> shadow_copy2_strip_snapshot_internal() returns a snappath instead of a stripped 
> path, that path could be outside the share.

Yes, that is the point. The snappath sets up the 'dummy' connectpath that allows check_path() to pretend the redirected pathname is still inside the share, even when it isn't.

> For example, if the share is at /vol/a/foo, and a snapshot has been taken of 
> volume a and mounted at /snapshots/@GMT-whatever, then looking at 
> snapshots/@GMT-whatever/foo/bar, its connectpath needs to be
> /snapshots/@GMT-whatever/foo, not /snapshots/@GMT-whatever.

No, I don't think so. The connectpath is set to  /snapshots/@GMT-whatever, the $cwd is /snapshots/@GMT-whatever/foo and the relative path being accessed is just bar.

Remember, we never get handed a path containing a snapshot until we've already been through the shadow2 VFS at least once (coming out from the REALPATH usually). Once we've gone through the realpath check we know we're already safely within the exported share path (or at least within the redirect into the snapshot area), and no one can create symlinks or links given a raw SMB @GMT-token pathname coming in from the client in this VFS module (the symlink and link functions refuse to work with pathnames containing timestamps).

> I imagine our tests don't cover the "already converted" case, and that's how
> they passed the previous patch. Without covering this, we don't really hit the
> snappath case. A comment in the pre-patch code said it happens with MxAc - I
> believe we need this comment back (unless it's incorrect)

Yeah the comment was incorrect. We hit the "already converted" case all the goddam time :-). That's why it took so long to write this patch, I kept hitting the test cases where smbd was reporting "path outside the share connectpath" inside check_path(), and had to invent the "cached connectpath" part of the code to fix it.

> What if we have /foo/..? Doesn't it end with an empty string?

F&^$%K&*** Damn. Yep, you're right. Updated patchset to follow... :-).

Jeremy.
Comment 14 Jeremy Allison 2017-01-26 23:59:59 UTC
(In reply to Jeremy Allison from comment #13)

> F&^$%K&*** Damn. Yep, you're right. Updated patchset to follow... :-).

I'm adding a local test for canonicalize_absolute_path(). I was just being lazy :-).
Comment 15 Jeremy Allison 2017-01-27 00:23:57 UTC
Created attachment 12851 [details]
git-am fix for master

Changes from the previous patch:

[PATCH 03/19] s3: lib: Fix two old, old bugs in set_conn_connectpath(), now in canonicalize_absolute_path().

-- now fixes the canonicalizing /foo/.. would end up as '\0' case as well.

[PATCH 04/19] s3: smbtorture: Add new local test  LOCAL-CANONICALIZE-PATH

-- Tests canonicalize_absolute_path() against the following test strings:

+       const char *src[] = {
+                       "/foo/..",
+                       "/..",
+                       "/foo/bar/../baz",
+                       "/foo/././",
+                       "/../foo",
+                       ".././././",
+                       ".././././../../../boo",
+                       "./..",
+                       NULL
+                       };
+       const char *dst[] = {
+                       "/",
+                       "/",
+                       "/foo/baz",
+                       "/foo",
+                       "/foo",
+                       "/",
+                       "/boo",
+                       "/",
+                       NULL
+                       };

All patches after this are the same, just moved down one.

Uri, if you +1 the review I'll ask Michael to give it a look also.

Thanks !

Jeremy.
Comment 16 Jeremy Allison 2017-01-27 01:28:10 UTC
Created attachment 12852 [details]
git-am fix for master.

Sigh. A few minor fixups.

Difference from previous patch:

1). Fix a few places where I missed setting timestamp variables to 0 in

[PATCH 06/20] s3: VFS: shadow_copy2: Correctly initialize timestamp and stripped variables.

(in the rename/link/symlink functions)

2). Add a final patch that ensures we also enforce read-only filesystem behavior on snapshot symlink/link/rename for already converted paths as well as @GMT-token SMB paths. Not sure if we can ever get to that case, but it doesn't hurt to ensure it's always so.

I'm going to stop updating now and let Uri finish the review :-).

Jeremy.
Comment 17 Uri Simchoni 2017-01-27 19:18:08 UTC
(In reply to Jeremy Allison from comment #16)
LGTM. *maybe* in [20/20], in the rename function, the error on dest being in a snapshot should be EROFS instead of EXDEV. Digging in history shows there's some value in raising EXDEV if the source is in a snapshot.

Thanks,
Uri.
Comment 18 Jeremy Allison 2017-01-27 19:25:39 UTC
Created attachment 12860 [details]
git-am fix for master.

Thanks - that EXDEV error was a cut-and-paste bug. Here is a new version with that fixed.

Only change from previous:

[PATCH 20/20] s3: VFS: Don't allow symlink, link or rename on already converted paths.

-- In rename function EXDEV -> EROFS if destination is a previously converted path.

I THINK WE'RE DONE !!!! Hurrah ! :-).

Jeremy.
Comment 19 Jeremy Allison 2017-01-27 19:58:20 UTC
Comment on attachment 12860 [details]
git-am fix for master.

Asking Michael for a glance-over, especially patch:

[PATCH 17/20] s3: VFS: shadow_copy2: Fix module to work with variable current working directory.

If Michael is too busy I will commit next week, but I'm hoping he has time.
Comment 20 Jeremy Allison 2017-01-31 17:19:31 UTC
Created attachment 12889 [details]
git-am fix for 4.6.next

Cherry-pick from master.
Comment 21 Jeremy Allison 2017-01-31 17:20:33 UTC
Created attachment 12890 [details]
git-am fix for 4.5.next

Back-port from master (mostly cherry-pick, only the local test patch needed modifying).
Comment 22 Jeremy Allison 2017-01-31 17:21:20 UTC
A back-port for 4.4.x is harder, due to divergence between the 4.4.x version of shadow_copy2 and the 4.5.x version. I'm looking at this right now.
Comment 23 Jeremy Allison 2017-01-31 18:29:15 UTC
Created attachment 12891 [details]
git-am fix for 4.4.next

This is a back-port of the 4.5.next patch to 4.4.next. Mostly change to cope with the fact that 4.4.x doesn't have the extra 'private data' structure, but sticks everything inside the config structure instead.

Note - the 'cherry-picked from' comments have been changed to 'backported from' so we can identify the original master commits these changes came from.
Comment 24 Uri Simchoni 2017-02-02 21:47:55 UTC
Comment on attachment 12891 [details]
git-am fix for 4.4.next

This is for the 4.4 patch:
[6/20] shadow_copy2_readlink() stripped is not initialized.
Comment 25 Jeremy Allison 2017-02-03 16:44:16 UTC
Created attachment 12904 [details]
git-am fix for 4.4.next

Fixed the problem with not initializing stripped == NULL in [6/20] noticed by Uri.

Thanks !
Comment 26 Uri Simchoni 2017-02-03 19:27:37 UTC
Comment on attachment 12904 [details]
git-am fix for 4.4.next

LGTM
Comment 27 Uri Simchoni 2017-02-03 19:31:22 UTC
Assigning to Karolin for inclusion in 4.4.next, 4.5.next, 4.6.next.

Thanks,
Uri.
Comment 28 Karolin Seeger 2017-02-13 15:42:26 UTC
(In reply to Uri Simchoni from comment #27)
Pushed to autobuild-v4-{6,5,4}-test.
Comment 29 Jeremy Allison 2017-02-15 00:34:54 UTC
Ping - I don't see this fix yet in the v4-4-test branch. Did that attachment get applied ?
Comment 30 Karolin Seeger 2017-02-15 07:51:33 UTC
(In reply to Jeremy Allison from comment #29)
Re-pushed to autobuild.
Comment 31 Karolin Seeger 2017-02-17 10:48:00 UTC
Pushed to all branches.
Closing out bug report.

Thanks!
Comment 32 Stefan Metzmacher 2017-03-27 08:14:14 UTC
There seems to be a regression see https://bugs.debian.org/858590
Comment 33 Uri Simchoni 2017-03-27 11:52:14 UTC
(In reply to Stefan Metzmacher from comment #32)
So far I haven't been able to reproduce on master, will try 4.2.
Comment 34 Uri Simchoni 2017-03-27 19:34:45 UTC
(In reply to Uri Simchoni from comment #33)
Reproduced with 4.2 fix.

FWIW, the shadow_copy2 module in 4.2 is different than the more recent versions with respect to wide-link checks, and the fix also included a pre-requisite patch to first bring it on par with the recent version.

I'll see what's the root cause in 4.2 to check whether or not it's in later versions.
Comment 35 Uri Simchoni 2017-03-28 05:53:25 UTC
(In reply to Uri Simchoni from comment #34)
Doesn't look like a regression, looks like a bug in the backport to 4.2.x. I posted a fix on bug 12496