Bug 15608 - lack of locking checking for the destination of rename
Summary: lack of locking checking for the destination of rename
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.19.3
Hardware: All All
: P5 major (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-19 17:22 UTC by Michael Tokarev
Modified: 2024-07-12 20:02 UTC (History)
1 user (show)

See Also:


Attachments
tcpdump of mv operation (3.49 KB, application/vnd.tcpdump.pcap)
2024-03-19 17:22 UTC, Michael Tokarev
no flags Details
samba level8 log of the rename operation (18.41 KB, text/x-log)
2024-03-19 17:23 UTC, Michael Tokarev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Tokarev 2024-03-19 17:22:59 UTC
Created attachment 18268 [details]
tcpdump of mv operation

It looks like samba server does not check if the destination file of rename operation is locked.  The source is checked, but not the destination.  So samba allows to overwrite a file which is opened and locked by another client, and to make clients misbehave.

Also, due to delayed delete and combining it with rename, samba allows unexpected silent data loss, - hence the severity of this bug report.

Having Samba server S, a Windows client W, and a Linux client L, and two files, file.exe and file.new.

1. On W, mount samba share and lock file.exe (for example, start the executable).

2. On L, mount the same samba share using cifs, and do a simple `mv file.new file.exe'.  This will succeed, despite the fact that file.exe is open and locked on W.  S does not check if destination file is locked.

3. Windows is unable to open file.exe anymore, returning "Invalid parameter" error at open.

Attached are tcpdump and level8 debug log from samba resulted from just the mv operation (mounted the filesystem over loopback on the same server).


For unexpected data loss, this issue is combined with delayed delete, like this:

1. On W, mount samba share and lock file.exe

2. On L, mount samba share and run `rm file.exe'.  This will hang.  Interrupt the rm process.

3. On L. do `mv file.new file.exe'

4. On W, release the lock.

5. Watch file.exe (which is now file.new already!) is deleted by S.
Comment 1 Michael Tokarev 2024-03-19 17:23:33 UTC
Created attachment 18269 [details]
samba level8 log of the rename operation
Comment 2 Michael Tokarev 2024-03-19 17:25:09 UTC
The observed behavior seems not dependent on the linux client kernel version, - tried 6.1, 5.10 and 6.6 kernels, all behave the same way.
Comment 3 Michael Tokarev 2024-03-19 17:29:09 UTC
In the traces, file.exe is xml_upload.exe, and file.new is xml_upload.exe.1 - just a random executable I found there.
Comment 4 Jeremy Allison 2024-03-25 19:00:33 UTC
I have a plan for this. In the SMB2 rename code path, before we take any locks we should attempt an SMB_VFS_CREATE_FILE() on the destination file with no allowed sharing (FILE_SHARE_NONE), DELETE access and create options FILE_DELETE_ON_CLOSE.

If this open fails then we know some other node has the target file open and we should deny the rename.

We need some torture tests for Windows so we understand the desired behavior first (what happens with target file leased etc.).

We might not be able to completely reproduce Windows behavior (lease breaks etc.) but this should fix the immediate problem.
Comment 5 Michael Tokarev 2024-07-12 20:02:16 UTC
I'm currently searching for any way to update files from linux, through samba/cifs, maybe using smbclient, anything.  It looks like samba currently is unable to update a file without adding a windows machine to the mix.  Right now I'm mounting samba resource on a (virtual) windows machine, export this filesystem from windows back to this same linux machine, mounting it using cifs, and updating files in there, - this FINALLY works.  The VM can be run on the same linux machine so it is still sort of self-contained.  But it's utter idiocy  that we don't have any better way to update files...