Bug 10297 - REGRESSION: Writing to a directory with -wx permissions on a share fails with ACCESS_DENIED
Summary: REGRESSION: Writing to a directory with -wx permissions on a share fails with...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.1.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-28 15:05 UTC by Andreas Schneider
Modified: 2016-11-01 07:53 UTC (History)
6 users (show)

See Also:


Attachments
Patch for master (1.50 KB, patch)
2013-11-28 15:19 UTC, Andreas Schneider
jra: review-
Details
git-am fix for master. (7.90 KB, patch)
2013-12-07 00:07 UTC, Jeremy Allison
no flags Details
git-am fix for 4.1.next and 4.0.next (8.38 KB, patch)
2013-12-09 21:18 UTC, Jeremy Allison
asn: review+
Details
Possible regression patch for master (9.64 KB, text/plain)
2016-10-22 08:07 UTC, Stefan Metzmacher
no flags Details
git-am fix for 4.5.next, 4.4.next (9.89 KB, patch)
2016-10-25 18:45 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2013-11-28 15:05:51 UTC
If you create a share

[test_733]
   /srv/samba/test_733
   writeable = yes

As root:
mkdir -p /srv/samba/test_733/upload
chmod 0733 /srv/samba/test_733/upload


smbclient //localhost/test_733 -Ubob1%secret
$ cd upload
$ put myfile
NT_STATUS_ACCESS_DENIED opening remote file \upload\myfile


I've did a 'git bisect' and it identified:

commit 0150086d44e90351634a68aced1e44ad076a693c
Author:     Volker Lendecke <Volker.Lendecke@SerNet.DE>
AuthorDate: Wed Aug 28 15:42:22 2013 -0700
Commit:     Karolin Seeger <kseeger@samba.org>
CommitDate: Fri Aug 30 10:07:24 2013 +0200

    smbd: Simplify dropbox special case in unix_convert


I think that the patch is probably correct but the code around it passing the right information and handling the return codes is probably broken.

I think the first fix is:

diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 0585a6e..6a3f016 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -1938,7 +1938,8 @@ void reply_open_and_X(struct smb_request *req)
                                conn,
                                req->flags2 & FLAGS2_DFS_PATHNAMES,
                                fname,
-                               (create_disposition == FILE_CREATE)
+                               (create_disposition == FILE_CREATE ||
+                                create_disposition == FILE_OVERWRITE_IF)
                                        ? UCF_CREATING_FILE : 0,
                                NULL,
                                &smb_fname);

But then smbclient fails with NT_STATUS_OBJECT_PATH_NOT_FOUND and the file isn't uploaded.
Comment 1 Andreas Schneider 2013-11-28 15:19:32 UTC
Created attachment 9491 [details]
Patch for master

Here is a proposed patch to handle the return value in this special case gracefully. I'm not so deep into the smbd file handling business so the patch might be wrong :)
Comment 2 Jeremy Allison 2013-12-04 18:42:11 UTC
Comment on attachment 9491 [details]
Patch for master

NAK for reasons discussed on samba-technical.

Let's see what Volker thinks of my alternate patch.
Comment 3 Jeremy Allison 2013-12-07 00:07:04 UTC
Created attachment 9515 [details]
git-am fix for master.

Patch I'm proposing for master (so we don't lose it in my inbox :-).

Jeremy.
Comment 4 Andreas Schneider 2013-12-07 14:49:34 UTC
Thanks. I wonder if it we should add tests which are only called if we are really root. Run only with socket_wrapper and without nss_wrapper and uid_wrapper. So we could test things like that. It would be enough to have them as a nightly build somewhere.

The issue has been found by Red Hat QA doing regression tests.
Comment 5 Jeremy Allison 2013-12-09 21:18:16 UTC
Created attachment 9519 [details]
git-am fix for 4.1.next and 4.0.next

Fix that applies cleanly to 4.1.next and 4.0.next, contains cherry-pick info from master.

Jeremy.
Comment 6 Andreas Schneider 2013-12-10 07:29:34 UTC
Comment on attachment 9519 [details]
git-am fix for 4.1.next and 4.0.next

LGTM
Comment 7 Andreas Schneider 2013-12-10 07:49:36 UTC
Karolin, please add the patchset to the relevant branches. Thanks!u
Comment 8 Karolin Seeger 2013-12-10 11:56:33 UTC
Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 9 Karolin Seeger 2013-12-10 15:21:51 UTC
Pushed to v4-0-test and v4-1-test.
Closing out bug report.

Thanks!
Comment 10 Yannick Bergeron (mail address dead) 2014-02-21 15:58:44 UTC
Hi,

We believe this also affect Samba 3.6.22 and the commit causing the issue would be: 0150086d44e90351634a68aced1e44ad076a693c

We'll try backporting this patch for 3.6 and see if it fix our issue.


Best regards,

Yannick Bergeron
Comment 11 Yannick Bergeron (mail address dead) 2014-02-21 17:48:19 UTC
Hi,

just to let you know that it seems to fix the issue in 3.6.22 also.

Best regards,

Yannick Bergeron
Comment 12 Michael Tokarev 2014-12-04 15:54:12 UTC
This change breaks another "interesting" access mode to existing files, and completely breaks roaming profiles in windows7 environment.

When a windows7 domain user logs in and windows syncronizes the profile from server to local, it does all the things necessary, but at some point, it tries to access Profile.V2/ntuser.ini AS DIFFERENT USER (usually but not necessary machine account).  I don't kno why it does that, but it does that quite consistently.

So, before this change, samba returned equivalent of PERMISSION DENIED to windows client, and it happily retried accessing this file using the right userid.

But after this change, samba returns FILE NOT FOUND instead, and windows "thinks" the profile disappeared and creates a temporary local profile instead.

strace of smbd shows it tries to open Profile.V2/ntuser.ini with wrong uid, gets EACCESS, tries to open Profile.V2/ directory for read, also gets EACCESS, next tries ntuser.ini again, receiving EACCESS again ofcourse, and returns FILE NOT FOUND to the client.  This is obviously wrong, it should return PERMISSION DENIED instead as it did before.

I tried to understand the logic in this place, but it is quite a bit twisted, so I don't really understand how to fix it properly.

Basically, it should not return FILE_NOT_FOUND when asked to open a file in an unaccessible directory.

Thanks,

/mjt
Comment 13 Michael Tokarev 2014-12-04 16:01:51 UTC
..it tries to _stat_ ntuser.ini, not open it -- typo in the previous comment.
Comment 14 Michael Tokarev 2014-12-04 18:14:40 UTC
Here's the level-5 debug trace of the event, starting from the switching of the security context.

[2014/12/04 19:07:13.409261,  4] ../source3/smbd/sec_ctx.c:316(set_sec_ctx)
  setting sec ctx (4021, 2000) - sec_ctx_stack_ndx = 0
[2014/12/04 19:07:13.409314,  5] ../libcli/security/security_token.c:63(security_token_debug)
  Security token SIDs (7):
    SID[  0]: S-1-5-21-411424318-379842365-2075518510-1002
    SID[  1]: S-1-5-21-411424318-379842365-2075518510-515
    SID[  2]: S-1-1-0
    SID[  3]: S-1-5-2
    SID[  4]: S-1-5-11
    SID[  5]: S-1-22-1-4021
    SID[  6]: S-1-22-2-2000
   Privileges (0x               0):
   Rights (0x               0):
[2014/12/04 19:07:13.409393,  5] ../source3/auth/token_util.c:629(debug_unix_user_token)
  UNIX token of user 4021
  Primary group is 2000 and contains 1 supplementary groups
  Group[  0]: 2000
[2014/12/04 19:07:13.409424,  5] ../source3/smbd/uid.c:363(change_to_user_internal)
  Impersonated user: uid=(4021,4021), gid=(0,2000)
[2014/12/04 19:07:13.409450,  4] ../source3/smbd/vfs.c:838(vfs_ChDir)
  vfs_ChDir to /home/mjt
[2014/12/04 19:07:13.409482,  4] ../source3/smbd/vfs.c:849(vfs_ChDir)
  vfs_ChDir got /home/mjt
[2014/12/04 19:07:13.409533,  5] ../source3/smbd/filename.c:258(unix_convert)
  unix_convert called on file "Profile.V2/ntuser.ini"
[2014/12/04 19:07:13.409560,  5] ../source3/smbd/filename.c:421(unix_convert)
  unix_convert begin: name = Profile.V2/ntuser.ini, dirpath = , start = Profile.V2/ntuser.ini
[2014/12/04 19:07:13.409585,  5] ../source3/smbd/statcache.c:143(stat_cache_add)
  stat_cache_add: Added entry (7f311a78aef0:size a) PROFILE.V2 -> Profile.V2
[2014/12/04 19:07:13.409619,  5] ../source3/smbd/dir.c:1577(OpenDir)
  OpenDir: Can't open Profile.V2. Permission denied
[2014/12/04 19:07:13.409745,  3] ../source3/smbd/filename.c:1150(get_real_filename_full_scan)
  scan dir didn't open dir [Profile.V2]
[2014/12/04 19:07:13.409763,  5] ../source3/smbd/filename.c:816(unix_convert)
  New file ntuser.ini
[2014/12/04 19:07:13.409791,  5] ../lib/dbwrap/dbwrap.c:187(dbwrap_check_lock_order)
  check lock order 1 for /var/run/samba/smbXsrv_open_global.tdb
[2014/12/04 19:07:13.409838,  5] ../lib/dbwrap/dbwrap.c:146(dbwrap_lock_order_state_destructor)
  release lock order 1 for /var/run/samba/smbXsrv_open_global.tdb
[2014/12/04 19:07:13.409860,  5] ../source3/smbd/files.c:128(file_new)
  allocated file structure fnum 1240946723 (1 used)
[2014/12/04 19:07:13.409895,  3] ../source3/smbd/dosmode.c:163(unix_mode)
  unix_mode(Profile.V2/ntuser.ini) returning 0664
[2014/12/04 19:07:13.409920,  5] ../source3/smbd/open.c:2168(open_file_ntcreate)
  open_file_ntcreate: FILE_OPEN requested for file Profile.V2/ntuser.ini and file doesn't exist.
[2014/12/04 19:07:13.409939,  5] ../lib/dbwrap/dbwrap.c:187(dbwrap_check_lock_order)
  check lock order 1 for /var/run/samba/smbXsrv_open_global.tdb
[2014/12/04 19:07:13.409962,  5] ../lib/dbwrap/dbwrap.c:146(dbwrap_lock_order_state_destructor)
  release lock order 1 for /var/run/samba/smbXsrv_open_global.tdb
[2014/12/04 19:07:13.409983,  5] ../source3/smbd/files.c:528(file_free)
  freed files structure 1240946723 (0 used)
[2014/12/04 19:07:13.410652,  4] ../source3/smbd/uid.c:384(change_to_user)
  Skipping user change - already user
[2014/12/04 19:07:13.410720,  5] ../source3/smbd/filename.c:258(unix_convert)
  unix_convert called on file "Profile.V2"
...

When returning from get_real_filename() in unix_convert(), errno is EACCESS, and ucf_flags == UCF_PREP_CREATEFILE.  Windows uses CreateFile() with READ open flag (not create flag).

For now, to let our users to work, I just commented out the check for UCF_PREP_CREATEFILE in there, because for now we don't use dropbox dirs.  But ofcourse it'd be nice to fix it for real, especially since this prob makes roaming profiles to stop working in a bad and difficult-to-debug way.

Thanks,

/mjt
Comment 15 Matthias Dieter Wallnöfer 2014-12-05 09:05:16 UTC
Interesting issue, I reopen it for further investigations.
Lately I have seen some corrupted roaming profiles on Windows XP too, so it might be the same bug.
Comment 16 Karolin Seeger 2014-12-20 15:54:13 UTC
Re-assigning to Jeremy.
Comment 17 Jeremy Allison 2014-12-20 23:44:22 UTC
(In reply to Michael Tokarev from comment #14)

Can you get me a wireshark trace of the client activity showing this bug ?

I need to have this before being able to fix this bug, as I need to have a good regression test to make sure we don't break anything else when fixing this.

Thanks,

Jeremy.
Comment 18 Michael Tokarev 2014-12-22 16:15:25 UTC
I don't have an easy way to reproduce it right now.  The thing is: we have just one domain, and I don't really want to break user logins again while re-introducing the issue.  So for me, this means setting up a separate samba domain and joining a separate windows machine to it.

I already provided some logs/traces (not on-wire.  Please note that as far as I can see, it uses some sort of encryption, at least tcpdump can't dissect the protocol easily, it just shows "data exchange" without other details).

I have logs from procmon on windows too, where I observed that windows mpersonates current user with machine account somewhere in the middle of profile syncronization, and tries to access ntuser.ini file.  When the server returns ACCESS_DENIED, it switches back to the rigth user and retries.  But when server returns FILE_NOT_FOUND, windows aborts profile sync.  I don't know _why_ windows does this impersonating in the first place.  But it does this, and this is an old way of doing things - windows vista, windows 7 and windows 8.1 does this. And after coming across this issue I verified other networks - I see that it is not something specific to our setup, other networks are also does this.

But at any rate, the problem should be easy to see, and easy to understand what samba does wrong.  It does this wrong in context of this dropbox config and in context of 

What we currently have is this: if accessing dir/file directly fails with EACCESS, we're trying read the directory.  If that fails with EACCESS _too_, we return FILE_NOT_FOUND.  This logic is wrong, but we're at the edge here.

From first, naive thought, we should return ACCESS_DENIED right away.  The problem, as far as I understand it, is case (in)sensitivity -- there might be a file with the same name in that dir which is accessible, but which differs only by the case (upper/lower) of the one which was requested.

But at any rate, we can not return FILE_NOT_FOUND if both dir/file and dir access returns EACCESS.  We can't be sure it does not exist.  And my example shows that it actually DOES exist.

So, I can try to set up a new samba+windows network to get a trace.  But at the very least I need to find some time for this, and need some instructions on how to get the good trace, because obviously encrypted packets are of little help.

Thanks,

/mjt
Comment 19 Jeremy Allison 2014-12-22 17:13:19 UTC
(In reply to Michael Tokarev from comment #18)

This is why I need to see a wireshark trace of it trying to access the file as
the different user. Note you don't need to break your changed version of Samba, I'm happy seeing the ACCESS_DENIED version so long as I can see exactly what open requests the Windows client does as the impersonated user that should return ACCESS_DENIED.

The problem is I need to write a test that keeps the current dropbox functionality working, whist catching the Windows impersonate error requirements.

In order to do that I need a wireshark trace showing what Windows does so I can emulate it in the test.

Thanks !

Jeremy.
Comment 20 Jeremy Allison 2014-12-22 17:14:50 UTC
To be clear - what I need to see are the packets from Windows that do (as you refer to it):

"if accessing dir/file directly fails with EACCESS, we're trying read the directory."

Exactly *how* is it trying to read the directory ? What Windows client request is driving this ?
Comment 21 Michael Tokarev 2014-12-22 18:08:24 UTC
(In reply to Jeremy Allison from comment #20)

Oh.  You misunderstood.

It is _samba_ who tries to read directory, not windows.  Windows just requests to open one file, Profile.V2/ntuser.ini.  Samba tries to open it, gets EACCESS, tries to open _directory_, gets EACCESS again, and returns FILE_NOT_FOUND to that single CreateFile() windows call.

On windows side it is just a single CreateFile.

/mjt
Comment 22 Jeremy Allison 2014-12-22 18:09:42 UTC
(In reply to Michael Tokarev from comment #21)

No, I know it's just a single Windows CreateFile :-). I need to see the parameters to that CreateFile on the wire.
Comment 23 Michael Tokarev 2014-12-22 18:19:46 UTC
Here's the syscall in question as captured by Procmon:

Process:        C:\Windows\system32\svchost.exe -k netsvc
Operation:      CreateFile
Result:         ACCESS DENIED
Path:           \\fs\mjt\Profile.V2\ntuser.ini
Desired Access:	Generic Read
Disposition:	Open
Options:	Synchronous IO Non-Alert, Non-Directory File
Attributes:	n/a
ShareMode:	Read, Write, Delete
AllocationSize:	n/a
Impersonating:	NT AUTHORITY\system

This one works with smbd patched by me as shown in comment #14.  The same result is returned by samba3.  Unpatched (current 4.1) returns FILE_NOT_FOUND here
instead of ACCESS_DENIED, and windows fails to syncronize the profile.

With ACCESS_DENIED result returned by samba, the next operation done by windows is the same CreateFile but Impersonating: TLS\mjt (the right user), and the result is, ofcourse, SUCCESS, and profile sync continues.  With FILE_NOT_FOUND returned by 4.1, windows aborts profile sync, removes whatever it already synced, and creates a new, temporary user profile.
Comment 24 Jeremy Allison 2014-12-22 20:14:10 UTC
(In reply to Michael Tokarev from comment #23)

Can you give me the actual POSIX permissions on the directory tree:

\\fs\mjt\Profile.V2\ntuser.ini

all the way down please ? I'm trying to reproduce this scenario with a dropbox directory (set -rx) and always get ACCESS_DENIED, not FILE_NOT_FOUND.

Also a debug level 10 from smbd of a request to open this file with:

Desired Access:	Generic Read
Disposition:	Open
Options:	Synchronous IO Non-Alert, Non-Directory File
Attributes:	n/a
ShareMode:	Read, Write, Delete

and getting NOT_FOUND returned would really help. I need to see how to reproduce this.
Comment 25 Michael Tokarev 2014-12-23 14:18:23 UTC
Here's the file permissions.

$ ls -ld / /home /home/mjt /home/mjt/Profile.V2 /home/mjt/Profile.V2/ntuser.ini 
drwxr-xr-x 24 root root 4096 Dec 24  2013 /
drwxr-xr-x 45 root root 4096 Dec  8 13:35 /home
drwxr-xr-x 74 mjt  mjt  4096 Dec 23 16:43 /home/mjt
drwx------ 16 mjt  mjt  4096 Dec 23 13:20 /home/mjt/Profile.V2
-rw-rw-r--  1 mjt  mjt   292 Dec 23 13:20 /home/mjt/Profile.V2/ntuser.ini

There's no ACLs, EAs or other fancy stuff.  The permissions are quite standard as created by windows7 client on a non-ACL-mounted filesystem.

Speaking of NOT_FOUND, -- I'll play with it a bit later today, when our users will go home.  Will restore samba 4.1 and send whatever traces needed.

Thanks,

/mjt
Comment 26 Michael Tokarev 2014-12-23 14:47:17 UTC
I created a level10 debug dump of the operation.

http://www.corpit.ru/mjt/tmp/smbd.bug10297.log.xz -- this is the complete trace, from the beginning up to the point where windows client loaded and told me that my profile is unaccessible.

The call in question is at timestamp "2014/12/23 17:39:48.889493" and below.  Note the userid we're impersonating as -- mjt userid is 1000:1000, while machine account is 4030:2000.

And I think the final decision is at "2014/12/23 17:39:48.889988", where it says ntuser.ini is a "new file".

Thanks,

/mjt
Comment 27 Jeremy Allison 2014-12-23 17:00:09 UTC
Ah, the key is the directory having no public access I think. Thanks for the info. I should be able to reproduce from here (I hope).
Comment 28 Jeremy Allison 2014-12-23 19:59:22 UTC
(In reply to Michael Tokarev from comment #26)

What are your smb.conf settings ?

What I'm finding when trying to reproduce accessing a file 'look' in a no access directory 'noaccess', is that the filename_convert fails to open the directory 'noaccess', and gets "New file look", but then goes into check_name().

Inside check_name() calls check_reduced_name() and in there the SMB_VFS_REALPATH() call fails with EACESS, and so the whole SMB2 create call returns NT_STATUS_ACCESS_DENIED.

However, check_reduced_name() is only called inside check_name() if:

        if (!lp_widelinks(SNUM(conn)) || !lp_follow_symlinks(SNUM(conn))) {

What are your smb.conf settings for widelinks or follow symlinks ?

Log from my local box is below:

  OpenDir: Can't open noaccess. Permission denied
[2014/12/23 11:05:26.656900,  3, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/filename.c:1150(get_real_filename_full_scan)
  scan dir didn't open dir [noaccess]
[2014/12/23 11:05:26.656916, 10, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/mangle_hash2.c:418(is_mangled)
  is_mangled look ?
[2014/12/23 11:05:26.656927, 10, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/mangle_hash2.c:357(is_mangled_component)
  is_mangled_component look (len 4) ?
[2014/12/23 11:05:26.656939,  5, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/filename.c:816(unix_convert)
  New file look
[2014/12/23 11:05:26.656954,  3, pid=13720, effective(120, 131), real(120, 0), class=vfs] ../source3/smbd/vfs.c:1143(check_reduced_name)
  check_reduced_name [noaccess/look] [/tmp/dropbox]
[2014/12/23 11:05:26.656971,  3, pid=13720, effective(120, 131), real(120, 0), class=vfs] ../source3/smbd/vfs.c:1197(check_reduced_name)
  check_reduced_name: couldn't get realpath for noaccess/look
[2014/12/23 11:05:26.656987,  5, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/filename.c:1050(check_name)
  check_name: name noaccess/look failed with NT_STATUS_ACCESS_DENIED
[2014/12/23 11:05:26.656999,  3, pid=13720, effective(120, 131), real(120, 0)] ../source3/smbd/filename.c:1404(filename_convert_internal)
  filename_convert_internal: check_name failed for name noaccess/look with NT_STATUS_ACCESS_DENIED
Comment 29 Michael Tokarev 2014-12-24 09:30:51 UTC
(In reply to Jeremy Allison from comment #28)

Heh.  Indeed, we have these symlink-related options:

 unix extensions = no
 wide links = yes

because for a long time we used symlinks to other parts of the system inside shares, and because we only have windows clients..

It is a fun combination of options which triggers this issue.  Oh well.

Thanks,

/mjt
Comment 30 Jeremy Allison 2014-12-24 20:26:06 UTC
So if you set "wide links = no" this should work for you.

I'll take a look at how to fix this in the "file not found"case inside filename_convert without having to depend on check_path() as a backstop, but in the meantime the default is "wide lins = no" so I think this just lost urgency :-).
Comment 31 Michael Tokarev 2014-12-25 05:06:10 UTC
(In reply to Jeremy Allison from comment #30)

Since I fixed this prob locally by patching out "wrong" test, it is not urgent for us anymore.  I just should not forget to do the same with any new version I'll install.

However, the whole thing is quite a bit twisted, with at least a fragile logic, and should be fixed before some other "interesting" case like this will be found... ;)

Thanks,

/mjt
Comment 32 Yannick Bergeron (mail address dead) 2015-01-05 14:00:29 UTC
Indeed we also have:
        unix extensions = No
        wide links = Yes

But cannot change them because it would break our environment.
Comment 33 Stefan Metzmacher 2016-10-22 08:07:17 UTC
Created attachment 12597 [details]
Possible regression patch for master
Comment 34 Jeremy Allison 2016-10-25 18:45:49 UTC
Created attachment 12599 [details]
git-am fix for 4.5.next, 4.4.next

Cherry-picked from master - applies cleanly to 4.5.next, 4.4.next.
Comment 35 Karolin Seeger 2016-10-31 10:19:31 UTC
(In reply to Jeremy Allison from comment #34)
Pushed to autobuild-v4-{5,4}-test.
Comment 36 Karolin Seeger 2016-11-01 07:53:37 UTC
(In reply to Karolin Seeger from comment #35)
Pushed to both branches.
Closing out bug report.

Thanks!