I have Samba servers that need to serve data out of IBM DCE/DFS and AFS. In the past we have used "acl check permissions = no" rather than writing code so that Samba can check permissions on the AFS or DFS directory. This no longer works from our Windows XP SP3 clients against a set of Samba servers running 3.5.8. I haven't tested this since Samba 3.2.7 so I don't know exactly what point this changed. I see in smbd/open.c there are two cases where can_delete_file_in_directory is called without an exception for lp_acl_check_permissions() being false. These are in open_directory() and open_file(), the exception is in place in create_file_unixpath(). I updated smbd/open.c so that can_delete_file_in_directory is not ever called when lp_acl_check_permissions is false. This seems to fix the problem for me. I'm not permitted to upload a patch, but this seems a simple fix. I've also checked in Samba 3.6.0rc2 and still see the problem.
Created attachment 6611 [details] git-am patch for 3.5.next John can you test this. Christian can you review please ? Thanks ! Jeremy.
This works for the delete case. I realized I was only testing deletes, while the original report from my test user was on a rename. I re-tested with rename and unfortunately it fails fix and this one. The difference between the delete and rename seem to be with the flags sent by the client. The rename shows the following in the log: [2011/06/23 12:10:51.375469, 10, pid=9633832] smbd/open.c:113(smbd_check_open_rights) smbd_check_open_rights: file usr8/jpjanosi/test_rename/testfile1 requesting 0x110080 returning 0x110000 (NT_STATUS_ACCESS_DENIED) Then after the SD lines: [2011/06/23 12:10:51.377623, 10, pid=9633832] smbd/open.c:537(open_file) open_file: overrode DELETE_ACCESS on file usr8/jpjanosi/test_rename/testfile1 [2011/06/23 12:10:51.377674, 10, pid=9633832] smbd/open.c:545(open_file) open_file: Access denied on file usr8/jpjanosi/test_rename/testfile1 So the problem in the case of the rename is that SYNCHRONIZE_ACCESS is getting set in access_mask, and therefore set in access_granted. I'm not sure the best way and for what cases I should override the following code in open_file: if (access_granted != 0) { DEBUG(10,("open_file: Access " "denied on file " "%s\n", smb_fname_str_dbg( smb_fname))); return status; } I suppose I could set access_granted to 0 if lp_acl_check_permissions() is false, but it seems ugly since the checks for it is now in can_delete_file_in_directory.
Created attachment 6612 [details] git-am patch for 3.5.next Updated patch that should deal with the comment from John. Christian please review. Jeremy.
The latest patch didn't compile for me in 3.5.8: Compiling smbd/open.c "smbd/open.c", line 90.35: 1506-045 (S) Undeclared identifier access_desired. "smbd/open.c", line 94.25: 1506-276 (S) Syntax error: possible missing ')'? "smbd/open.c", line 92.17: 1506-275 (S) Unexpected text ')' encountered. I had to change access_desired to access_mask and add a couple of commas to the debug statement. After those changes both the rename and deletes work as expected. Thanks again.
Oh, how embarrassing. I was pretty sure I'd compiled this once I updated the fix :-). I'll fix and update the patch... Jeremy.
Created attachment 6613 [details] git-am patch for 3.5.next This one actually compiles :-(. Sorry.
Comment on attachment 6613 [details] git-am patch for 3.5.next Lacking a test system, I stared at the code for a while. Basically, it looks good, but I am not sure if the added check in smbd_check_open_rights() is not too coarse. Maybe you can comment on the following scenario to see if my thoughts are valid: If a client wants to create a temporary file with WRITE/DELETE, access will be granted because DELETE is set and so all other checks will be skipped. Shouldn't we still check the other create options and just ignore DELETE? Christian
Christian - you're missing the logic here I think. These checks before open are only to detect (a) if a stored NT ACL would deny an open and (b) to detect if a open for delete access would be allowed (i.e. would the opener be able to delete). When you set "acl check permissions = no" you're *explicitly* asking Samba not to do check (a) (or check (b) as well). What you're asking is that Samba default back to the permissions on disk only. So all the new check in smbd_check_open_rights() does is prevent us checking case (a), which is what the user asked for. The other change in can_delete_file_in_directory() fixes the case (b) check. There is no security issue as the checks by the operating system are still performed on any access. So I'm arguing my patch is correct here :-). Jeremy.
Jeremy/Christian: John has contacted me about this as I've a similar environment by using Samba with DCE/DFS. Since 3.3.0, I'm simply using some "#ifdef 0" to bypass those check but the use of "acl permission check = no" with this patch would be much better. I've compiled 3.5.9 tuesday with the patch for bug 8238. Let me know if you want me to test it by using this patch instead of my "#ifdef 0" Best regards, Yannick Bergeron
Yes please. Another data point that the patch fixes your problem would be helpful (the patch has already gone into master). Jeremy.
(In reply to comment #8) > Christian - you're missing the logic here I think. These checks before open are > only to detect (a) if a stored NT ACL would deny an open and (b) to detect if a > open for delete access would be allowed (i.e. would the opener be able to > delete). > When you set "acl check permissions = no" you're *explicitly* asking Samba not > to do check (a) (or check (b) as well). What you're asking is that Samba > default back to the permissions on disk only. So all the new check in > smbd_check_open_rights() does is prevent us checking case (a), which is what > the user asked for. The other change in can_delete_file_in_directory() fixes > the case (b) check. the man page for "acl check permissions" says, that this flag only control smbd's behaviour for delete on close. So my understanding is that this concerns only check (b). The check that gets added in smbd_check_open_rights() now looks at each request that has DELETE set and makes a decision, regardless of the other flags. I think this is not accurate enough as every request that comes in that has the DELETE flag set will be considered valid in this check, even if e.g. WRITE is requested but the user does not have write permission for that file. > There is no security issue as the checks by the operating system are still > performed on any access. Yes, in the case above, the filesystem will then make the final decision and reject the request. So there finally is no problem, my argument would just be that we could detect the fact that access will be denied before we hand over the request to the file system. So the patch does what it should do, it might just not be perfect as it does more than what is described in the manpage. Christian
The only way DELETE_ON_CLOSE can succeed is if DELETE rights are granted in the open, so the only way to detect an attempt to DELETE_ON_CLOSE is to assume that every open requesting DELETE access is such an attempt. Once a handle is open with DELETE access a client can set/unset the DELETE_ON_CLOSE bit at will. So the fix is as granular as it possibly can be given the desire to avoid this on filesystems whose ACLs are different from POSIX. I think you're agreeing with me that the patch is good, I just want to make sure you know that it isn't due to lazyness on my part that it isn't better :-). Please +1 the patch and re-assign this to Karolin for inclusion in 3.5.next. The same patch also applies to 3.6.0, but I'm ok with this going in for 3.6.1 if she so decides as this isn't a critical fix. Jeremy.
I was always good with the patch, just wanted to discuss the partly subtle semantics. Karolin, it's all yours now
(In reply to comment #12) > The same patch also applies to 3.6.0, but I'm ok with this going in for 3.6.1 > if she so decides as this isn't a critical fix. Pushed patch to v3-5-test. I tried to apply it for 3.6 also (I think this should be fixed before the release), but it does not apply on top of current v3-6-test (833fdb5b3): user@host:/data/git/samba/v3-6-test> git am 0001-Fix-bug-8254-acl-check-permissions-no-does-not-work-.patch Applying: Fix bug #8254 - "acl check permissions = no" does not work in all cases error: patch failed: source3/smbd/open.c:86 error: source3/smbd/open.c: patch does not apply Patch failed at 0001 Fix bug #8254 - "acl check permissions = no" does not work in all cases Re-assigning to Jeremy to either close out the bug report or attach an updated patch for 3.6. Thanks!
Created attachment 6640 [details] git-am fix for 3.6.0 Sorry, forgot to add the 3.6.0 patch (same as the patch that went into master). Jeremy.
Re-assigning to Karolin with added 3.6.0 patch for inclusion. Jeremy.
(In reply to comment #15) > Created attachment 6640 [details] > git-am fix for 3.6.0 > > Sorry, forgot to add the 3.6.0 patch (same as the patch that went into master). > Jeremy. Stupid me! Thanks!
(In reply to comment #16) > Re-assigning to Karolin with added 3.6.0 patch for inclusion. > Jeremy. Pushed to v3-6-test. Closing out bug report. Thanks!
Ok I seem to still have this issue, only on Windows 7 SP1, only with its explorer. WinXP and Win7 SP1 using Samba 3.5.5 and my old home-grown patch (#ifdef 0) works. WinXP using Samba 3.5.9 with this patch works. Win7 SP1 using Samba 3.5.9 with this patch works on the command.exe prompt, but not with the explorer (access is denied when I try to list the content of the network drive) I'm appending my log level 10 logs from the WinXP workstation (yaberger) and from the Win7 SP1 workstation (tstprog-7)
Created attachment 6704 [details] log level 10 from WinXP workstation accessing network drive on 3.5.9
Created attachment 6705 [details] log level 10 from Win7 SP1 workstation accessing network drive on 3.5.9, through the explorer
Created attachment 6706 [details] log level 10 from Win7 SP1 workstation accessing network drive on 3.5.9, throught the command prompt part 1 just issuing the Z: command to access the network drive, not even listing the content
Created attachment 6707 [details] log level 10 from Win7 SP1 workstation accessing network drive on 3.5.9, throught the command prompt part 2 already in the network drive Z:, issuing the dir command
Your explorer log shows that the explorer client is getting access denied when attempting to open the "." directory on the share, which has mapped POSIX permissions of: u:rwx g:rwx o:--- I'm guessing the user accessing this share isn't either the user or group, in which case smbd is doing the right thing. The client is asking for access 0x100001 (SYNCHRONIZE|FILE_LIST_DIRECTORY) access. Remember, "acl check permissions" only acts when the client is asking for DELETE access (see the smb.conf man page). Jeremy.
I won't have time to duplicate Yannick's tests until later next week due to vacation. I didn't realize there were cases besides delete(which I understand due to the different Windows behaviour of denying delete on open), that Samba tried to resolve ACLs to check access rather than relying on trying the operation as the user. If this can happen then I can't use Samba to export these filesystems without patching Samba similar to the way Yannick has. Also I dont see the point of the parameter fixing this for delete operations and not other operations. If any operation can fail for a user that should have access then it becomes a problem to use Samba with that filesystem.
As John stated, with DFS (or AFS), this will not work. Yes the logged user is not the user owning the file or part of the group. But there are extended ACLs behind (not posix) which allow him to have read and/or write access to it. I'll take a closer look at my log files today because I simply don't understand why it would work on WinXP (explorer and command prompt) and on Win7 SP1 (command prompt only) I would really hate to bring back my old ugly "#ifdef 0" patch and hope we could figure a real patch to be pushed in master/3.5/3.6 Best regards,
acl check permissions was created to fix the open-for-delete problem only. It's a hack, basically, that doesn't really belong anymore. Modern Samba has to check users permissions without OS help all the time (it is used to enable us to have completely compatible Windows permissions, which is what users want). If AFS has hidden permissions then we need to create a vfs_acl_afs module that exposes these (either as Windows perms or POSIX ACL perms). That seems like the correct solution. Can you point me at the AFS ACL API's so I can see how this might be done. Looks like a good summer job for an IBM intern to me :-). Jeremy.
vfs_afsacl is already there, Volker wrote it a while ago and it became a little bit bit-rot because nobody tried to build it for a while As part of Bug #8263, I have made it compile again in master But this does not help for DCE/DFS, there is no module for that filesystem yet.
Oh, I forgot you're an IBM employee Christian ! This means this is *your* bug :-). Seriously, let me know exactly what is needed here and we'll spruce up the AFS ACL module or add another one for DCE/DFS. It's not too hard - we just need to work out the mapping from the underlying ACL model to Windows ACLs. Jeremy.
I don't think a vfs module for DCE/DFS worth it. The product is end of life and most of remaining cells are going to be replaced sooner or later. But fact is that they still exist :/ As for AFS, well OpenAFS is still being developped and IBM AFS is still being used internally. That would probably be a good idea to have its VFS module working if it's not currently. But most likely, people will not use Samba to share AFS cells, they will simply use AFS clients unless there are none for their specific platform.
I agree with Yannick that a DCE/DFS module is not worth it. I know of a few Samba servers in a DCE/DFS cell I support, but we are planning to move the users to AFS shortly. Unfortunately the AFS client for Windows 7 has some problems so we need Samba for AFS access. Also we have users that will not install AFS clients because they want the OS install they use and test with to exactly match what their customers and partners have. The customers and partners don't have AFS environments. I've got the vfs_afsacl module configured in my test environment, it only resolves users on the ACLs, not groups. I could probably update this but I have some doubts. Right now there is no support in Samba for switching an smbd process between AFS pags. This would be needed so that it can do some things in AFS as root with one set of AFS tokens and the rest when running as the user with their AFS tokens. In a past Samba 3.2.X environment I patched Samba so that the unix token contained the AFS and DFS pag numbers. I added a simple kernel module to the servers to expose the kernel functions to get and set a pag. This allowed the samba process to switch between pags in the code in sec_ctx.c. I could bring this back and then somehow implement a way that the root pag had AFS tokens with the authority needed stat any file in AFS and enumerate all group memberships. I don't see that changes like this would be accepted back into Samba and I'm not sure how to do them in a way that is portable.