From Hemanth Thummala <hemanth.thummala@gmail.com>: Looks like there are no ACL checks against stream files while performing generic file operations like read and write. For example: we have created a stream file on remote CIFS share using domain administrator as follows. C:\>echo "NON-Default stream file" > z:\non-def-stream .txt:stream1 C:\>more < z:\non-def-stream.txt:stream1 "NON-Default stream file" After that we have added explicit deny write ACE for a specific domain user. But that user was still be able to access and write the data to this stream file thought he has an explicit deny permission. Whereas this issue is not seen with $DATA streams. >From the code, I could see whenever create request comes for stream file, we trying to open the base file with access mask as zero.. /* Open the base file. */ status = create_file_unixpath(conn, NULL, smb_fname_base, 0, ===> Access mask is zero. FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, base_create_disposition, 0, 0, 0, 0, 0, NULL, NULL, &base_fsp, NULL); And of course lot other parameters were sent as zero. FSP coming out of this getting assigned to fsp->base_fsp. In open_acl_common(), if fsp->base_fsp is non null then we are skipping the ACL check claiming we already did on base file. But while doing on base file, we used access mask as zero and it is not evaluated for original access request from client. Here is the code snippet in open_acl_common() which does this.. if (fsp->base_fsp) { /* Stream open. Base filename open already did the ACL check. */ DEBUG(10,("open_acl_common: stream open on %s\n", fsp_str_dbg(fsp) )); return SMB_VFS_NEXT_OPEN(handle, smb_fname, fsp, flags, mode); } In my case, problem is not see after using the original access mask in create_file_unixpath() for base file. Here is the simple change that worked for me. bash-4.0$ diff -up samba-3.6.12/source3/smbd/open.c.orig samba-3.6.12/source3/smbd/open.c --- samba-3.6.12/source3/smbd/open.c.orig 2013-10-25 03:49:03.651630453 -0700 +++ samba-3.6.12/source3/smbd/open.c 2013-10-25 03:48:46.243898186 -0700 @@ -3262,7 +3262,7 @@ static NTSTATUS create_file_unixpath(con } /* Open the base file. */ - status = create_file_unixpath(conn, NULL, smb_fname_base, 0, + status = create_file_unixpath(conn, NULL, smb_fname_base, access_mask, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, Please let me know if this is the correct thing to do.
Created attachment 9351 [details] git-am fix for master only. Patchset that applies cleanly for master, 4.1.x, 4.0.x. Jeremy.
Created attachment 9352 [details] Back-port of patch for 3.6.next.
Comment on attachment 9351 [details] git-am fix for master only. I'm a little concerned about the skip of check_base_file_access() on stat failure. Shouldn't the stat error be propagated upwards for anything other than ((errno == ENOENT) && (base_create_disposition = FILE_OPEN_IF))? Looks good otherwise, especially with the regression test.
(In reply to comment #3) > Comment on attachment 9351 [details] > git-am fix for master, 4.1.next and 4.0.next. > > I'm a little concerned about the skip of check_base_file_access() on stat > failure. > Shouldn't the stat error be propagated upwards for anything other than ((errno > == ENOENT) && (base_create_disposition = FILE_OPEN_IF))? Is the stat fails then once it goes into the normal open path then it will be flagged as "file does not exist" (or access denied, depending on why the stat was denied) and the normal open code paths will take care of it. I really didn't want to add "does file exist" logic additionally into the wrapper function, as it's already correctly handled in open. > Looks good otherwise, especially with the regression test. Thanks ! Jeremy.
I was trying to review the patches, but while they look good generally, the master-patchset fails to apply to master.
(In reply to comment #5) > I was trying to review the patches, but while they look good generally, > the master-patchset fails to apply to master. Oh gosh, forget this. The patches _are_ already in master...
Comment on attachment 9351 [details] git-am fix for master only. This doesn't compile on top of v4-0-stable, I haven't checked v4-1-stable yet...
Ah, thought I'd checked that. I'll ensure they work for 4.1.x and 4.0.x. Jeremy.
(In reply to comment #8) > Ah, thought I'd checked that. I'll ensure they work for 4.1.x and 4.0.x. I'd propose to upload patches for each branch including cherry-pick information (git cherry-pick -x).
Ok, but if the master patch doesn't compile on 4.0.x, then only the 4.1.x patch will include cherry-pick info. I'm working on this right now. Jeremy.
Comment on attachment 9351 [details] git-am fix for master only. Changed description to specify master-only.
Created attachment 9381 [details] git-am patchset for 4.1.x
Created attachment 9382 [details] git-am patchset for 4.0.x Back-ported specifically for 4.0.x. Jeremy.
Created attachment 9397 [details] git-am patchset for 4.1.x
Pushed to v4-1-test, v4-0-test and v3-6-test. Included in Samba 4.1.1, 4.0.11 and 3.6.20. Closing out bug report. Thanks!
Thanks Jeremy and others for quickly addressing the issue. From my side, I have verified the changes. It works fine. Once again Thank you very much!