Bug 14760 - vfs_streams_depot directory creation permissions and store location problems
Summary: vfs_streams_depot directory creation permissions and store location problems
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.13.0
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on: 14805
  Show dependency treegraph
Reported: 2021-07-15 13:35 UTC by Dinu-Razvan Chis-Serban
Modified: 2021-08-27 16:42 UTC (History)
2 users (show)

See Also:

git-am fix for master. (4.88 KB, patch)
2021-07-19 22:14 UTC, Jeremy Allison
no flags Details
git-am fix for master, 4.15.RCnext (8.04 KB, patch)
2021-08-17 22:58 UTC, Jeremy Allison
no flags Details
git-am fix for 4.15.RCnext. (10.79 KB, patch)
2021-08-19 17:26 UTC, Jeremy Allison
npower: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Dinu-Razvan Chis-Serban 2021-07-15 13:35:02 UTC
Somewhere between v4.12 and v4.13 (all the way up to v4.15) vfs_streams_depot started to ignore "directory mask = 0777" and is creating the directories with 0755 permission which leads to the fact that nobody else can delete/modify the file if it has a stream attached ("create mask = 0777" still works and the stream can be modifyed but not the file). More then that, if the streams folder is outside the share path it generates this error:
conn_rootdir =/samba/shareddata
smbd_smb2_request_error_ex: smbd_smb2_request_error_ex: idx[1] status[NT_STATUS_INVALID_PARAMETER] || at ../../source3/smbd/smb2_create.c:334
smbd_do_qfsinfo: level = 1007
check_reduced_name: Bad access attempt: . is a symlink outside the share path
Comment 1 Jeremy Allison 2021-07-15 16:33:02 UTC
We are trying to discourage access outside the share folder, for obvious reasons.
Comment 2 Jeremy Allison 2021-07-15 16:50:04 UTC
I checked back in the git history and vfs_streams_depot has *always* used 0755 when creating directories within the streams folder.

Here is the initial commit creating vfs_streams_depot.c


If you look inside you'll see:

+       if ((SMB_VFS_NEXT_MKDIR(handle, rootdir, 0755) != 0)
+           && (errno != EEXIST)) {
+               goto fail;
+       }
+       tmp = talloc_asprintf(result, "%s/%2.2X", rootdir, first);
+       if (tmp == NULL) {
+               errno = ENOMEM;
+               goto fail;
+       }
+       if ((SMB_VFS_NEXT_MKDIR(handle, tmp, 0755) != 0)
+           && (errno != EEXIST)) {
+               goto fail;
+       }
+       TALLOC_FREE(tmp);
+       tmp = talloc_asprintf(result, "%s/%2.2X/%2.2X", rootdir, first,
+                             second);
+       if (tmp == NULL) {
+               errno = ENOMEM;
+               goto fail;
+       }
+       if ((SMB_VFS_NEXT_MKDIR(handle, tmp, 0755) != 0)
+           && (errno != EEXIST)) {
+               goto fail;
+       }
+       TALLOC_FREE(tmp);
+       if ((SMB_VFS_NEXT_MKDIR(handle, result, 0755) != 0)
+           && (errno != EEXIST)) {
+               goto fail;
+       }

When did this ever look at "directory mask =" ? I don't see any commits that would change that from inception onwards.

Also, it's very unlikely we will allow stream folders outside of the share path, as it's very dangerous to allow this. It may have worked in the past, but increased awareness of security means this will almost certainly be disallowed in the future.
Comment 3 Dinu-Razvan Chis-Serban 2021-07-16 05:13:19 UTC
OK. The fact that it must reside in the share path is a little annoying (visually) but the access rights are making it unusable because if a group has read/write access over a file but user A (member of that group) adds a stream, than that file becomes read-only for everyone except user A (and root if root is a also a samba user) because smbd drops root privileges and suid to the acccessing user and that coroborated with the 0755 dir create mode (and file crete mode) leads to group haveing read-only access.
Comment 4 Jeremy Allison 2021-07-16 05:15:10 UTC
Did it ever work ? Seriously. Did you have a working implementation before ?

If so, can you describe exactly what it did in terms of user access rights, and I'll take a look at fixing it.
Comment 5 Dinu-Razvan Chis-Serban 2021-07-16 05:59:37 UTC
Well... Previously of version 4.13 I kept the streams directory outside of the share path and with "directory mask" and "create mask" both set to 0777 it is usable. The access is done based on the parent file ACL so that only the users that have r/w on the main file have rw access on the streams. I have changed all SMB_VFS_NEXT_MKDIRAT in vfs_streams_depot.c from 0755 to 0777 and it works as before (if I move the streams directory from outside share path, inside it). I have saw that 0755 was set even in v4.12 but something has changed somewhere. I don't know what and where because the diff is very large and I don't know the insides of Samba.
Comment 6 Jeremy Allison 2021-07-17 02:41:31 UTC
Oh, I know why the "streams directory" outside of share path isn't working.

Let me see what I can do here..
Comment 7 Jeremy Allison 2021-07-17 06:03:25 UTC
I actually think I have a simple fix for this, but I'll start work on it again on Monday.
Comment 8 Jeremy Allison 2021-07-19 22:14:50 UTC
Created attachment 16691 [details]
git-am fix for master.

Can you check this fix with 4.15.rc1 ? It should fix the problem of allowing streams directories outside of the share path.

I've been thinking some more about this and it isn't a security issue, as this module is the only code that access that exterior path, there's no other way to get to it within normal code paths.
Comment 9 Dinu-Razvan Chis-Serban 2021-07-21 08:51:09 UTC
Unfortunately I can't test build v4.15 because I have to create a VM for that and for the moment I lack the time, but I did build it in v4.14.6 and it works.
Comment 10 Jeremy Allison 2021-07-21 16:02:16 UTC
Yeah, unfortunately the fix isn't quite right yet as I added some regression tests. I'm still working on it.
Comment 11 Dinu-Razvan Chis-Serban 2021-08-17 06:56:23 UTC
I've finally found time to fire up a test environment (Fedora35 rawhide in docker) and tested up v4.15.0rc2 with your patch. It works for may use-case (with create mode set up as 0777). The test server is an AD member.
Comment 12 Jeremy Allison 2021-08-17 22:58:45 UTC
Created attachment 16744 [details]
git-am fix for master, 4.15.RCnext

Put this into CI as:


I found the problem with the tests :-).
Comment 13 Samba QA Contact 2021-08-19 17:05:03 UTC
This bug was referenced in samba master:

Comment 14 Jeremy Allison 2021-08-19 17:26:19 UTC
Created attachment 16745 [details]
git-am fix for 4.15.RCnext.

Cherry-picked from master.
Comment 15 Jeremy Allison 2021-08-20 02:19:36 UTC
Note, the fix for https://bugzilla.samba.org/show_bug.cgi?id=14805 needs to go in before this bugfix to ensure the known failure added here is determinstic across all platforms.
Comment 16 Noel Power 2021-08-20 09:55:06 UTC
Comment on attachment 16745 [details]
git-am fix for 4.15.RCnext.

Comment 17 Noel Power 2021-08-20 09:58:12 UTC
Reassigning to Jule for inclusion in 4.15 (note: see comment #15 from Jeremy, needs fix from bug #14805 first)
Comment 18 Jule Anger 2021-08-25 08:49:37 UTC
Pushed to autobuild-v4-15-test.
Comment 19 Dinu-Razvan Chis-Serban 2021-08-25 09:02:04 UTC
I have patched v4.14.6 (current stable from Fedora 34) with this patch and that for bug 14805 and released it into the wild (production). Apparently it works. But still I have a misunderstanding: if the directory mode is 0755 (and the file mode is 0644) how it is supposed to work in a multi-group environment where different users from different groups should modify same/different streams (assuming that that groups have full access on the main file)?
Comment 20 Samba QA Contact 2021-08-25 13:55:12 UTC
This bug was referenced in samba v4-15-test:

Comment 21 Samba QA Contact 2021-08-26 09:20:41 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.0rc3):

Comment 22 Jule Anger 2021-08-27 07:31:47 UTC
(In reply to Dinu-Razvan Chis-Serban from comment #19)
Reassigning to Jeremy to answer the comment #19.
Comment 23 Jeremy Allison 2021-08-27 16:42:37 UTC
I'm going to close this out. The permissions problem is a different issue. To be honest no one has worked on the permission issue. The existing code hasn't changed in that respect since creation, so one could argue it's "working as designed".

Now whether that design is correct, and should be fixed is another matter (and a different bugid :-).