Bug 14279 - REGRESSION: We can't change to a dropbox directory with mode 733 and drop a file
Summary: REGRESSION: We can't change to a dropbox directory with mode 733 and drop a file
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.11.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on:
Reported: 2020-02-13 16:17 UTC by Andreas Schneider
Modified: 2020-02-20 00:29 UTC (History)
2 users (show)

See Also:

A 'dropbox' test (4.79 KB, patch)
2020-02-13 16:29 UTC, Andreas Schneider
no flags Details
Add improved dropbox test (4.76 KB, patch)
2020-02-13 16:37 UTC, Andreas Schneider
no flags Details
dropbox test with 'put wurst dirmode733/wurst' (9.98 KB, patch)
2020-02-14 16:53 UTC, Andreas Schneider
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2020-02-13 16:17:51 UTC
mkdir -p /srv/samba/dropbox/dirmode733
chmod 0733 /srv/samba/dropbox/dirmode733


    comment = dropbox
    path = /srv/samba/dropbox

$ bin/smbclient // -Uasn%secret
smb: \> cd dirmode733\
cd \dirmode733\: NT_STATUS_ACCESS_DENIED

The issue is that since 4.11 we are doing the following:

open_directory(".", O_RDONLY|O_DIRECTORY, 0);

which does: fd = open(".", O_RDONLY|O_DIRECTORY, 0);

and we get 'Permission denied'.

Reverting cd66614a4e8a8f93e1debb8a87dd2afd46b02990 allows me to enter the directory.

Sadly Linux doesn't offer a O_SEARCH and the only way to fix this is probably openat() and fchdir().

If I revert cd66614a4e8a8f93e1debb8a87dd2afd46b02990, then we have the next problem:

smb: \dirmode733\> put wurst
NT_STATUS_ACCESS_DENIED opening remote file \dirmode733\wurst
Comment 1 Andreas Schneider 2020-02-13 16:18:54 UTC
asn is 'other' in case of 0733, so falling into the 3 case. /srv/samba/dropbox/dirmode733 is owned by root.
Comment 2 Andreas Schneider 2020-02-13 16:29:49 UTC
Created attachment 15788 [details]
A 'dropbox' test

Here is a reproducer.
Comment 3 Andreas Schneider 2020-02-13 16:37:54 UTC
Created attachment 15789 [details]
Add improved dropbox test
Comment 4 Jeremy Allison 2020-02-13 16:42:30 UTC
We can't revert cd66614a4e8a8f93e1debb8a87dd2afd46b02990. It's part of the move to always use file handles everywhere in the VFS.

We may have to abandon this functionality, it's not safe to use.
Comment 5 Jeremy Allison 2020-02-13 16:46:24 UTC
Thinking more about this, use of Samba as a dropbox into a directory we can't read is counter to good security practice.

Right now I don't see a way to fix this and still maintain secure access. Andreas, can you think of any ways this could work given we must have fd access to all directories we operate within ?

I'm close to "WILLNOTFIX"..
Comment 6 Andreas Schneider 2020-02-13 16:48:30 UTC
Please don't close it as WONTFIX for now. If we do not support this anymore we probably should write that down in WHATSNEW.txt at least for 4.12.
Comment 7 Jeremy Allison 2020-02-13 17:12:42 UTC
Sure, I *didn't* close it :-). I just don't currently see how this can be fixed.
Comment 8 Ralph Böhme 2020-02-13 17:15:33 UTC
Jeremy, wouldn't the O_PATH we were discussing elsewhere also allow this to work?
Comment 9 Jeremy Allison 2020-02-13 17:28:06 UTC
No, O_PATH doesn't allow reading directory contents.
Comment 10 Jeremy Allison 2020-02-13 17:31:02 UTC
I need to investigate closer, but after a conversation with Volker the issue is the difference between detecting if a path is valid (for which O_PATH would work) and opening a directory to read the contents (for which O_PATH doesn't, we need O_RDONLY|O_DIRECTORY).

Volker may be correct in that the directory open with read is being done in order to try and do case insensitive matching in the pathname lookup.

In which case, for the dropbox case we simply have to ignore case insensitive name matching and always use the name as given by the client.
Comment 11 Ralph Böhme 2020-02-13 17:32:44 UTC
(In reply to Jeremy Allison from comment #9)
we're not interested in listing directory content here. All we need is an fd referring to the parent dir, so we can fchdir() to dir.

This is basically the same problem brought up by Ralf Würthner.
Comment 12 Jeremy Allison 2020-02-13 17:35:02 UTC
I need to investigate, but if the ACCESS_DENIED error is coming from the "get_real_filename()" codepath inside source3/smbd/filename.c we may be able to fix this by deliberately going into the code here:

1026                                 /*
1027                                  * ENOENT/EACCESS are the only valid errors
1028                                  * here.
1029                                  */
1031                                 if (errno == EACCES) {
1032                                         if ((ucf_flags & UCF_PREP_CREATEFILE) == 0) {
1033                                                 status = NT_STATUS_ACCESS_DENIED;
1034                                                 goto fail;
1035                                         } else {
1036                                                 /*
1037                                                  * This is the dropbox
1038                                                  * behaviour. A dropbox is a
1039                                                  * directory with only -wx
1040                                                  * permissions, so
1041                                                  * get_real_filename fails
1042                                                  * with EACCESS, it needs to
1043                                                  * list the directory. We
1044                                                  * nevertheless want to allow
1045                                                  * users creating a file.
1046                                                  */
1047                                                 errno = 0;
1048                                         }
1049                                 }
Comment 13 Jeremy Allison 2020-02-13 17:35:43 UTC
Ralph, that's incorrect, in pathname lookup we *are* interesting in enumerating the directory contents in order to match case insensitive names.
Comment 14 Ralph Böhme 2020-02-13 17:37:08 UTC
(In reply to Jeremy Allison from comment #10)
Case sensitivity, right, we're screwed. :)
Comment 15 Ralph Böhme 2020-02-13 17:39:10 UTC
(In reply to Jeremy Allison from comment #12)
That smells like as if it would allow creating files only differing in case in a drobox. Is that what we want?
Comment 16 Jeremy Allison 2020-02-13 17:49:53 UTC
Andreas, we can split this into 2 problems.


cd dirmode733

in smbclient calls:


which does cli_resolve_path(), which over SMB2 I gets a handle to the directory. That is failing with ACCESS_DENIED as we're almost certainly asking for read access on the directory (we'll want to look up attributes etc).

For SMB2 POSIX that specific case could be fixed by using O_PATH in open_directory() when the open request doesn't include read directory contents access, only traverse. That way we'd still maintain a fd handle on the directory but the client would never read it (as they promised).

In order to do a dropbox using smbclient you don't have to cd into the directory. You can instead do:

put \full\pathname\to\dropbox\file.

I'll check this. It may still work as doing vfs_ChDir() on /full/pathname/to/dropbox should still succeed.
Comment 17 Jeremy Allison 2020-02-13 17:51:14 UTC
Ralph wrote:

> That smells like as if it would allow creating files only differing in case in
> a dropbox. Is that what we want?

Yes, that has always been the case. The current code inside source3/smbd/filename.c has always allowed this. If we can't look up existing filename how could it not ?

Historically this hasn't been a problem :-).
Comment 18 Andreas Schneider 2020-02-13 17:54:28 UTC
> I'll check this. It may still work as doing vfs_ChDir() on /full/pathname/to/dropbox should still succeed.

Yes, see the initial description. The vfs_ChDir() succeeds but the vfs_open(".", O_RDONLY|O_DIRECTORY, 0) fails with "Permisson denied".
Comment 19 Jeremy Allison 2020-02-13 17:57:31 UTC
Yes, this still works (just checked master).

Andreas, dropboxes still work. You just can't do a cd into the dropbox directory first. Just use the absolute path from the root of the share and you can create the file.

If you think about it, that makes perfect sense.

This may be a change in behavior, but I'm having a hard time seeing this as a bug.
Comment 20 Jeremy Allison 2020-02-13 18:00:38 UTC
Is the customer using smbclient to do this ?

If so, we *could* change the do_cd() command inside smbclient to ignore ACCESS_DENIED errors (we think the directory is good - we didn't get PATH_NOT_FOUND - we just can't get access anymore) and still set the internal string of the current directory to the requested cd target.

Then the put would work after that. It's a bit wonky though, as it leaves you in a current directory that's not accessible other than for "put" commands.
Comment 21 Jeremy Allison 2020-02-13 18:13:01 UTC
> Andreas, dropboxes still work. You just can't do a cd into the dropbox 
> directory first. Just use the absolute path from the root of the share and you
> can create the file.

Or cd into the directory above the dropbox if the dropbox isn't at the root of the share and then do:

put dropbox/file

will still work. It's the cd failing that is the actual change here. But from what the server sees the client asking for (read access on the cd directory) the server is doing the right thing.
Comment 22 Jeremy Allison 2020-02-13 18:15:50 UTC
Volker, for SMB2 POSIX we can make this work by changing the code in open_directory() in the server to use O_PATH on Linux if the client in a POSIX open only asks for SEC_DIR_TRAVERSE and no other flags.
Comment 23 Andreas Schneider 2020-02-14 08:09:08 UTC
This has been found by Red Hat QA as we have a regression test for it. It was broken in RHEL5 with Samba 3.5.x :-)

Looking at the original bug, the customer used smbclient to drop a file. Exactly the testcase I implemented.

I'm not 100% sure if we should ignore ACCESS_DENIED. We don't know if it is coming from the chdir or the open ...
Comment 24 Andreas Schneider 2020-02-14 10:52:09 UTC
Jeremy, putting the file in the dirmode733 directory from the share directory doesn't work either:

smb: \> put wurst.7623 dirmode733/wurst.7623
NT_STATUS_NOT_FOUND opening remote file \dirmode733\wurst.7623
Comment 25 Jeremy Allison 2020-02-14 16:39:31 UTC
(In reply to Andreas Schneider from comment #24)

Can you check in master first so I can see if it's a regression for 4.11 ? This works in master for me.
Comment 26 Jeremy Allison 2020-02-14 16:41:56 UTC
(In reply to Andreas Schneider from comment #23)

We can ignore the ACCESS_DENIED if it comes from the check_path code, as we know this is an attempt to get attributes on the directory. What we'd do if we get access denied is check the parent directory path, and if that works then we know the lower-level directory exists (otherwise we'd have gotten OBJECT_NOT_FOUND or NOT_A_DIRECTORY) so we can then set the smbclient internal path to the directory that returned ACCESS_DENIED. It would only then work for "put" commands though.

It's a horrible hack, and if the customer can just use the full path instead I'd rather not do it.
Comment 27 Andreas Schneider 2020-02-14 16:53:01 UTC
Created attachment 15790 [details]
dropbox test with 'put wurst dirmode733/wurst'

Yes, I'm testing on master. I've attached my modified test!

make -j20 test TESTS="samba3.blackbox.dropbox"
Comment 28 Jeremy Allison 2020-02-14 17:06:32 UTC
(In reply to Andreas Schneider from comment #27)

Can you try testing by hand first please. I tested by hand and it works fine. I don't want to end up debugging your test :-).
Comment 29 Jeremy Allison 2020-02-14 17:08:24 UTC
Note I'm testing against a share with no VFS modules loaded. Can you test by hand in the same way, and if it works for you then I think you need to look into your test environment.
Comment 30 Jeremy Allison 2020-02-14 17:21:15 UTC
Yep. Just confirmed testing by hand on master, it still works.

Share definition:

        path = /tmp
        read only = no

# mkdir /tmp/test
# chmod 733 /tmp/test
# ls -ld /tmp/test
drwx-wx-wx 2 root root 4096 Feb 14 09:16 /tmp/test

$ /usr/local/samba/bin/smbclient // -Uuser%pass
smb: \> cd test
smb: \> put look test/look
putting file look as \test\look (213.9 kb/s) (average 213.9 kb/s)

$ ls -l /tmp/test/look
-rw-rw-rw- 1 user roup 657 Feb 14 09:16 /tmp/test/look

I think you need to debug your test, sorry.
Comment 31 Jeremy Allison 2020-02-20 00:29:43 UTC
Test for this went into master as e2ea059e671140b32ea2ab1a35691a8e35d4a5f4

I think this shows it's not a bug, so closing as RESOLVED/WORKSFORME.