Bug 8370 - vfs_chown_fsp broken -- returns in the wrong directory
vfs_chown_fsp broken -- returns in the wrong directory
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.0
All All
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8399
  Show dependency treegraph
 
Reported: 2011-08-11 10:40 UTC by Volker Lendecke
Modified: 2011-08-25 16:35 UTC (History)
1 user (show)

See Also:


Attachments
Patch (948 bytes, patch)
2011-08-11 10:44 UTC, Volker Lendecke
jra: review-
ambi: review+
Details
git-am fix for 3.6.0 (725 bytes, patch)
2011-08-11 16:28 UTC, Jeremy Allison
no flags Details
regular patch for both 3.6.0 and 3.5.12 (1.51 KB, patch)
2011-08-11 19:36 UTC, Jeremy Allison
no flags Details
Final (?) version of this patch for 3.6.0 (2.34 KB, patch)
2011-08-11 21:44 UTC, Jeremy Allison
vl: review+
ambi: review+
jra: review? (metze)
Details
Back-port for 3.5.12. (1.11 KB, patch)
2011-08-11 22:07 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Volker Lendecke 2011-08-11 10:40:52 UTC

    
Comment 1 Volker Lendecke 2011-08-11 10:44:25 UTC
Created attachment 6772 [details]
Patch
Comment 2 Volker Lendecke 2011-08-11 10:44:50 UTC
Is this a security problem?
Comment 3 Christian Ambach 2011-08-11 10:46:38 UTC
Comment on attachment 6772 [details]
Patch

I have seen robocopy failing to copy a directory structure including permissions and ownerships and Explorer listing the contents of a subdirectory as share root.

With Volker's patch, robocopy suceeds
Comment 4 Jeremy Allison 2011-08-11 16:19:48 UTC
(In reply to comment #2)
> Is this a security problem?

Not sure. What the hell is that cache doing in vfs_ChDir() ?
Comment 5 Jeremy Allison 2011-08-11 16:28:12 UTC
If bites if "dos filemode = true", but that's not true by default. Also if SeBackup/SeRestore are set, but again they're not set by default.

FYI. The same bug is present in modules/acl_common.c:acl_common_remove_object()

I think the correct fix is just to remove that cache in vfs_ChDir() and make it equivalent to SMB_VFS_CHDIR() - we can then change all calls from vfs_ChDir() directly to SMB_VFS_CHDIR at our leisure.

This also hits 3.5.x as well I think.

It's my fault as I should have checked in internals of vfs_ChDir().

Jeremy.
Comment 6 Jeremy Allison 2011-08-11 16:28:49 UTC
Created attachment 6776 [details]
git-am fix for 3.6.0

How about this one ?
Comment 7 Jeremy Allison 2011-08-11 16:30:06 UTC
Comment on attachment 6772 [details]
Patch

This bug hits other places too.. I think vfs_ChDir needs to be changed to SMB_VFS_CHDIR().
Comment 8 Jeremy Allison 2011-08-11 17:06:23 UTC
Comment on attachment 6776 [details]
git-am fix for 3.6.0

This patch also applies to 3.5.x (and is needed there too).

Jeremy.
Comment 9 Jeremy Allison 2011-08-11 19:34:58 UTC
Ok, been thinking about this - it's not a security hole. The become_root()/unbecome_root() pairs are correctly matched up, so it never does anything with greater than allowed privilege. It's just a plain bug - in that if the chdir() isn't done back to the service directory, then if the next packet is also on that tid (which is likely), then the action will be done from the wrong directory - but that will be under the original service directory path.

My fix removes the singleton cache that vfs_ChDir() uses to prevent doing a chdir() if subsequent packets come in on the same tid - but to be honest I think that is probably a premature optimization anyway (although it will add one more syscall to each packet processing).

Volker, Metze and Christian - what do you think - kill the singleton cache or fix all cases where we do SMB_VFS_CHDIR() within a call to always return back to the connect path (there are more than one of these) ?

Either way it's annoying, but can wait until 3.6.1 (and 3.5.12) as it's been in since 3.5.9 I think.

Jeremy.
Comment 10 Jeremy Allison 2011-08-11 19:36:46 UTC
Created attachment 6777 [details]
regular patch for both 3.6.0 and 3.5.12

Adds removal of LastDir from globals as well as removal of the singleton cache from vfs_ChDir().

Jeremy.
Comment 11 Jeremy Allison 2011-08-11 21:44:49 UTC
Created attachment 6778 [details]
Final (?) version of this patch for 3.6.0

Sorry for all the fixes, but I'm trying to get everything in order on this bug before I go completely offline for 6 days on Saturday 13th August (in a cottage with no internet access until I get home on the 18th).

This is a version for 3.6 that (hopefully) will fix the problem. It does things slightly differently from Volker's original patch.

1). It changes the fchown code to us vfs_ChDir() instead of SMB_VFS_CHDIR(). If we're going to have that singleton cache to prevent syscalls on every incoming packet we need to use it consistently and not call SMB_VFS_CHDIR directly.

2). It fixes the same issue in the modules/vfs_acl_common.c code - I also ensured we got the current directory we need to change back to instead of just assuming it was changing back to the connection path. These modules can be stacked so who knows what directories a module above us in the chain may have switched to - make sure we switch back.

3). It fixes error conditions where we could return out of the code after the vfs_ChDir() was called without going back to the original directory (this is in the fchown code). These are identical errors waiting to happen.

I will post a follow-up patch for 3.5.12 once I've back ported it.

Christian and Volker, please test and review and let me know what you think before Saturday morning UK time :-).

Cheers,

Jeremy.
Comment 12 Jeremy Allison 2011-08-11 22:07:08 UTC
Created attachment 6779 [details]
Back-port for 3.5.12.

Same fixes for modules/vfs_acl_common.c. The changes to smbd/vfs.c don't apply as 3.5.x doesn't do the chmod to pin the parent directory for root chowns.

Jeremy.
Comment 13 Christian Ambach 2011-08-12 14:31:11 UTC
Comment on attachment 6778 [details]
Final (?) version of this patch for 3.6.0

run the robocopy testrun again with and w/o patch

w/o it fails, with patch it goes through

so patch looks good to me
Comment 14 Jeremy Allison 2011-08-18 19:28:08 UTC
Ping vl - can I get a review of this one please ? I'd like to get it into 3.6.1 and 3.5.12.

Raising to blocker to make sure we don't release without a resolution to this.

Cheers,

Jeremy.
Comment 15 Jeremy Allison 2011-08-19 16:27:52 UTC
Not a security issue - removing team-only restriction.

Jeremy.
Comment 16 Karolin Seeger 2011-08-20 18:51:03 UTC
(In reply to comment #11)
> Created attachment 6778 [details]
> Final (?) version of this patch for 3.6.0
> 
> Sorry for all the fixes, but I'm trying to get everything in order on this bug
> before I go completely offline for 6 days on Saturday 13th August (in a cottage
> with no internet access until I get home on the 18th).
> 
> This is a version for 3.6 that (hopefully) will fix the problem. It does things
> slightly differently from Volker's original patch.
> 
> 1). It changes the fchown code to us vfs_ChDir() instead of SMB_VFS_CHDIR(). If
> we're going to have that singleton cache to prevent syscalls on every incoming
> packet we need to use it consistently and not call SMB_VFS_CHDIR directly.
> 
> 2). It fixes the same issue in the modules/vfs_acl_common.c code - I also
> ensured we got the current directory we need to change back to instead of just
> assuming it was changing back to the connection path. These modules can be
> stacked so who knows what directories a module above us in the chain may have
> switched to - make sure we switch back.
> 
> 3). It fixes error conditions where we could return out of the code after the
> vfs_ChDir() was called without going back to the original directory (this is in
> the fchown code). These are identical errors waiting to happen.
> 
> I will post a follow-up patch for 3.5.12 once I've back ported it.
> 
> Christian and Volker, please test and review and let me know what you think
> before Saturday morning UK time :-).
> 
> Cheers,
> 
> Jeremy.

Pushed to v3-6-test.
Comment 17 Karolin Seeger 2011-08-20 18:51:20 UTC
(In reply to comment #12)
> Created attachment 6779 [details]
> Back-port for 3.5.12.
> 
> Same fixes for modules/vfs_acl_common.c. The changes to smbd/vfs.c don't apply
> as 3.5.x doesn't do the chmod to pin the parent directory for root chowns.
> 
> Jeremy.

Pushed to v3-5-test.
Comment 18 Karolin Seeger 2011-08-20 18:51:40 UTC
Closing out bug report.

Thanks!