Bug 13964 - smbd does not correctly parse arguments passed to dfree and quota scripts.
Summary: smbd does not correctly parse arguments passed to dfree and quota scripts.
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: Karolin Seeger
QA Contact: Samba QA Contact
Depends on:
Reported: 2019-05-23 19:49 UTC by Jeremy Allison
Modified: 2019-06-20 09:50 UTC (History)
2 users (show)

See Also:

git-am fix for 4.10.next (46.99 KB, patch)
2019-05-24 21:09 UTC, Jeremy Allison
slow: review+
git-am fix for 4.9.next (46.96 KB, patch)
2019-05-24 21:10 UTC, Jeremy Allison
slow: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2019-05-23 19:49:41 UTC
As pointed out by Michael Hanselmann <public@hansmi.ch> on the samba-technical mailing list, the sample dfree scripts in the smb.conf man page don't properly quote their arguments.

smbd also mis-parses a pathname containing spaces and passes the incorrect
This allows a pathname containing spaces to be miss-parsed within smbd.

This is not a security issue as no scripts are invoked by smbd by default, plus mis-parsing the pathname does not allow for any exploits.

Patch to follow hardens the command line script processing code for all uses with smbd and winbindd, and also fixes the man pages to correctly quote their arguments.
Comment 1 hansmi 2019-05-23 22:18:55 UTC
My original report:

Samba 4.x supports a configuration option named "dfree command" (no default),
used by the "source3/smbd/dfree.c:sys_disk_free" function. When configured the
command is invoked to determine the available disk space for a given path, but
unfortunately the command arguments aren't split properly, allowing the
injection of arguments and leaking information.

Example smb.conf:

$ cat >/usr/local/samba/etc/smb.conf <<'EOF'
log file=/usr/local/samba/var/log.%m
log level=5

path = /srv/data
readonly = no
dfree command = /usr/local/samba/bin/dfree

dfree script as proposed in smb.conf(5):

$ cat >/usr/local/samba/bin/dfree <<'EOF' && chmod +x /usr/local/samba/bin/dfree
df $1 | tail -1 | awk '{print $(NF-4),$(NF-2)}'

Note how the argument to "df" isn't quoted.

For an easier investigation we can patch Samba slightly:

--- a/lib/util/sys_popen.c
+++ b/lib/util/sys_popen.c
@@ -81,6 +81,7 @@ static char **extract_args(TALLOC_CTX *mem_ctx, const char *command)
                if (!(argl[i++] = talloc_strdup(argl, ptr))) {
                        goto nomem;
+               printf("arg[%d] = \"%s\"\n", i - 1, argl[i - 1]);

        argl[i++] = NULL;

Use smbclient to connect. Example command:

$ bin/smbclient -U johndoe '\\\data'

Create directories:

smb: \> mkdir " "
smb: \> mkdir " \bin"
smb: \> mkdir " \bin\ls"
smb: \> mkdir " \bin\ls\foo"
smb: \> cd " \bin\ls"
smb: \ \bin\ls\> dir
  28703652 blocks of size 1024. 16285184 blocks available
smb: \ \bin\ls\>

Log output on server:

disk_free: Running command '/usr/local/samba/bin/dfree  /bin/ls'
arg[1] = "/bin/ls"

Information leak: The resulting free space information is for the filesystem
behind "/bin", not the filesystem underlying the share.


smb: \> cd "\ \bin\ls\foo"
smb: \ \bin\ls\foo\> dir

Log output:

disk_free: Running command '/usr/local/samba/bin/dfree  /bin/ls/foo'
arg[1] = "/bin/ls/foo"
df: /bin/ls/foo: Not a directory
disk_free: file_lines_load() failed for command '/usr/local/samba/bin/dfree  /bin/ls/foo'. Error was : No child processes

This case will fall back to the built-in code determining free disk space. The
difference in the response indicates to a determined attacker whether a file or
directory exists.

Remember how I highlighted the non-quoted argument to "df"?

smb: \> mkdir "--version bar"
smb: \> cd "--version bar"
smb: \--version bar\> dir

The result is an argument injection (fortunately shell metacharacters aren't
evaluated with the example script):

disk_free: Running command '/usr/local/samba/bin/dfree --version bar'
arg[1] = "--version"
arg[2] = "bar"
Read input from dfree, "David and"
Parsed output of dfree, dsize=4294967295, dfree=4294967295, bsize=4294967295

Also note how the "dfree" script is actually given two arguments.

We can also mount the share and create a somewhat unusual directory name with
a newline character:

$ mount -t cifs -o username=johndoe // /mnt
Password for johndoe@// …
$ mkdir /mnt/$'new\nline'

List directory contents via smbclient (directory name will vary):

smb: \> cd NQ7DQR~Z\
smb: \NQ7DQR~Z\> dir

Note how Samba and the shell disagree on how to split the command (Samba sees
one, the shell two arguments):

disk_free: Running command '/usr/local/samba/bin/dfree new
arg[1] = "new
df: new: No such file or directory
df: line: No such file or directory
disk_free: file_lines_load() failed for command '/usr/local/samba/bin/dfree new
line'. Error was : No child processes

Proposal: Build the command arguments as an array instead of a string which is
then split up again.
Comment 2 Jeremy Allison 2019-05-24 21:09:34 UTC
Created attachment 15189 [details]
git-am fix for 4.10.next

Cherry-picked and back-ported from master.
Comment 3 Jeremy Allison 2019-05-24 21:10:06 UTC
Created attachment 15190 [details]
git-am fix for 4.9.next

Cherry-picked and back-ported from master.
Comment 4 Ralph Böhme 2019-05-27 12:53:14 UTC
Reassigning to Karolin for inclusion in 4.9 and 4.10.
Comment 5 Karolin Seeger 2019-06-04 09:47:23 UTC
(In reply to Ralph Böhme from comment #4)
Pushed to autobuild-v4-{9,10}-test.
Comment 6 Karolin Seeger 2019-06-20 09:50:19 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to both branches.
Closing out bug report.