| Message ID | 1433142369-14266-1-git-send-email-mrezanin@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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>
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
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
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. [...]
> 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
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");