[Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Michael Tokarev]

In this version I used mkdtemp(3) which is:

        _BSD_SOURCE
        || /* Since glibc 2.10: */
            (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700)

so should be available on systems we care about.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 net/slirp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 9bbed74..0ad32ad 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s)
 static int slirp_smb(SlirpState* s, const char *exported_dir,
                      struct in_addr vserver_addr)
 {
-    static int instance;
     char smb_conf[128];
     char smb_cmdline[128];
     struct passwd *passwd;
@@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
         return -1;
     }
 
-    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
-             (long)getpid(), instance++);
-    if (mkdir(s->smb_dir, 0700) < 0) {
+    strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX");
+    if (!mkdtemp(s->smb_dir)) {
         error_report("could not create samba server dir '%s'", s->smb_dir);
         return -1;
     }
-- 
2.1.4

Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> In this version I used mkdtemp(3) which is:
>
>         _BSD_SOURCE
>         || /* Since glibc 2.10: */
>             (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700)

In short, it's POSIX.1-2008.

> so should be available on systems we care about.

Yes.

> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> In this version I used mkdtemp(3) which is:
>
>         _BSD_SOURCE
>         || /* Since glibc 2.10: */
>             (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700)
>
> so should be available on systems we care about.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  net/slirp.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index 9bbed74..0ad32ad 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s)
>  static int slirp_smb(SlirpState* s, const char *exported_dir,
>                       struct in_addr vserver_addr)
>  {
> -    static int instance;
>      char smb_conf[128];
>      char smb_cmdline[128];
>      struct passwd *passwd;
> @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
>          return -1;
>      }
>  
> -    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
> -             (long)getpid(), instance++);
> -    if (mkdir(s->smb_dir, 0700) < 0) {
> +    strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX");
> +    if (!mkdtemp(s->smb_dir)) {
>          error_report("could not create samba server dir '%s'", s->smb_dir);
>          return -1;
>      }

Uh, I have to suspend my R-by.  This clobbers s->smb_dir[] when
mkdtemp() fails.  Is that safe?  Explain why, and get my R-by back.

The alternative "[PATCH] net: fix insecure temporary file creation in
SLiRP" doesn't clobber it.
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg00005.html

Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Michael Tokarev]

01.06.2015 11:01, Markus Armbruster wrote:
[]
>> -    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
>> -             (long)getpid(), instance++);
>> -    if (mkdir(s->smb_dir, 0700) < 0) {
>> +    strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX");
>> +    if (!mkdtemp(s->smb_dir)) {
>>          error_report("could not create samba server dir '%s'", s->smb_dir);
>>          return -1;
>>      }
> 
> Uh, I have to suspend my R-by.  This clobbers s->smb_dir[] when
> mkdtemp() fails.  Is that safe?  Explain why, and get my R-by back.

At least this is not different from the original behavour.

The only place where this variable is used is in the slirp_smb_cleanup()
function, which, if smb_dir[0] != 0, does rm -rf on it.
I think it will be better to add s->smb_dir[0] = 0 here before return.
On the other hand this isn't really that important given when we can
expect mkdtemp() to fail.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 01.06.2015 11:01, Markus Armbruster wrote:
> []
>>> -    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
>>> -             (long)getpid(), instance++);
>>> -    if (mkdir(s->smb_dir, 0700) < 0) {
>>> +    strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX");
>>> +    if (!mkdtemp(s->smb_dir)) {
>>>          error_report("could not create samba server dir '%s'", s->smb_dir);
>>>          return -1;
>>>      }
>> 
>> Uh, I have to suspend my R-by.  This clobbers s->smb_dir[] when
>> mkdtemp() fails.  Is that safe?  Explain why, and get my R-by back.
>
> At least this is not different from the original behavour.

Fair enough, R-by unsuspended.

> The only place where this variable is used is in the slirp_smb_cleanup()
> function, which, if smb_dir[0] != 0, does rm -rf on it.
> I think it will be better to add s->smb_dir[0] = 0 here before return.

Makes sense.

> On the other hand this isn't really that important given when we can
> expect mkdtemp() to fail.

Not sure I got this part.  Not sure I need to :)

Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

[Miroslav Rezanina]

On Thu, May 28, 2015 at 02:15:43PM +0300, Michael Tokarev wrote:
> In this version I used mkdtemp(3) which is:
> 
>         _BSD_SOURCE
>         || /* Since glibc 2.10: */
>             (_POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700)
> 
> so should be available on systems we care about.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  net/slirp.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index 9bbed74..0ad32ad 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s)
>  static int slirp_smb(SlirpState* s, const char *exported_dir,
>                       struct in_addr vserver_addr)
>  {
> -    static int instance;
>      char smb_conf[128];
>      char smb_cmdline[128];
>      struct passwd *passwd;
> @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
>          return -1;
>      }
>  
> -    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
> -             (long)getpid(), instance++);
> -    if (mkdir(s->smb_dir, 0700) < 0) {
> +    strcpy(s->smb_dir, "/tmp/qemu-smb.XXXXXX");
> +    if (!mkdtemp(s->smb_dir)) {
>          error_report("could not create samba server dir '%s'", s->smb_dir);
>          return -1;
>      }
> -- 
> 2.1.4
> 
> 

I suggest to go with this patch as:

1) It was sent first

2) Is simplier

3) Keep original behavior

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>