Re: [Qemu-devel] [PATCH v2] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

2015-06-07 Thread Miroslav Rezanina
On Tue, Jun 02, 2015 at 08:46:35AM +0300, Michael Tokarev wrote:
 In this version I used mkdtemp(3) which is:
 
 _BSD_SOURCE
 || /* Since glibc 2.10: */
 (_POSIX_C_SOURCE = 200809L || _XOPEN_SOURCE = 700)
 
 (POSIX.1-2008), so should be available on systems we care about.
 
 While at it, reset the resulting directory name within smb structure
 on error so cleanup function wont try to remove directory which we
 failed to create.
 
 Signed-off-by: Michael Tokarev m...@tls.msk.ru
 ---
 v2:
 Add resetting of the dirname on failure so that cleanup function
 does not try to remove directory which we failed to create.
 
 Use snprintf() as was in the original code, not strcpy(): while
 in this very case it does not matter at all since both strings
 are of known size, some people dislike strcpy() in principle.
 
  net/slirp.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)
 
 diff --git a/net/slirp.c b/net/slirp.c
 index 0e15cf6..3533837 100644
 --- a/net/slirp.c
 +++ b/net/slirp.c
 @@ -481,7 +481,6 @@ 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];
  struct passwd *passwd;
 @@ -505,10 +504,10 @@ 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) {
 +snprintf(s-smb_dir, sizeof(s-smb_dir), /tmp/qemu-smb.XX);
 +if (!mkdtemp(s-smb_dir)) {
  error_report(could not create samba server dir '%s', s-smb_dir);
 +s-smb_dir[0] = 0;
  return -1;
  }
  snprintf(smb_conf, sizeof(smb_conf), %s/%s, s-smb_dir, smb.conf);
 -- 
 2.1.4
 
 
Reviewed-by: Miroslav Rezanina mreza...@redhat.com



Re: [Qemu-devel] [PATCH v2] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)

2015-06-03 Thread Markus Armbruster
Michael Tokarev m...@tls.msk.ru writes:

 In this version I used mkdtemp(3) which is:

 _BSD_SOURCE
 || /* Since glibc 2.10: */
 (_POSIX_C_SOURCE = 200809L || _XOPEN_SOURCE = 700)

 (POSIX.1-2008), so should be available on systems we care about.

 While at it, reset the resulting directory name within smb structure
 on error so cleanup function wont try to remove directory which we
 failed to create.

 Signed-off-by: Michael Tokarev m...@tls.msk.ru
 ---
 v2:
 Add resetting of the dirname on failure so that cleanup function
 does not try to remove directory which we failed to create.

 Use snprintf() as was in the original code, not strcpy(): while
 in this very case it does not matter at all since both strings
 are of known size, some people dislike strcpy() in principle.

I guess I would've used pstrcpy(), but your use of snprintf() is just
fine.

Reviewed-by: Markus Armbruster arm...@redhat.com