Bug 15419 - weird filename can cause assert to fail in openat_pathref_fsp_nosymlink()
Summary: weird filename can cause assert to fail in openat_pathref_fsp_nosymlink()
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
Depends on:
Reported: 2023-07-13 10:59 UTC by Robert Morris
Modified: 2023-09-27 08:15 UTC (History)
1 user (show)

See Also:

program to reproduce (7.87 KB, text/x-csrc)
2023-07-13 20:23 UTC, Robert Morris
no flags Details
Patch for master. (1.74 KB, patch)
2023-07-14 06:24 UTC, Jeremy Allison
no flags Details
Patch for master (1.77 KB, patch)
2023-07-14 06:33 UTC, Jeremy Allison
no flags Details
git-am fix for 4.18.next (7.63 KB, patch)
2023-07-27 18:40 UTC, Jeremy Allison
slow: review+
git-am fix for 4.17.next (2.35 KB, patch)
2023-07-27 18:45 UTC, Jeremy Allison
slow: review+
git-am fix for 4.18.next (7.66 KB, patch)
2023-08-18 16:46 UTC, Jeremy Allison
slow: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2023-07-13 10:59:50 UTC
If a client with dialect "NT LM 0.12" sends an SMB_COM_CHECK_DIRECTORY
with SMB_FLAGS2_DFS and this path:


(with C string escaping, "\\x//\\/"), then this assert fails in
openat_pathref_fsp_nosymlink() in source3/smbd/files.c:

                 * Path sanitizing further up has cleaned or rejected
                 * empty path components. Assert this here.
                SMB_ASSERT(rel_fname.base_name[0] != '\0');

The function is called with path_in = "/", and its call to
path_to_strv() yields empty components.

This is in the git source as of a few days ago.
Comment 1 Jeremy Allison 2023-07-13 16:59:46 UTC
Do you have a test client that can easily reproduce this ? Or did you use smbclient ?
Comment 2 Ralph Böhme 2023-07-13 17:14:11 UTC
(In reply to Jeremy Allison from comment #1)
Also, can you run this against Windows to see what it returns?
Comment 3 Robert Morris 2023-07-13 20:23:03 UTC
Created attachment 17980 [details]
program to reproduce
Comment 4 Robert Morris 2023-07-13 20:25:25 UTC
(In reply to Jeremy Allison from comment #1)
I've attached a client program that, on my setup, triggers the problem.

However, I suspect it may take a lot of work to turn my client code
into something that works for others.
Comment 5 Robert Morris 2023-07-13 20:25:50 UTC
(In reply to Ralph Böhme from comment #2)
I'm afraid I don't have access to a Windows setup.
Comment 6 Jeremy Allison 2023-07-14 03:04:38 UTC
(In reply to Robert Morris from comment #4)

Well it compiles easily enough :-). So I should be able to run this against a local Samba instance with no security once I get back from Portland next week.

Thanks for sending this in !
Comment 7 Jeremy Allison 2023-07-14 05:08:53 UTC
Ah. Found the problem. You're using our SMB1 DFS path code processing...
Comment 8 Jeremy Allison 2023-07-14 06:24:54 UTC
Created attachment 17981 [details]
Patch for master.

Clever attack. It uses the fact that SMB1 DFS processing swallows the "server" and "share" components without looking at them - separated by '\\' characters, and the fact that our SMB1 code is designed to convert '\\' DFS separators into '/' before passing them to the "strip  DFS server and share components" code.

That allows you to tunnel UNIX path components inside the SMB1 DFS server and share names, and because we only remove the first 2 matching components starting with the '/' character, any others you can add get passed into the pathname walk functions without them going through check_path_syntax() first.

This won't work against SMB2 as we explicitly only look at and remove the first two '\\' separated components, and any '/' characters hidden in the server and sharenames will get removed with them. The rest always gets passed to check_path_syntax() explicitly.

This patch fixes the problem for me. I'll work on adding a regression test to our SMB1 DFS test code.
Comment 9 Jeremy Allison 2023-07-14 06:27:58 UTC
Actually I think I'm going to harden this some more by converting both '/' and '\\' characters in the server and sharename components to '_''s, just in case I've missed a code path where we convert '\\' to '/' on the full passed in client path.

As the server and share components of the DFS path are ignored, it won't matter to any valid codepaths.
Comment 10 Jeremy Allison 2023-07-14 06:33:26 UTC
Created attachment 17982 [details]
Patch for master

Converts both '/' and '\\' characters in servername and sharename for SMB1 DFS paths to '_' characters.
Comment 11 Jeremy Allison 2023-07-14 18:07:37 UTC
Robert, can you confirm this patch fixes the issue for you ? Thanks !
Comment 12 Robert Morris 2023-07-14 19:38:13 UTC
(In reply to Jeremy Allison from comment #11)
Yes, the patch fixes the problem for me.
Comment 13 Jeremy Allison 2023-07-26 23:48:16 UTC
Got a build with a regression test cooking in gitlab. I'll update with the MR if it passes ci.
Comment 15 Samba QA Contact 2023-07-27 10:53:04 UTC
This bug was referenced in samba master:

Comment 16 Jeremy Allison 2023-07-27 18:40:43 UTC
Created attachment 18004 [details]
git-am fix for 4.18.next

Cherry-picked from master.
Comment 17 Jeremy Allison 2023-07-27 18:45:00 UTC
Created attachment 18005 [details]
git-am fix for 4.17.next

Back-ported from master. NB. The SMB1 DFS test infrastructure doesn't exist in 4.17 so this is just the server fix code.
Comment 18 Samba QA Contact 2023-07-28 12:14:52 UTC
This bug was referenced in samba v4-19-test:

Comment 19 Samba QA Contact 2023-07-28 12:18:06 UTC
This bug was referenced in samba v4-19-stable (Release samba-4.19.0rc1):

Comment 20 Jeremy Allison 2023-08-11 20:32:07 UTC
Jule, can we get the 4.18 and 4.17 patches into the next releases ?

Thanks !

Comment 21 Jule Anger 2023-08-14 07:29:33 UTC
Pushed to autobuild-v4-{18,17}-test.
Comment 22 Samba QA Contact 2023-08-14 09:28:03 UTC
This bug was referenced in samba v4-17-test:

Comment 23 Jeremy Allison 2023-08-16 17:08:57 UTC
Hi Jule, I don't see this one in 4.18.6. It did get into the v4-17-test branch, just not the v4-18-test branch.

Can we get it in for the 4.18.7 release please ?
Comment 24 Jule Anger 2023-08-18 12:03:15 UTC
The autobuild for 4.18 failed several times for tasks samba-fileserver and samba-no-opath2: 

UNEXPECTED(failure): samba3.smbtorture_s3.smb1.SMB1-DFS-BADPATH.smbtorture(fileserver_smb1)
REASON: Exception: Exception: using seed 1692358048
host= share=msdfs-pathname-share user=janger myname=fileserversmb1
../../source3/torture/test_smb1_dfs.c:3829 SMB1chkpath of \x//\/ failed (NT_STATUS_OBJECT_PATH_NOT_FOUND)
SMB1-DFS-BADPATH took 0.025272 secs

Sorry for not informing you sooner.
Comment 25 Jeremy Allison 2023-08-18 16:46:48 UTC
Created attachment 18064 [details]
git-am fix for 4.18.next

Looks like for this specific non-functional DFS path "\\x//\\/" in SMB1 the 4.18.x SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND instead of NT_STATUS_OK (as in 4.19 and master).

Easiest thing is to change the test to look for NT_STATUS_OBJECT_PATH_NOT_FOUND instead of NT_STATUS_OK, as this is a DFS path that should map to "\X_\_" rather than trying to change the SMB1 server behavior.

We're only checking that a path contining '/' characters doesn't get through the DFS \server\share checks anyway.

Sorry Jule for not checking earlier.
Comment 26 Jeremy Allison 2023-08-18 20:20:02 UTC
(In reply to Jeremy Allison from comment #25)

It also explains why the 4.17 fix wasn't affected (I'm sure that smbd returns NT_STATUS_OBJECT_PATH_NOT_FOUND in this case also) - the test wasn't back-ported for that release as the test framework isn't there :-).

As I said, it's a corner case - in 4.17,4.18 I think the server checks the \server\share names are correct for the actual server of the DFS share and so returns NT_STATUS_OBJECT_PATH_NOT_FOUND.

4.19,master follow the Windows server behavior in that *any* strings are allowed in the \SERVER and \SHARE DFS name components so it doesn't check them and so \BAD\NAME just maps to root of share (an empty "" pathname).
Comment 27 Jeremy Allison 2023-08-30 18:58:35 UTC
Comment on attachment 18064 [details]
git-am fix for 4.18.next

Ping Ralph, can I get this reviewed for 4.18.next ?
Comment 28 Ralph Böhme 2023-08-30 19:22:11 UTC
Reassigning to Jule for inclusion in 4.18.
Comment 29 Samba QA Contact 2023-08-31 09:39:04 UTC
This bug was referenced in samba v4-18-test:

Comment 30 Jule Anger 2023-08-31 13:35:30 UTC
Closing out bug report.

Comment 31 Samba QA Contact 2023-09-07 09:03:32 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.11):

Comment 32 Samba QA Contact 2023-09-27 08:15:20 UTC
This bug was referenced in samba v4-18-stable (Release samba-4.18.7):