Bug 14760 - vfs_streams_depot directory creation permissions and store location problems
Summary: vfs_streams_depot directory creation permissions and store location problems
Status: ASSIGNED
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
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-15 13:35 UTC by Dinu-Razvan Chis-Serban
Modified: 2021-07-21 16:02 UTC (History)
1 user (show)

See Also:


Attachments
git-am fix for master. (4.88 KB, patch)
2021-07-19 22:14 UTC, Jeremy Allison
no flags Details

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
resolved_name=/samba/.streams/9A/8D/9FC36137C23292243000640000000000
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

dfd05b9b65c5ad12dbb64b626d3180fb9e21ccb0

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.