Bug 8556 - ACL permissions ignored when SMBsetatr is requested
 Summary: ACL permissions ignored when SMBsetatr is requested
 Status: RESOLVED FIXED Samba 3.6 Unclassified File services unspecified All All P5 normal --- Jeremy Allison Samba QA Contact Show dependency tree / graph

 Reported: 2011-10-30 18:30 UTC by Richard Sharpe 2012-05-25 19:41 UTC (History) 2 users (show) fumiyas metze

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.
 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. 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. 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. 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. 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. 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. 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 ? 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. 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) 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? 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. 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.. 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. 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. Jeremy Allison 2011-11-21 19:12:18 UTC Created attachment 7133 [details] Same patch as before but as a .tgz directory. Jeremy Allison 2011-11-21 19:12:35 UTC This may be too much for 3.6.2. Please advise. Jeremy. Richard Sharpe 2011-11-26 05:38:03 UTC Can we have a fix for v3-5-test as well? 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. 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. 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. 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)))) { Richard Sharpe 2012-01-23 02:56:59 UTC Of course, an astute reader will insert the missing closing brace. 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. 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. 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. Jeremy Allison 2012-01-23 20:54:41 UTC Pushed this fix to master (under your name). Thanks ! Will update the patch next.. Jeremy. 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. Richard Sharpe 2012-05-14 01:23:26 UTC This is now fixed, I believe ...