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.
Status: RESOLVED FIXED
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
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-23 19:49 UTC by Jeremy Allison
Modified: 2019-06-20 09:50 UTC (History)
2 users (show)

See Also:


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

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'
[global]
log file=/usr/local/samba/var/log.%m
log level=5

[data]
path = /srv/data
readonly = no
dfree command = /usr/local/samba/bin/dfree
EOF
---

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

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

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 '\\172.17.0.2\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.

Alternative:

---
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 //172.17.0.2/data /mnt
Password for johndoe@//172.17.0.2/data: …
$ 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
line'
arg[1] = "new
line"
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.

Thanks!