Bug 200953

Summary: Backup Intent flag not set for directory access
Product: File System Reporter: whh
Component: CIFSAssignee: fs_cifs (fs_cifs)
Severity: high CC: lsahlber, smfrench
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.13 Subsystem:
Regression: No Bisected commit-id:
Attachments: trace trace of directory access using backupuid option
trace trace of file access using backupuid option
Proposed fix
proposed fix
tcp trace of mkdir error
tcp trace of failed touch operation
tcp trace of failed chmod operation
fix compounded ops (like mkdir, setinfo etc.) to send backup intent on backupuid mounts

Description whh 2018-08-27 16:55:29 UTC
Created attachment 278137 [details]
trace trace of directory access using backupuid option

I am using "backupuid=<uid>" option for SMB mount, but I noticed that the "backup intent" flag is only set when client is accessing files, but not directories, resulting permission error during directory access.

I attached a trace for directory access, and a trace for file access.
Comment 1 whh 2018-08-27 16:56:26 UTC
Created attachment 278139 [details]
trace trace of file access using backupuid option
Comment 2 whh 2018-08-27 17:01:26 UTC
Created attachment 278141 [details]
Proposed fix

This is the patch I used to fix the problem. Note it also fixes a few places where backup intent flag is not being set.
Comment 3 Steve French 2018-08-27 21:22:07 UTC
Looks reasonable.  Any thoughts about the two places in inode.c (rename) whether someone with backup_intent would ever rename? or whether it would make a difference for security?
Comment 4 Steve French 2018-08-27 22:14:23 UTC
Created attachment 278151 [details]
proposed fix
Comment 5 Steve French 2018-08-27 22:20:38 UTC
Let me know if the patch works for you
Comment 6 whh 2018-08-27 22:33:14 UTC
Thanks. I will try the patch and let you know.

Yeah, these places in inode.c should add as well (although I have not tested rename). <SMB2 CREATE Request> should have the flag set if client intends the file open/create for backup intent. 

From SMB2 document on the flag "FILE_OPEN_FOR_BACKUP_INTENT 0x00004000":

The file is being opened for backup intent. That is, it is being
opened or created for the purposes of either a backup or a restore
operation. The server can check to ensure that the caller is capable
of overriding whatever security checks have been placed on the file
to allow a backup or restore operation to occur. The server can
check for access rights to the file before checking the DesiredAccess field.
Comment 7 Steve French 2018-08-28 01:36:43 UTC
I have mixed feelings about whether rename is worth changing - but I did add a few more (reading alternate device and symlink formats e.g.)
Comment 8 whh 2018-08-28 14:57:37 UTC
Tested the patch using kernel version 4.13, works fine except creating directory under a ACL-restricted directory. I will attach a tcp trace for the mkdir error.
Comment 9 whh 2018-08-28 14:58:06 UTC
Created attachment 278157 [details]
tcp trace of mkdir error
Comment 10 whh 2018-08-28 14:59:19 UTC
Creating a new file under the same directory is fine (the backup intent flag is set properly). In mkdir case, the backup intent flag is not set, hence the access denied failure.
Comment 11 whh 2018-08-28 18:45:22 UTC
Another problem is when I simply does a "touch" operation on a file (ACL restricted), the backup intent flag is not set and operation is denied:

root@dynapod-colo-2136-105-7-107-134-vnode1:~# touch /tmp/mount/windows_dir/1.txt
touch: setting times of '/tmp/mount/windows_dir/1.txt': Permission denied

Surprisingly, I can edit the same file using vi and it went through fine.

I will attached the tcp trace for the touch operation as well.
Comment 12 whh 2018-08-28 18:45:49 UTC
Created attachment 278179 [details]
tcp trace of failed touch operation
Comment 13 whh 2018-08-28 18:55:27 UTC
chmod operation also has the same problem.
Comment 14 whh 2018-08-28 19:09:37 UTC
Looking at the code, I think smb2_open_op_close() function should also add the CREATE_OPEN_BACKUP_INTENT flag to create_options if backupuid matches.

diff --git a/linux-4.13.0/fs/cifs/smb2inode.c b/linux-4.13.0/fs/cifs/smb2inode.c
index 1238cd3..779cc11 100644
--- a/linux-4.13.0/fs/cifs/smb2inode.c
+++ b/linux-4.13.0/fs/cifs/smb2inode.c
@@ -56,6 +56,8 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
        oparms.tcon = tcon;
        oparms.desired_access = desired_access;
        oparms.disposition = create_disposition;
+       if (backup_cred(cifs_sb))
+               create_options |= CREATE_OPEN_BACKUP_INTENT;
        oparms.create_options = create_options;
        oparms.fid = &fid;
        oparms.reconnect = false;

I can test this (diff is based on 4.13, but latest kernel appears to be the same).
Comment 15 Steve French 2018-08-28 19:11:21 UTC
Now "smb2_compound_op()" needs this (with Ronnie's compounding optimizations this actually makes it easy to hit multiple paths with the needed additional change).
Comment 16 Steve French 2018-08-28 19:12:44 UTC
open_op_close has changed to smb2_compound_op
Comment 17 whh 2018-08-28 19:20:58 UTC
Created attachment 278181 [details]
tcp trace of failed chmod operation

chmod trace is attached
Comment 18 Steve French 2018-08-28 21:22:29 UTC
Created attachment 278187 [details]
fix compounded ops (like mkdir, setinfo etc.) to send backup intent on backupuid mounts

Requires Ronnie's compounding patches (see linux-next build or equivalently cifs-2.6.git for-next branch) because open_query_close function is not used any more
Comment 19 whh 2018-08-28 21:55:09 UTC
I tested using 4.13 kernel (due to limitation of my side), and my previous patch in smb2_open_op_close() works (chmod/touch/mkdir). 

I'd assume smb2_compound_op() works the same way as smb2_open_op_close().

Comment 20 Ronnie Sahlberg 2019-02-07 22:12:34 UTC
From comment #19,  it is confirmed the patches works? and we can close this bug?
Comment 21 whh 2019-02-07 22:46:42 UTC
The patch works fine (there is another problem but tracked in another bug, I believe)