From: Steven Engelhardt via samba-technical <samba-technical@lists.samba.org> To: "samba-technical@lists.samba.org" <samba-technical@lists.samba.org> Subject: Duplicate SMB file_ids leading to Windows client cache poisoning Samba Team, While trying to roll out recent versions of Samba, we believe we have discovered a bug in Samba related to SMB file id generati on which can lead to Windows SMB clients to observe wrong data. Samba version 4.11.1 changed how it generates SMB file ids to be instantiation time-based. This change introduced a bug in that it made it possible for two different files on the same volume to have the same file id, in volation of the SMB spec. This ap pears to lead to cache poisoning in the Windows SMB client-side cache, in which two different files share a common cache key, w hich can cause Windows SMB clients to observe wrong data (e.g. the length of a file is reported incorrectly). We first noticed this with a C# client program on Windows which created 10,000 files in parallel, each of a different file size (e.g. the first file has size 1 byte, the second has size 2 bytes, etc.), and then checked the file sizes of each of the files one-by-one using Directory.EnumerateFiles() and FileInfo.Length, noting any time that a file does not have the correct expecte d size. This bug is particularly easy to trigger on Ubuntu 18.04 in WSL2 on Windows 10, as it appears to have an effective itime clock resolution (based on observing returned st_ctime and st_mtime from `stat(2)` system calls) of 10ms. In other words, if you can create two files on a WSL2 machine within 10ms of each other, its exceptionally likely that they will be assigned the same iti me and hence have the same SMB file id. However, we have also observed this bug in the wild on Ubuntu 16.04 VMs and Ubuntu 18. 04 VMs running in Azure. Obviously, the better resolution of the clock from stat(2), the less likely this bug is to trigger. We tested a number of different Samba versions and observed the following: | Samba Version | Date Tested | Status | | ------------- | ----------- | ------ | | 4.10.18 | 2021-11-26 | Good | | 4.11.0 | 2021-11-26 | Good | | 4.11.1 | 2021-11-26 | Bad | | 4.11.3 | 2021-11-26 | Bad | | 4.11.7 | 2021-11-26 | Bad | | 4.11.17 | 2021-11-26 | Bad | | 4.13.10 | 2021-11-26 | Bad | | 4.13.11 | 2021-11-26 | Bad | | 4.13.14 | 2021-11-26 | Bad | | 4.14.10 | 2021-11-26 | Bad | | 4.15.2 | 2021-11-26 | Bad | Using git bisect, we traced the bug to the commit 459acf2728aa0c3bc935227998cdc59ead5a2e7c. We can also deliberately trigger the bug by creating multiple files in a directory with identical itimes by manually setting th e user.DOSATTRIB xattr, and observe that these files are served with duplicate SMB file_ids, in violation of the SMB protocol. We wrote a simple patch to temporarily remove itime-based file id generation which has resolved this bug for us: diff --color -Naur samba-4.13.11.orig/source3/modules/vfs_default.c samba-4.13.11/source3/modules/vfs_default.c --- samba-4.13.11.orig/source3/modules/vfs_default.c 2021-08-09 02:20:55.000000000 -0500 +++ samba-4.13.11/source3/modules/vfs_default.c 2021-11-30 15:40:00.089302200 -0600 @@ -3003,10 +3003,6 @@ { uint64_t file_id; - if (!(psbuf->st_ex_iflags & ST_EX_IFLAG_CALCULATED_FILE_ID)) { - return psbuf->st_ex_file_id; - } - if (handle->conn->base_share_dev == psbuf->st_ex_dev) { return (uint64_t)psbuf->st_ex_ino; } We would appreciate any guidance on the correct long-term resolution of this issue. Steven Engelhardt
https://lists.samba.org/archive/samba-technical/2021-December/137005.html
Created attachment 17061 [details] Prototype fix for master. Ralph, what do you think ? Should we parameterize this, and keep the old behavior as default ? If we pass a conn struct to create_random_itime() we can have it select the behavior via a new smb.conf parameter where: fileid = [random|btime] selects between the new 'itime = random uint64_t' NTTIME behavior or the existing 'itime = btime' NTTIME behavior. Set by default to fileid = btime. Thoughts ?
Comment on attachment 17061 [details] Prototype fix for master. This patch passes ci here: https://gitlab.com/samba-team/devel/samba/-/pipelines/431595528
Comment on attachment 17061 [details] Prototype fix for master. I don't think a random 64 bit number is a good idea. The probability of a collision is to high afaict, see https://en.wikipedia.org/wiki/Birthday_attack. I'd much favour somehow using the system's current time.
(In reply to Ralph Böhme from comment #4) Maybe a hires timestamp with a few random bits as high bits?
(In reply to Stefan Metzmacher from comment #5) yes, something like that.
Sounds good to me ! I only wanted to get a proof of concept fix working :-). I think the fix of having a function (I called it create_random_itime(), but only because I was using a random val) called in these places is the right one. As we store itime as an NTTIME in the xattr, then having that function do: void create_itime(struct stat_ex *dst) { NTTIME tval; // generate NTTIME "val" via system clock time + a few random bits. //... itime_val = nt_time_to_full_timespec(tval); update_stat_ex_itime(dst, itime_val); } and then calling it from the 2 open.c paths that create a new file/directory object instead of doing: smb_fname->st.st_ex_iflags &= ~ST_EX_IFLAG_CALCULATED_ITIME; in the caller is the right way to go. That way there are fewer places that need to be aware of the horrendously complex logic around ST_EX_IFLAG_CALCULATED_ITIME (took me a while to understand it :-). Tom is right on the list that we don't do this for the hardlink case either, but they're so rare we've never run into this in practice and I don't intend to start worrying now :-). I'm still trying to start my vacation (I only had to check in to see how my fix was received :-), so I will try not to look at this any more until the New Year :-). Metze, if you want to run with this in the meantime please be my guest !
Hmm. Thinking some more we can ignore the hardlink case. It's just a reference to the same file to the fileid should be identical. Either it already has an itime stored in an xattr from the original create or it doesn't. Either way we don't need to do anything to the new name IMHO.
Try to combine nanoseconds and low-bit inode number in make_file_id_from_itime() but looks like not a good idea, it failed when smbtorture with smb2.fileid: > REASON: Exception: Exception: ../../source4/torture/smb2/create.c:2173: returned_fileid was 10984941285550153256 (0x987259E603BD5E28), expected 10984941285550153245 (0x987259E603BD5E1D): bad fileid Looks like the stream's File-ID is not the same as the base file after creating stream. https://gitlab.com/samba-team/devel/samba/-/pipelines/437152282 @@ -103,6 +103,12 @@ uint64_t make_file_id_from_itime(const struct stat_ex *st) * inode number. */ file_id_low = ino & ((1 << 30) - 1); + } else { + /* + * Combine nanoseconds and low-bit inode number. + */ + file_id_low &= ~((((uint64_t)1) << 8) - 1); + file_id_low |= ino & ((((uint64_t)1) << 8) - 1); } /*
(In reply to Jones Syue from comment #9) You're doing it in the wrong place. The idea would be to modify the value that gets stored on disk as itime in the DOS attributes xattr.
(In reply to Ralph Böhme from comment #10) Ohh okay, now i get it! Thanks! This makes sense and helps me understand why Jeremy's patch could pass smbtorture.
(In reply to Ralph Böhme from comment #10) And this idea is only applied to newly created files, is my understanding correct? If my linux box already have two files with the same itime saved into DOSATTRIB, looks i might need to delete them, or manually revised the itime in DOSATTTIB, or perhaps find a new path to generate them again. This is my case: Altaro Backup to my linux box has this issue too. SMB client is windows server 2016 with Altaro Backup software installed. linux box is aarch64 cpu, and linux kernel 4.2.8, ext4 file system, Inode size (tune2fs -l) is 256, so the timestamp granularity could reach 1 nanoseconds, after linux box upgraded samba from samba-4.10.18 to samba-4.13.12, Altaro Backup job failed to save data to linux box, the error message said: > The operation cannot proceed as the file already exists: > \\${linux_box_ip}\Web\vm\AltaroV7\d887b242-db03-4e41-8939-eb3619a62971\Data\D001.altrdf (Error code 'DEDUP_001') Windows server 2016 'fsutil' command: two files are with the same QFid: 0x986a6fa6454d060c > PS C:\> fsutil file queryfileid z:\vm\AltaroV7\d887b242-db03-4e41-8939-eb3619a62971\Locks\BlockSizeParameters.dat.lock > File ID is 0x986a6fa6454d060c > PS C:\> fsutil file queryfileid z:\vm\AltaroV7\d887b242-db03-4e41-8939-eb3619a62971\BlockSizeParameters.dat > File ID is 0x986a6fa6454d060c linux box log.smbd, two files are with the sambe file id: 986a6fa6454d060c > [2021/12/13 16:09:12.837804, 10, pid=5843, effective(0, 0), real(0, 0)] ../../source3/smbd/dosmode.c:343(parse_dos_attribute_blob) > parse_dos_attribute_blob: file [vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/Locks/BlockSizeParameters.dat.lock] itime [Fri Dec 3 02:52:09 PM 2021 CST] fileid [986a6fa6454d060c] > [2021/12/13 16:09:12.842265, 10, pid=5843, effective(0, 0), real(0, 0)] ../../source3/smbd/dosmode.c:343(parse_dos_attribute_blob) > parse_dos_attribute_blob: file [vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/BlockSizeParameters.dat] itime [Fri Dec 3 02:52:09 PM 2021 CST] fileid [986a6fa6454d060c] linux box 'stat' command, two files are with same 'Change time': 14:52:09.098933941 > <0> # stat --printf "Inode: %i\nAccess: %x\nModify: %y\nChange: %z\nBirth: %w\n" /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/Locks/BlockSizeParameters.dat.lock > Inode: 49020957 > Access: 2021-12-27 01:00:06.871036664 +0800 > Modify: 2021-12-03 14:52:09.116637141 +0800 > Change: 2021-12-03 14:52:09.098933941 +0800 > Birth: - > <0> # stat --printf "Inode: %i\nAccess: %x\nModify: %y\nChange: %z\nBirth: %w\n" /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/BlockSizeParameters.dat > Inode: 49020958 > Access: 2021-12-27 01:00:06.911036664 +0800 > Modify: 2021-12-03 14:52:09.113978421 +0800 > Change: 2021-12-03 14:52:09.098933941 +0800 > Birth: - linux box 'getfattr' command, two files are with the same user.DOSATTRIB. > <0> # getfattr -d /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/Locks/BlockSizeParameters.dat.lock 2>/dev/nul | grep DOS > user.DOSATTRIB=0sAAAEAAQAAABRAAAAIAAAAHsEsUoS6NcBewSxShLo1wE= > <0> # getfattr -d /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/BlockSizeParameters.dat 2>/dev/nul | grep DOS > user.DOSATTRIB=0sAAAEAAQAAABRAAAAIAAAAHsEsUoS6NcBewSxShLo1wE= linux box 'samba-tool' command, two files are with the itime. > <0> # samba-tool ntacl getdosinfo /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/Locks/BlockSizeParameters.dat.lock |grep -i itime > 1: XATTR_DOSINFO_ITIME > itime : Fri Dec 3 14:52:09 2021 CST > <0> # samba-tool ntacl getdosinfo /share/Web/vm/AltaroV7/d887b242-db03-4e41-8939-eb3619a62971/BlockSizeParameters.dat |grep -i itime > 1: XATTR_DOSINFO_ITIME > itime : Fri Dec 3 14:52:09 2021 CST
Created attachment 17070 [details] git-am fix for master. Testing in gitlab ci now.
Passes ci. MR is: https://gitlab.com/samba-team/samba/-/merge_requests/2316
(In reply to Jones Syue from comment #12) Jones, can you re-test with the associated fix ? I'm expecting is should fix your issue with the backup (although it won't fix the existing dos attributes you have, only for a new backup).
(In reply to Jeremy Allison from comment #15) Test with uploading 3 files (test1.txt test2.txt test3.txt) to linux box. After applying this patch https://gitlab.com/samba-team/samba/-/commit/4fc4efeb6afd9dcf2661df8a5b01ee1dc112d822 the fileid of new files are the same, hmm it looks like not as expected. PS H:\> remove-item Z:\test* PS H:\> cp H:\test* Z:\ PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "test$i.txt: " + (fsutil.exe file queryfileid Z:\test$i.txt) >> } test1.txt: File ID is 0x826111395c774d9c test2.txt: File ID is 0x826111395c774d9c test3.txt: File ID is 0x826111395c774d9c Before applying this patch, the fileid of new files are different: PS H:\> remove-item Z:\test* PS H:\> cp H:\test* Z:\ PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "test$i.txt: " + (fsutil.exe file queryfileid Z:\test$i.txt) >> } test1.txt: File ID is 0x9875ae34a28358d4 test2.txt: File ID is 0x9875ae34a358f854 test3.txt: File ID is 0x9875ae34a41f5594 PS H:\>
I figured the problem that Jonas reported out. It's the LOW/HIGH constraints (TIME_T_MIN/TIME_T_MAX) when converting from NTTIME to struct timespec. New patch follows - also adds regression test to ensure we are always consistent and never return duplicate fileids.
Created attachment 17071 [details] git-am fix for master.
Created attachment 17073 [details] git-am fix for master. Much cleaner version with enhanced error messages. Jones if you could check this one I'd appreciate it. It's been uploaded to the mr: https://gitlab.com/samba-team/samba/-/merge_requests/2316 and under ci here: https://gitlab.com/samba-team/devel/samba/-/pipelines/443698182 I'll mark as ready to merge once this passes. New regression test shows we are currently returning duplicate fileids even on ext4 if files / directories are being created in a tight loop.
Created attachment 17074 [details] git-am fix for master. Minimal version (3 patches only), that leaves out the cleanup changes to nt_time_to_unix_timespec() and nt_time_to_full_timespec(). They are not needed for this fix, so I'll submit them as a separate master-only cleanup. This will minimize the back-port needed for 4.15.next. MR: https://gitlab.com/samba-team/samba/-/merge_requests/2316 CI: https://gitlab.com/samba-team/devel/samba/-/pipelines/443714881
https://gitlab.com/samba-team/samba/-/merge_requests/2316 passes CI. Ready to merge now.
This bug was referenced in samba master: 30fea0d31117c1a899cd333a9b8a62ba765dbb02 29d69c22a0d945193ce3dac27e1083dbc5c53f03 23fbf0bad0332a0ae0d4dc3c8f6df6e7ec46b88b
Created attachment 17076 [details] git-am fix for 4.15.next. Cherry-picked from master.
> MR: https://gitlab.com/samba-team/samba/-/merge_requests/2316 > CI: https://gitlab.com/samba-team/devel/samba/-/pipelines/443714881 This one works to me, thank you Jeremy! fsutil test: passed. Altaro backup to new share: passed. smbtorutre smb2.fileid & smb2.fileid_unique: passed. Test output: 1. fsutil test 3 files with unique fileid. PS H:\> remove-item Z:\test* PS H:\> cp H:\test* Z:\ PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "test$i.txt: " + (fsutil.exe file queryfileid Z:\test$i.txt) >> } test1.txt: File ID is 0xa6413a1cdace2b3c test2.txt: File ID is 0x9ef6c30047663404 test3.txt: File ID is 0xbe86ab4035d7db94 PS H:\> 2. fsutil test 3 directories with unique fileid. PS H:\> mkdir Z:\dir1 ... <cut> ... PS H:\> mkdir Z:\dir2 ... <cut> ... PS H:\> mkdir Z:\dir3 Directory: Z:\ Mode LastWriteTime Length Name ---- ------------- ------ ---- d----- 1/10/2022 10:51 AM dir3 PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "dir$i " + (fsutil.exe file queryfileid Z:\dir$i) >> } dir1 File ID is 0xb2fd75e167dd1aa4 dir2 File ID is 0xd6cc1ca3497a6e68 dir3 File ID is 0xd7ff228a79d745b8 PS H:\> 3. smbtorture smb2.fileid <0> # smbtorture //localhost/altaro -U user%password smb2.fileid smbtorture 4.13.12 Using seed 1641784566 time: 2022-01-10 03:16:06.140500 progress: 2 test: fileid time: 2022-01-10 03:16:06.141372 time: 2022-01-10 03:16:06.272547 success: fileid test: fileid-dir time: 2022-01-10 03:16:06.272824 time: 2022-01-10 03:16:06.399368 success: fileid-dir <0> # 4. smbtorture smb2.fileid_unique <0> # smbtorture //localhost/altaro -U admin%tina123! smb2.fileid_unique smbtorture 4.13.12 Using seed 1641784633 time: 2022-01-10 03:17:13.942189 progress: 2 test: fileid_unique time: 2022-01-10 03:17:13.944049 time: 2022-01-10 03:17:14.320354 success: fileid_unique test: fileid_unique-dir time: 2022-01-10 03:17:14.320419 time: 2022-01-10 03:17:14.859104 success: fileid_unique-dir <0> #
Have a bit concern about the itime output of 'samba-tool ntacl getdosinfo', after patch the year of itime is '14300', '25717', and '8363', before patch the year of itime is '2022'. Looks like itime depends on the random result after patch, and itime has 3 different comment made me a bit hard to read the code logic: instantiation time, imaginary time, and invented time, should we use a consistent comment 'ivented time' for 'itime', which is based on c190f3efa9eb4f633df28074b481ff884b67e65f > * we set the itime (aka invented time) that get's stored in the DOS CI is in progress: https://gitlab.com/samba-team/devel/samba/-/pipelines/444503054 After patch: PS H:\> remove-item Z:\test* PS H:\> cp H:\test* Z:\ PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "test$i.txt: " + (fsutil.exe file queryfileid Z:\test$i.txt) >> } test1.txt: File ID is 0xa6413a1cdace2b3c test2.txt: File ID is 0x9ef6c30047663404 test3.txt: File ID is 0xbe86ab4035d7db94 PS H:\> <0> # samba-tool ntacl getdosinfo /share/altaro/test1.txt 2>/dev/null | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Tue Jul 17 23:25:39 14300 CST <0> # samba-tool ntacl getdosinfo /share/altaro/test2.txt 2>/dev/null | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Thu Aug 12 15:08:49 25717 CST <0> # samba-tool ntacl getdosinfo /share/altaro/test3.txt 2>/dev/null | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Sun Sep 1 03:44:01 8363 CST <0> # Before patch: PS H:\> cp H:\test* Z:\ PS H:\> for ( $i=1; $i -le 3; $i++) >> { >> "test$i.txt: " + (fsutil.exe file queryfileid Z:\test$i.txt) >> } test1.txt: File ID is 0x9876fad88d91a19c test2.txt: File ID is 0x9876fad88e48bc9c test3.txt: File ID is 0x9876fad88e76835c PS H:\> <0> # samba-tool ntacl getdosinfo test1.txt | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Mon Jan 10 16:16:34 2022 CST <0> # samba-tool ntacl getdosinfo test2.txt | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Mon Jan 10 16:16:34 2022 CST <0> # samba-tool ntacl getdosinfo test3.txt | grep -i itime 1: XATTR_DOSINFO_ITIME itime : Mon Jan 10 16:16:34 2022 CST <0> #
(In reply to Jones Syue from comment #25) CI is passed: https://gitlab.com/samba-team/devel/samba/-/pipelines/444503054
> after patch the year of itime is '14300', '25717', and '8363' That's because it's not really a time, even though it's stored as such. You really have to think of it as a fileid value. We can't do anything about making it a "normal" time - that was the core of the problem - the time granularity in filesystem timstamps isn't good enough to make unique fileid values.
Comment on attachment 17076 [details] git-am fix for 4.15.next. Withdrawing review request until CLOCK_REALTIME change goes in.
(In reply to Jones Syue from comment #25) Ah, now I see. Yes, it's a good idea to use a consistent name for this, and "invented" time is a good name for it ! I'll add this to the follow-up patch.
Created attachment 17077 [details] git-am fix for 4.15.next.
Created attachment 17078 [details] git-am fix for 4.14.next. Back-port (only test needed backporting) for 4.14.next.
Re-assigning to Jule for inclusion in 4.15.next, 4.14.next.
Pushed to autobuild-v4-{15,14}-test.
This bug was referenced in samba v4-14-test: 9f0353b2f4c3222cc326c605701b448db5895b8a a0934daa711ce51d0e5e6d369eddac4926af6094 7841b45a7f12f1ca241d8d4911e96cee4bf099f0 15599f3390908941a2a184eabd36ca902bfbd823 70d81ab1481580e821d3fc69e7f0e9683cc5177e
This bug was referenced in samba v4-15-test: 032df88d61d569b60f311c7447521759ac7c0735 263aeea95d8df206250ee40ab2a539169618eb1f b48e5c61aaf9c81c38f5873c22724cfc911c66ca 85941fe0cd10a01aa82d2a5a977e25c636a6b84a a43ad2777e3e7997faab2a8a37d168a4db559815
Closing out bug report. Thanks!
*** Bug 14838 has been marked as a duplicate of this bug. ***
This bug was referenced in samba v4-15-stable (Release samba-4.15.4): 032df88d61d569b60f311c7447521759ac7c0735 263aeea95d8df206250ee40ab2a539169618eb1f b48e5c61aaf9c81c38f5873c22724cfc911c66ca 85941fe0cd10a01aa82d2a5a977e25c636a6b84a a43ad2777e3e7997faab2a8a37d168a4db559815
This bug was referenced in samba v4-14-stable (Release samba-4.14.13): 9f0353b2f4c3222cc326c605701b448db5895b8a a0934daa711ce51d0e5e6d369eddac4926af6094 7841b45a7f12f1ca241d8d4911e96cee4bf099f0 15599f3390908941a2a184eabd36ca902bfbd823 70d81ab1481580e821d3fc69e7f0e9683cc5177e