Re: [Qemu-devel] [PATCH] net: fix insecure temporary file creation in SLiRP

[Markus Armbruster]

address@hidden writes:

> From: Miroslav Rezanina <address@hidden>
>
> Qemu's user mode networking stack(-net user) is vulnerable to
> a predictable temporary file creation flaw. This patch uses
> mkdtemp(3) routine to fix it.
>
> Fixes CVE-2015-4037.
>
> Signed-off-by: P J P <address@hidden>
> Signed-off-by: Miroslav Rezanina <address@hidden>
> ---
> [1] http://seclists.org/oss-sec/2015/q2/538
> ---
>  net/slirp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index 0e15cf6..804b095 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -27,6 +27,7 @@
>  
>  #ifndef _WIN32
>  #include <pwd.h>
> +#include <stdlib.h>
>  #include <sys/wait.h>
>  #endif
>  #include "net/net.h"
> @@ -481,9 +482,9 @@ 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];
> +    char smb_dir[] = "/tmp/qemu-smb.XXXXXX",  *tmpdir = NULL;
>      struct passwd *passwd;
>      FILE *f;
>  
> @@ -505,12 +506,11 @@ 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) {
> -        error_report("could not create samba server dir '%s'", s->smb_dir);
> +    if (!(tmpdir = mkdtemp(smb_dir))) {
> +        error_report("could not create samba server dir '%s'", smb_dir);
>          return -1;
>      }
> +    strncpy(s->smb_dir, tmpdir, sizeof(s->smb_dir));

We tend to eschew strncpy() and use our (slightly less bad) pstrcpy().
Recommend to use it here, too.

>      snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir,
>      "smb.conf");
>  
>      f = fopen(smb_conf, "w");

Michael (cc'ed) already posted "[PATCH] slirp: use less predictable
directory name in /tmp for smb config (CVE-2015-4037)"[*].  His patch
clobbers s->smb_dir[] when mkdtemp() fails (missed that in my review),
yours doesn't.

Suggest you guys figure out together which solution you want.

Preferably with strncpy() replaced by pstrcpy():
Reviewed-by: Markus Armbruster <address@hidden>


[*] http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05767.html