Bug 10229 - No access check verification on stream files.
Summary: No access check verification on stream files.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-25 19:53 UTC by Jeremy Allison
Modified: 2018-04-03 14:10 UTC (History)
5 users (show)

See Also:


Attachments
git-am fix for master only. (10.51 KB, patch)
2013-10-31 20:52 UTC, Jeremy Allison
no flags Details
Back-port of patch for 3.6.next. (2.97 KB, patch)
2013-10-31 20:52 UTC, Jeremy Allison
ddiss: review+
metze: review+
Details
git-am patchset for 4.1.x (128 bytes, patch)
2013-11-05 19:22 UTC, Jeremy Allison
no flags Details
git-am patchset for 4.0.x (10.95 KB, patch)
2013-11-05 19:24 UTC, Jeremy Allison
metze: review+
Details
git-am patchset for 4.1.x (10.95 KB, patch)
2013-11-07 10:39 UTC, Karolin Seeger
metze: review+
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2013-10-25 19:53:17 UTC
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.
Comment 1 Jeremy Allison 2013-10-31 20:52:08 UTC
Created attachment 9351 [details]
git-am fix for master only.

Patchset that applies cleanly for master, 4.1.x, 4.0.x.

Jeremy.
Comment 2 Jeremy Allison 2013-10-31 20:52:36 UTC
Created attachment 9352 [details]
Back-port of patch for 3.6.next.
Comment 3 David Disseldorp 2013-11-03 16:54:13 UTC
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.
Comment 4 Jeremy Allison 2013-11-04 17:24:38 UTC
(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.
Comment 5 Michael Adam 2013-11-05 12:21:16 UTC
I was trying to review the patches, but while they look good generally,
the master-patchset fails to apply to master.
Comment 6 Michael Adam 2013-11-05 12:23:56 UTC
(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 7 Stefan Metzmacher 2013-11-05 18:20:07 UTC
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...
Comment 8 Jeremy Allison 2013-11-05 18:26:55 UTC
Ah, thought I'd checked that. I'll ensure they work for 4.1.x and 4.0.x.

Jeremy.
Comment 9 Stefan Metzmacher 2013-11-05 18:32:41 UTC
(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).
Comment 10 Jeremy Allison 2013-11-05 18:33:55 UTC
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 11 Jeremy Allison 2013-11-05 19:21:05 UTC
Comment on attachment 9351 [details]
git-am fix for master only.

Changed description to specify master-only.
Comment 12 Jeremy Allison 2013-11-05 19:22:00 UTC
Created attachment 9381 [details]
git-am patchset for 4.1.x
Comment 13 Jeremy Allison 2013-11-05 19:24:36 UTC
Created attachment 9382 [details]
git-am patchset for 4.0.x

Back-ported specifically for 4.0.x.

Jeremy.
Comment 14 Karolin Seeger 2013-11-07 10:39:52 UTC
Created attachment 9397 [details]
git-am patchset for 4.1.x
Comment 15 Karolin Seeger 2013-11-11 10:56:25 UTC
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!
Comment 16 Hemanth 2013-11-11 11:28:37 UTC
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!