Bug 200953 - Backup Intent flag not set for directory access
Summary: Backup Intent flag not set for directory access
Status: ASSIGNED
Alias: None
Product: File System
Classification: Unclassified
Component: CIFS (show other bugs)
Hardware: All Linux
: P1 high
Assignee: fs_cifs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-27 16:55 UTC by whh
Modified: 2018-08-28 21:55 UTC (History)
1 user (show)

See Also:
Kernel Version: 4.13
Tree: Mainline
Regression: No


Attachments
trace trace of directory access using backupuid option (1.88 KB, application/vnd.tcpdump.pcap)
2018-08-27 16:55 UTC, whh
Details
trace trace of file access using backupuid option (1.50 KB, application/vnd.tcpdump.pcap)
2018-08-27 16:56 UTC, whh
Details
Proposed fix (1.35 KB, patch)
2018-08-27 17:01 UTC, whh
Details | Diff
proposed fix (3.36 KB, application/mbox)
2018-08-27 22:14 UTC, Steve French
Details
tcp trace of mkdir error (1014 bytes, application/vnd.tcpdump.pcap)
2018-08-28 14:58 UTC, whh
Details
tcp trace of failed touch operation (4.62 KB, application/vnd.tcpdump.pcap)
2018-08-28 18:45 UTC, whh
Details
tcp trace of failed chmod operation (3.23 KB, application/vnd.tcpdump.pcap)
2018-08-28 19:20 UTC, whh
Details
fix compounded ops (like mkdir, setinfo etc.) to send backup intent on backupuid mounts (1.11 KB, application/mbox)
2018-08-28 21:22 UTC, Steve French
Details

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

Thx.

Note You need to log in before you can comment on or make changes to this bug.