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

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

[mrezanin]

From: Miroslav Rezanina <mrezanin@redhat.com>

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 <pjp@fedoraproject.org>
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
[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));
     snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
 
     f = fopen(smb_conf, "w");
-- 
2.4.1

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

[Markus Armbruster]

mrezanin@redhat.com writes:

> From: Miroslav Rezanina <mrezanin@redhat.com>
>
> 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 <pjp@fedoraproject.org>
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
> [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 <armbru@redhat.com>


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

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:
> mrezanin@redhat.com writes:
> 
> > From: Miroslav Rezanina <mrezanin@redhat.com>
> >
> > 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 <pjp@fedoraproject.org>
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > ---
> > [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 <armbru@redhat.com>
> 
> 
> [*] http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05767.html
> 

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

[Paolo Bonzini]

On 01/06/2015 09:58, 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) {
>> > -        error_report("could not create samba server dir '%s'", s->smb_dir);
>> > +    if (!(tmpdir = mkdtemp(smb_dir))) {

mkdtemp is unfortunately missing on Windows, and g_mkdtemp requires glib
2.30.

Paolo

>> > +        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 <armbru@redhat.com>

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

[Michael Tokarev]

01.06.2015 11:47, Paolo Bonzini пишет:
> 
> 
> On 01/06/2015 09:58, 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) {
>>>> -        error_report("could not create samba server dir '%s'", s->smb_dir);
>>>> +    if (!(tmpdir = mkdtemp(smb_dir))) {
> 
> mkdtemp is unfortunately missing on Windows, and g_mkdtemp requires glib
> 2.30.

This code is within #ifndef WIN32 block.

/mjt

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

[P J P]

  Hello Markus,

> On Monday, 1 June 2015 1:28 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 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.


Thank you so much for the review. IMO using separate smb_dir[] is prudent than s->smb_dir.


> Preferably with strncpy() replaced by pstrcpy():

  Yes.

Thank you.

---
Regards
   -P J P
http://feedmug.com

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

[Markus Armbruster]

P J P <pjp@fedoraproject.org> writes:

>   Hello Markus,
>
>> On Monday, 1 June 2015 1:28 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> 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.
>
>
> Thank you so much for the review. IMO using separate smb_dir[] is
> prudent than s->smb_dir.

Let's go with Michael's v2, because it also fixes the "cleanup after
mkdir() / mkdtemp() failed" scenario.

[...]

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

[P J P]

> On Wednesday, 3 June 2015 4:33 PM, Markus Armbruster wrote:
> Let's go with Michael's v2, because it also fixes the "cleanup
> after mkdir() / mkdtemp() failed" scenario.



  -> https://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg00982.html


Ah yes, looks concise. Thank you.

---
Regards
   -P J P
http://feedmug.com