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

[Miroslav Rezanina]

On Mon, Jun 01, 2015 at 09:58:10AM +0200, Markus Armbruster wrote:
> 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.

Good point. Worth changing.

> 
> >      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.

I have to remember to check qemu-devel before preparing/sending patch. My
"different version already sent" ratio is too high. In this case are
both fixes almost identical.

Mirek
> 
> 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
>