Bug 8556 - ACL permissions ignored when SMBsetatr is requested
ACL permissions ignored when SMBsetatr is requested
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-30 18:30 UTC by Richard Sharpe
Modified: 2012-05-25 19:41 UTC (History)
2 users (show)

See Also:


Attachments
Jumbo patch set for 3.6.2 (104.51 KB, patch)
2011-11-21 19:10 UTC, Jeremy Allison
jra: review? (metze)
Details
Same patch as before but as a .tgz directory. (21.59 KB, patch)
2011-11-21 19:12 UTC, Jeremy Allison
no flags Details
Back-port of the fixes to Samba 3.5.12 (46.41 KB, application/octet-stream)
2011-12-24 17:30 UTC, Richard Sharpe
no flags Details
Patch for the problem with store dos attributes that builds (468 bytes, application/octet-stream)
2012-01-23 14:56 UTC, Richard Sharpe
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2011-10-30 18:30:37 UTC
If you set vfs object = acl_xattr ... and then change the ACL on a file to prevent a user, say USER1, from changing attributes, they are still able to change DOS attributes.

This seems to be because modules/vfs_acl_xattr.c does not implement set_ea_dos_attribute, and so does not get a chance to check if the request is allowed by the ACL. The same would likely be true of get_ea_dos_attribute and setting times etc.

I am not sure if this problem exists in master, but will check.

If my analysis is correct, I am happy to implement the required functionality.
Comment 1 Richard Sharpe 2011-10-30 19:21:13 UTC
OK, it would appear that I am incorrect in my analysis.

Ultimately, reply.c::reply_setatr calls file_set_dosmode, and file_set_dosmode calls set_ea_dos_attribute, which calls SMB_VFS_SETXATTR to perform this if you have store_dos_attributes set.

So, the acl_xattr module does not get a chance to declare its interest in this, and this appears to be a minor security hole if you are using acl_xattr (or, I imagine, acl_tdb.) It is only minor because people who do not have permissions could change DOS attributes and change times.
Comment 2 Richard Sharpe 2011-10-30 19:22:47 UTC
Changed the title to better reflect what I think is happening.

Also, this applies to master as far as I can see, as well.
Comment 3 Jeremy Allison 2011-10-30 19:41:33 UTC
Correct - thanks Richard. Not a security hole I think, more a minor annoyance. I'll update the vfs_acl modules to fix this next week.

Jeremy.
Comment 4 Jeremy Allison 2011-10-30 19:43:40 UTC
Actually, it doesn't belong in the vfs_acl_XX modules, more that the main code needs to check for an open with FILE_WRITE_ATTRIBUTES before updating them (or the pathname equivalent). I'll fix this differently for different branches, for 3.5.x I'll do the vfs_acl_XX change. but in master and in 3.6.x I'll update the main code.

Jeremy.
Comment 5 Jeremy Allison 2011-11-01 23:46:33 UTC
Richard, what calls are you testing ?

Inside smbd/trans2.c we have:

smb_set_file_basic_info() which has:

        if (fsp && !(fsp->access_mask & FILE_WRITE_ATTRIBUTES)) {
                return NT_STATUS_ACCESS_DENIED;
        }

So I'm presuming this is the pathname based wire calls rather than the handle based wire calls that are showing this problem.

Can you tell me how you reproduced it ?

Jeremy.
Comment 6 Jeremy Allison 2011-11-02 00:02:15 UTC
The other interesting thing we need to figure out is what happens to a file which doesn't have FILE_WRITE_ATTRIBUTES allowed for a user, but does have FILE_WRITE_DATA, and the file data is updated. Currently the FILE_ATTRIBUTE_ARCHIVE bit changes on write - does this still happen on a file with FILE_WRITE_ATTRIBUTES denied for a user who is writing to the file ?
Comment 7 Richard Sharpe 2011-11-02 01:40:18 UTC
(In reply to comment #5)
> Richard, what calls are you testing ?
> 
> Inside smbd/trans2.c we have:
> 
> smb_set_file_basic_info() which has:
> 
>         if (fsp && !(fsp->access_mask & FILE_WRITE_ATTRIBUTES)) {
>                 return NT_STATUS_ACCESS_DENIED;
>         }
> 
> So I'm presuming this is the pathname based wire calls rather than the handle
> based wire calls that are showing this problem.
> 
> Can you tell me how you reproduced it ?
> 
> Jeremy.

I actually used smbclient and setmode file-path +r after I had change the ACL from a Real(tm) Windows client to prevent the user I was logged in as via smbclient from modifying attributes or writing the files.

However, the people who alerted me to this used two different Windows clients. Win 2k03 I believe and did attrib +r g:\path\to\file, I believe.
Comment 8 Christian Ambach 2011-11-02 17:21:33 UTC
This does not only affect windows attributes but also extended attributes.
When doing setfattr from a Linux CIFS client no access checks are done (it uses a path based set file information call, not handle based)
Comment 9 Richard Sharpe 2011-11-03 15:10:17 UTC
It looks like smbd/trans2.c::call_trans2findfirst also ignores permissions.

That is, if your set an ACL on a folder that denies user fred any read permissions on that folder (no traverse/execute and no read/list), Samba will still allow the user to list the contents of the folder.

A quick inspection of call_trans2findfirst in 3.5.6 did reveal any code doing permission checking.

Should I open a separate bug on this?
Comment 10 Jeremy Allison 2011-11-03 17:34:32 UTC
If you look in vfs_acl_common you should see a check for LIST rights in the opendir() code.

So yeah, file a separate bug for that one - it should be denied.

In master I moved the permission check to smbd/dir.c - although I may have gotten this wrong.

Jeremy.
Comment 11 Jeremy Allison 2011-11-03 18:28:14 UTC
Scratch that - I've *definitely* gotten this wrong. Doh! Checking the parent, not the directory itself. I'll fix..
Comment 12 Jeremy Allison 2011-11-21 19:10:41 UTC
Created attachment 7132 [details]
Jumbo patch set for 3.6.2

Here is a 40-patch set (back ported from master) that fixes all the issues with checking ACLs on all operations.

I'll also add a tar.gz file of a directory containing each patch separately for people to look at.

Jeremy.
Comment 13 Jeremy Allison 2011-11-21 19:10:59 UTC
Comment on attachment 7132 [details]
Jumbo patch set for 3.6.2

Metze really needs to look at this as well.
Comment 14 Jeremy Allison 2011-11-21 19:12:18 UTC
Created attachment 7133 [details]
Same patch as before but as a .tgz directory.
Comment 15 Jeremy Allison 2011-11-21 19:12:35 UTC
This may be too much for 3.6.2. Please advise.

Jeremy.
Comment 16 Richard Sharpe 2011-11-26 05:38:03 UTC
Can we have a fix for v3-5-test as well?
Comment 17 Jeremy Allison 2011-11-26 21:06:30 UTC
Doing this for 3.5.x is a lot of fiddly back-porting work... Can you get someone to give you a contract to do that ?

Jeremy.
Comment 18 Richard Sharpe 2011-12-24 17:30:12 UTC
Created attachment 7216 [details]
Back-port of the fixes to Samba 3.5.12

This is one single patch that applies to 3.5.12 and provides equivalent functionality, I believe.
Comment 19 Richard Sharpe 2012-01-23 00:29:39 UTC
There still seems to be a bug when a user does:

NTCreate&X on a directory

TRANS2 SET_FILE_BASIC_INFO.

The call to smb_set_file_basic_info calls 

        status = smbd_check_access(conn, fsp, smb_fname, FILE_WRITE_ATTRIBUTES);
        if (!NT_STATUS_IS_OK(status)) {
                return status;
        }

However, back in smbd/open.c we overrode things:

[2012/01/22 14:42:06.203889, 10] smbd/open.c:167(smbd_check_access_rights)
  smbd_check_access_rights: overrode FILE_WRITE_ATTRIBUTES on file folder1
[2012/01/22 14:42:06.203908,  5] smbd/files.c:119(file_new)

so that when the call to smbd_check_access occurs, we now think it is OK to change the attributes.

However, this is wrong. The user did not have permission in the ACL to change attributes.

Checking why this occurs now.
Comment 20 Richard Sharpe 2012-01-23 02:55:52 UTC
The patch still contains an error.

Keep in mind that map archive and map readonly default to yes.

Now, in smbd/open.c we have:

NTSTATUS smbd_check_access_rights(struct connection_struct *conn,
                                const struct smb_filename *smb_fname,
                                uint32_t access_mask)

and in this we have:

        /* Here we know status == NT_STATUS_ACCESS_DENIED. */
        if ((access_mask & FILE_WRITE_ATTRIBUTES) &&
                        (rejected_mask & FILE_WRITE_ATTRIBUTES) &&
                        (lp_map_readonly(SNUM(conn)) ||
                        lp_map_archive(SNUM(conn)) ||
                        lp_map_hidden(SNUM(conn)) ||
                        lp_map_system(SNUM(conn)))) {
                rejected_mask &= ~FILE_WRITE_ATTRIBUTES;

                DEBUG(10,("smbd_check_access_rights: "
                        "overrode "
                        "FILE_WRITE_ATTRIBUTES "
                        "on file %s\n",
                        smb_fname_str_dbg(smb_fname)));
        }

which ignore the value of store dos attributes, which means that users who should not be able to change attributes can still do so because of those defaults.

The test should really be:

        /* Here we know status == NT_STATUS_ACCESS_DENIED. */
        if ((access_mask & FILE_WRITE_ATTRIBUTES) &&
                        (rejected_mask & FILE_WRITE_ATTRIBUTES) &&
                        !lp_store_dos_attributes(SNUM(conn) && 
                        (lp_map_readonly(SNUM(conn)) ||
                        lp_map_archive(SNUM(conn)) ||
                        lp_map_hidden(SNUM(conn)) ||
                        lp_map_system(SNUM(conn)))) {
Comment 21 Richard Sharpe 2012-01-23 02:56:59 UTC
Of course, an astute reader will insert the missing closing brace.
Comment 22 Jeremy Allison 2012-01-23 04:45:32 UTC
Ok - thanks for that Richard. I'll evaluate your change and get it into master and update the patch for 3.6.x. Maybe once 3.6.2 has shipped we can get it into 3.6.3.

Jeremy.
Comment 23 Richard Sharpe 2012-01-23 14:56:50 UTC
Created attachment 7251 [details]
Patch for the problem with store dos attributes that builds

Attached is a patch that builds on 3.5.12 + previous patch.

It has not been tested as yet, but seems correct.
Comment 24 Richard Sharpe 2012-01-23 16:51:30 UTC
(In reply to comment #23)
> Created attachment 7251 [details]
> Patch for the problem with store dos attributes that builds
> 
> Attached is a patch that builds on 3.5.12 + previous patch.
> 
> It has not been tested as yet, but seems correct.

You can trigger this bug by:

1. storing ACLs in XATTRs

2. setting store dos attributes = yes

3. Set up an ACL on a file or folder that does not allow CHANGE ATTRIBUTES for a user, say DOM\fred

4. As DOM\fred, access the share and then in the explorer, try to set HIDDEN or something. Samba versions with the first patch provided to fix the other problems will allow it from the explorer but not from DOS.
Comment 25 Jeremy Allison 2012-01-23 20:54:41 UTC
Pushed this fix to master (under your name). Thanks ! Will update the patch next..
Jeremy.
Comment 26 Jeremy Allison 2012-01-23 22:12:42 UTC
An additional fix is needed to finalize this - need to remove the check for FILE_WRITE_ATTRIBUTE permission check in the POSIX chmod call path. This is incorrect, we're changing POSIX permissions here - not file attributes (unless you're storing attributes in POSIX permissions, which is not recommended).

Jeremy.
Comment 27 Richard Sharpe 2012-05-14 01:23:26 UTC
This is now fixed, I believe ...