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

[Qemu-devel] [PATCH v2] 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)

(POSIX.1-2008), so should be available on systems we care about.

While at it, reset the resulting directory name within smb structure
on error so cleanup function wont try to remove directory which we
failed to create.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
v2:
Add resetting of the dirname on failure so that cleanup function
does not try to remove directory which we failed to create.

Use snprintf() as was in the original code, not strcpy(): while
in this very case it does not matter at all since both strings
are of known size, some people dislike strcpy() in principle.

 net/slirp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 0e15cf6..3533837 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,10 +504,10 @@ 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) {
+    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.XXXXXX");
+    if (!mkdtemp(s->smb_dir)) {
         error_report("could not create samba server dir '%s'", s->smb_dir);
+        s->smb_dir[0] = 0;
         return -1;
     }
     snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2] 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)
>
> (POSIX.1-2008), so should be available on systems we care about.
>
> While at it, reset the resulting directory name within smb structure
> on error so cleanup function wont try to remove directory which we
> failed to create.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v2:
> Add resetting of the dirname on failure so that cleanup function
> does not try to remove directory which we failed to create.
>
> Use snprintf() as was in the original code, not strcpy(): while
> in this very case it does not matter at all since both strings
> are of known size, some people dislike strcpy() in principle.

I guess I would've used pstrcpy(), but your use of snprintf() is just
fine.

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

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

[Miroslav Rezanina]

On Tue, Jun 02, 2015 at 08:46:35AM +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)
> 
> (POSIX.1-2008), so should be available on systems we care about.
> 
> While at it, reset the resulting directory name within smb structure
> on error so cleanup function wont try to remove directory which we
> failed to create.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v2:
> Add resetting of the dirname on failure so that cleanup function
> does not try to remove directory which we failed to create.
> 
> Use snprintf() as was in the original code, not strcpy(): while
> in this very case it does not matter at all since both strings
> are of known size, some people dislike strcpy() in principle.
> 
>  net/slirp.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index 0e15cf6..3533837 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,10 +504,10 @@ 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) {
> +    snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.XXXXXX");
> +    if (!mkdtemp(s->smb_dir)) {
>          error_report("could not create samba server dir '%s'", s->smb_dir);
> +        s->smb_dir[0] = 0;
>          return -1;
>      }
>      snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
> -- 
> 2.1.4
> 
> 
Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>