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.

Changed the title to better reflect what I think is happening.

Also, this applies to master as far as I can see, as well.

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.

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.

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.

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 ?

(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.

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)

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?

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.

Scratch that - I've *definitely* gotten this wrong. Doh! Checking the parent, not the directory itself. I'll fix..

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 on attachment 7132 [details]
Jumbo patch set for 3.6.2

Metze really needs to look at this as well.

Created attachment 7133 [details]
Same patch as before but as a .tgz directory.

This may be too much for 3.6.2. Please advise.

Jeremy.

Can we have a fix for v3-5-test as well?

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.

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.

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.

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)))) {

Of course, an astute reader will insert the missing closing brace.

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.

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.

(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.

Pushed this fix to master (under your name). Thanks ! Will update the patch next..
Jeremy.

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.

This is now fixed, I believe ...