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

[Paolo Bonzini <pbonzini@redhat.com>]

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>