[Qemu-devel] [PATCH] security_20040618

[Qemu-devel] [PATCH] security_20040618

[Tim]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Updates to this, from the previous version two days ago, include
additional strcpy replacements, as well as TODO comments pointing out
unhealthy calls to {m,re}alloc, which don't check for NULL return
values. (I am not sure how to handle error return codes in most places,
so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
should behave exactly the same as before.

See attached.
Thanks,
tim

[-- Attachment #2: README --]
[-- Type: text/plain, Size: 1117 bytes --]

This patch contains fixes for calls to potentially unsafe string
manipulation functions.  It is almost certain that some of these updates
patch security vulnerabilities.  Other changes may avert future
vulnerabilities.  In addition, TODO comments were added to certain
sections of code where further checks should be implemented.  This
version of the patch makes changes to the following files in the most
current CVS (as of today): 

block.c
dyngen.c
monitor.c
sparc-dis.c
thunk.c
vl.c
linux-user/elfload.c
linux-user/path.c
linux-user/syscall.c
slirp/debug.c
slirp/mbuf.c
slirp/misc.c
slirp/tcp_subr.c
slirp/udp.c
target-i386/helper2.c

Any original code contained within this patch is hereby released into
the public domain.  All derivative portions of code contained within,
for the purposes of documentation, are a part of QEMU, and are therefore
protected by the copyright of their respective authors.

While care was taken in making these changes, it is possible that this
patch will break something.  Use your own risk.  Please report any
success/failure to:
tim {HYPHEN} qemu {AT} sentinelchicken {DOT} org.

[-- Attachment #3: security.patch --]
[-- Type: text/plain, Size: 13175 bytes --]

--- qemu-current/block.c	2004-06-16 20:49:58.000000000 -0700
+++ qemu-dev/block.c	2004-06-17 22:14:20.000000000 -0700
@@ -92,7 +92,7 @@
     bs->fd = -1;
     bs->cow_fd = -1;
     bs->cow_bitmap = NULL;
-    strcpy(bs->filename, filename);
+    snprintf(bs->filename, 1024, "%s", filename);
 
     /* open standard HD image */
 #ifdef _WIN32
--- qemu-current/dyngen.c	2004-04-04 05:56:28.000000000 -0700
+++ qemu-dev/dyngen.c	2004-06-18 08:33:58.000000000 -0700
@@ -458,6 +458,7 @@
 
     /* read all section data */
     sdata = malloc(sizeof(void *) * ehdr.e_shnum);
+    /* TODO: check for sdata == NULL */
     memset(sdata, 0, sizeof(void *) * ehdr.e_shnum);
     
     for(i = 0;i < ehdr.e_shnum; i++) {
@@ -653,6 +654,7 @@
 	
     /* read all section data */
     sdata = malloc(sizeof(void *) * fhdr.f_nscns);
+    /* TODO: check for sdata == NULL */
     memset(sdata, 0, sizeof(void *) * fhdr.f_nscns);
     
     const char *p;
@@ -698,6 +700,7 @@
 
 	/* set coff symbol */
 	symtab = malloc(sizeof(struct coff_sym) * nb_syms);
+        /* TODO: check for symtab == NULL */
 
 	int aux_size, j;
 	for (i = 0, ext_sym = coff_symtab, sym = symtab; i < nb_syms; i++, ext_sym++, sym++) {
@@ -746,6 +749,7 @@
 
     /* set coff relocation */
     relocs = malloc(sizeof(struct coff_rel) * nb_relocs);
+    /* TODO: check for relocs == NULL */
     for (i = 0, ext_rel = coff_relocs, rel = relocs; i < nb_relocs; 
          i++, ext_rel++, rel++) {
         memset(rel, 0, sizeof(*rel));
--- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
+++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
@@ -1221,7 +1221,7 @@
                     goto fail;
                 }
                 str = qemu_malloc(strlen(buf) + 1);
-                strcpy(str, buf);
+                pstrcpy(str, strlen(buf) + 1,  buf);
                 str_allocated[nb_args] = str;
             add_str:
                 if (nb_args >= MAX_ARGS) {
@@ -1437,7 +1437,7 @@
 static void term_print_cmdline (const char *cmdline)
 {
     term_show_prompt();
-    term_printf(cmdline);
+    term_printf("%s", cmdline);
     term_flush();
 }
 
@@ -1521,7 +1521,7 @@
     }
     term_hist_entry--;
     if (term_hist_entry >= 0) {
-	strcpy(term_cmd_buf, term_history[term_hist_entry]);
+	snprintf(term_cmd_buf, 4096, "%s", term_history[term_hist_entry]);
 	term_printf("\n");
 	term_print_cmdline(term_cmd_buf);
 	term_cmd_buf_index = term_cmd_buf_size = strlen(term_cmd_buf);
@@ -1533,7 +1533,7 @@
     if (term_hist_entry == TERM_MAX_CMDS - 1 || term_hist_entry == -1)
 	return;
     if (term_history[++term_hist_entry] != NULL) {
-	strcpy(term_cmd_buf, term_history[term_hist_entry]);
+	snprintf(term_cmd_buf, 4096, "%s", term_history[term_hist_entry]);
     } else {
 	term_hist_entry = -1;
     }
--- qemu-current/sparc-dis.c	2003-06-09 08:23:31.000000000 -0700
+++ qemu-dev/sparc-dis.c	2004-06-18 09:05:50.000000000 -0700
@@ -2468,6 +2468,7 @@
       if (!opcodes_initialized)
 	sorted_opcodes = (const struct sparc_opcode **)
             malloc (sparc_num_opcodes * sizeof (struct sparc_opcode *));
+      /* TODO: check for sorted_opcodes == NULL */
       /* Reset the sorted table so we can resort it.  */
       for (i = 0; i < sparc_num_opcodes; ++i)
 	sorted_opcodes[i] = &sparc_opcodes[i];
@@ -3183,6 +3184,7 @@
   if (hash_buf != NULL)
     free (hash_buf);
   hash_buf = (struct opcode_hash *) malloc (sizeof (struct opcode_hash) * num_opcodes);
+  /* TODO: check for hash_buf == NULL */
   for (i = num_opcodes - 1; i >= 0; --i)
     {
       register int hash = HASH_INSN (opcode_table[i]->match);
--- qemu-current/thunk.c	2003-06-15 12:52:54.000000000 -0700
+++ qemu-dev/thunk.c	2004-06-18 08:48:05.000000000 -0700
@@ -85,6 +85,7 @@
         offset = 0;
         max_align = 1;
         se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+        /* TODO: check for NULL from malloc() */
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
--- qemu-current/vl.c	2004-06-05 06:46:47.000000000 -0700
+++ qemu-dev/vl.c	2004-06-18 08:58:21.000000000 -0700
@@ -265,19 +265,11 @@
 
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
-    int c;
-    char *q = buf;
-
-    if (buf_size <= 0)
-        return;
-
-    for(;;) {
-        c = *str++;
-        if (c == 0 || q >= buf + buf_size - 1)
-            break;
-        *q++ = c;
+    if(buf_size > 0) {
+        buf_size--;
+        strncpy(buf, str, buf_size);
+        buf[buf_size] = '\0';
     }
-    *q = '\0';
 }
 
 /* strcat and truncate. */
--- qemu-current/linux-user/elfload.c	2004-06-16 20:47:28.000000000 -0700
+++ qemu-dev/linux-user/elfload.c	2004-06-15 19:31:55.000000000 -0700
@@ -1067,7 +1067,7 @@
 	char * passed_p;
 
 	if (interpreter_type == INTERPRETER_AOUT) {
-	    sprintf(passed_fileno, "%d", bprm->fd);
+	    snprintf(passed_fileno, sizeof(passed_fileno), "%d", bprm->fd);
 	    passed_p = passed_fileno;
 
 	    if (elf_interpreter) {
--- qemu-current/linux-user/path.c	2003-04-10 17:15:13.000000000 -0700
+++ qemu-dev/linux-user/path.c	2004-06-18 11:02:32.000000000 -0700
@@ -43,7 +43,9 @@
 				  struct pathelem *parent,
 				  const char *name)
 {
+    /* TODO: is this line safe? */
     struct pathelem *new = malloc(sizeof(*new));
+    /* TODO: check for new == NULL */
     new->name = strdup(name);
     asprintf(&new->pathname, "%s/%s", root, name);
     new->num_entries = 0;
@@ -75,7 +77,7 @@
 
     root = realloc(root, sizeof(*root)
 		   + sizeof(root->entries[0])*root->num_entries);
-
+    /* TODO: check for root == NULL */
     root->entries[root->num_entries-1] = new_entry(root->pathname, root, name);
     root->entries[root->num_entries-1]
 	= add_dir_maybe(root->entries[root->num_entries-1]);
--- qemu-current/linux-user/syscall.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/linux-user/syscall.c	2004-06-18 09:03:45.000000000 -0700
@@ -1409,6 +1409,7 @@
     
     if (flags & CLONE_VM) {
         ts = malloc(sizeof(TaskState) + NEW_STACK_SIZE);
+        /* TODO: check for ts == NULL */
         memset(ts, 0, sizeof(TaskState));
         new_stack = ts->stack;
         ts->used = 1;
@@ -2450,7 +2451,7 @@
 		    tnamelen = treclen - (2 * sizeof(target_long) + 2);
 		    if (tnamelen > 256)
                         tnamelen = 256;
-		    strncpy(tde->d_name, de->d_name, tnamelen);
+		    snprintf(tde->d_name, tnamelen, '%s', de->d_name);
                     de = (struct dirent *)((char *)de + reclen);
                     len -= reclen;
                     tde = (struct dirent *)((char *)tde + treclen);
--- qemu-current/slirp/debug.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/debug.c	2004-06-15 19:40:42.000000000 -0700
@@ -305,7 +305,7 @@
 			
 	for (so = tcb.so_next; so != &tcb; so = so->so_next) {
 		
-		n = sprintf(buff, "tcp[%s]", so->so_tcpcb?tcpstates[so->so_tcpcb->t_state]:"NONE");
+		n = snprintf(buff, sizeof(buff),"tcp[%s]", so->so_tcpcb?tcpstates[so->so_tcpcb->t_state]:"NONE");
 		while (n < 17)
 		   buff[n++] = ' ';
 		buff[17] = 0;
@@ -319,7 +319,7 @@
 		   
 	for (so = udb.so_next; so != &udb; so = so->so_next) {
 		
-		n = sprintf(buff, "udp[%d sec]", (so->so_expire - curtime) / 1000);
+		n = snprintf(buff, sizeof(buff), "udp[%d sec]", (so->so_expire - curtime) / 1000);
 		while (n < 17)
 		   buff[n++] = ' ';
 		buff[17] = 0;
--- qemu-current/slirp/mbuf.c	2004-04-21 17:10:47.000000000 -0700
+++ qemu-dev/slirp/mbuf.c	2004-06-18 11:04:03.000000000 -0700
@@ -152,6 +152,7 @@
         if (m->m_flags & M_EXT) {
 	  /* datasize = m->m_data - m->m_ext; */
 	  m->m_ext = (char *)realloc(m->m_ext,size);
+	  /* TODO: fix? and re-add check for m->m_ext == NULL */
 /*		if (m->m_ext == NULL)
  *			return (struct mbuf *)NULL;
  */		
@@ -161,6 +162,7 @@
 	  char *dat;
 	  datasize = m->m_data - m->m_dat;
 	  dat = (char *)malloc(size);
+          /* TODO: fix? and re-add the dat==NULL check */
 /*		if (dat == NULL)
  *			return (struct mbuf *)NULL;
  */
--- qemu-current/slirp/misc.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/misc.c	2004-06-18 11:04:55.000000000 -0700
@@ -182,6 +182,7 @@
 	
 	tmp_ptr = *ex_ptr;
 	*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
+	/* TODO: check for *ex_ptr == NULL */
 	(*ex_ptr)->ex_fport = port;
 	(*ex_ptr)->ex_addr = addr;
 	(*ex_ptr)->ex_pty = do_pty;
@@ -362,10 +363,10 @@
 		
 		if (x_port >= 0) {
 #ifdef HAVE_SETENV
-			sprintf(buff, "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+			snprintf(buff, sizeof(buff), "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
 			setenv("DISPLAY", buff, 1);
 #else
-			sprintf(buff, "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+			snprintf(buff, sizeof(buff), "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
 			putenv(buff);
 #endif
 		}
@@ -401,7 +402,7 @@
 		  {
 			  char buff[256];
 			  
-			  sprintf(buff, "Error: execvp of %s failed: %s\n", 
+			  snprintf(buff, sizeof(buff), "Error: execvp of %s failed: %s\n", 
 				  argv[0], strerror(errno));
 			  write(2, buff, strlen(buff)+1);
 		  }
@@ -448,6 +449,7 @@
 	char *bptr;
 	
 	bptr = (char *)malloc(strlen(str)+1);
+	/* TODO: check for bptr == NULL */
 	strcpy(bptr, str);
 	
 	return bptr;
@@ -476,7 +478,7 @@
 		sock_in.sin_port = htons(slirp_socket_port);
 		if (connect(s, (struct sockaddr *)&sock_in, sizeof(sock_in)) != 0)
 		   slirp_exit(1); /* just exit...*/
-		sprintf(buff, "kill %s:%d", slirp_socket_passwd, slirp_socket_unit);
+		snprintf(buff, sizeof(buff), "kill %s:%d", slirp_socket_passwd, slirp_socket_unit);
 		write(s, buff, strlen(buff)+1);
 	}
 #ifndef NO_UNIX_SOCKETS
@@ -489,7 +491,7 @@
 		if (connect(s, (struct sockaddr *)&sock_un,
 			      sizeof(sock_un.sun_family) + sizeof(sock_un.sun_path)) != 0)
 		   slirp_exit(1);
-		sprintf(buff, "kill none:%d", slirp_socket_unit);
+		snprintf(buff, sizeof(buff), "kill none:%d", slirp_socket_unit);
 		write(s, buff, strlen(buff)+1);
 	}
 #endif
@@ -615,7 +617,7 @@
 			                        
 			lprint_sb->sb_data = (char *)realloc(lprint_sb->sb_data,
 							     lprint_sb->sb_datalen + TCP_SNDSPACE);
-			
+			/* TODO: check for lprint_sb->sb_data == NULL */
 			/* Adjust all values */
 			lprint_sb->sb_wptr = lprint_sb->sb_data + deltaw;
 			lprint_sb->sb_rptr = lprint_sb->sb_data + deltar;
@@ -715,6 +717,7 @@
 	
 	/* link it */
 	emup = (struct emu_t *)malloc(sizeof (struct emu_t));
+	/* TODO: check for emup == NULL */
 	emup->lport = (u_int16_t)lport;
 	emup->fport = (u_int16_t)fport;
 	emup->tos = tos;
@@ -889,10 +892,10 @@
 		/* Set the DISPLAY */
            if (x_port >= 0) {
 #ifdef HAVE_SETENV
-             sprintf(buff, "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+             snprintf(buff, sizeof(buff), "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
              setenv("DISPLAY", buff, 1);
 #else
-             sprintf(buff, "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+             snprintf(buff, sizeof(buff), "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
              putenv(buff);
 #endif
            }
@@ -907,7 +910,7 @@
            
            /* Ooops, failed, let's tell the user why */
            
-           sprintf(buff, "Error: execlp of %s failed: %s\n", 
+           snprintf(buff, sizeof(buff), "Error: execlp of %s failed: %s\n", 
                    "rsh", strerror(errno));
            write(2, buff, strlen(buff)+1);
            close(0); close(1); close(2); /* XXX */
--- qemu-current/slirp/tcp_subr.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/tcp_subr.c	2004-06-15 19:51:31.000000000 -0700
@@ -731,7 +731,7 @@
 				if (*ptr++ == 0) {
 					n++;
 					if (n == 2) {
-						sprintf(args, "rlogin -l %s %s",
+						snprintf(args, sizeof(args), "rlogin -l %s %s",
 							ptr, inet_ntoa(so->so_faddr));
 					} else if (n == 3) {
 						i2 = so_rcv->sb_wptr - ptr;
@@ -739,9 +739,9 @@
 							if (ptr[i] == '/') {
 								ptr[i] = 0;
 #ifdef HAVE_SETENV
-								sprintf(term, "%s", ptr);
+								snprintf(term, sizeof(term), "%s", ptr);
 #else
-								sprintf(term, "TERM=%s", ptr);
+								snprintf(term, sizeof(term), "TERM=%s", ptr);
 #endif
 								ptr[i] = '/';
 								break;
--- qemu-current/slirp/udp.c	2004-04-21 17:10:48.000000000 -0700
+++ qemu-dev/slirp/udp.c	2004-06-18 10:55:20.000000000 -0700
@@ -493,6 +493,7 @@
 			if (!req) {	/* no entry for so, create new */
 				req = (struct talk_request *)
 					malloc(sizeof(struct talk_request));
+				/* TODO: check for req == NULL */
 				req->udp_so = so;
 				req->tcp_so = solisten(0,		
 					OTOSIN(omsg, addr)->sin_addr.s_addr,	
--- qemu-current/target-i386/helper2.c	2004-05-08 14:05:19.000000000 -0700
+++ qemu-dev/target-i386/helper2.c	2004-06-18 08:56:24.000000000 -0700
@@ -212,7 +212,7 @@
     
     if (flags & X86_DUMP_CCOP) {
         if ((unsigned)env->cc_op < CC_OP_NB)
-            strcpy(cc_op_name, cc_op_str[env->cc_op]);
+            snprintf(cc_op_name, sizeof(cc_op_name), "%s", cc_op_str[env->cc_op]);
         else
             snprintf(cc_op_name, sizeof(cc_op_name), "[%d]", env->cc_op);
         fprintf(f, "CCS=%08x CCD=%08x CCO=%-8s\n",

Re: [Qemu-devel] [PATCH] security_20040618

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Fri, 2004-06-18 at 11:40 -0700, Tim wrote:
> Updates to this, from the previous version two days ago, include
> additional strcpy replacements, as well as TODO comments pointing out
> unhealthy calls to {m,re}alloc, which don't check for NULL return
> values. (I am not sure how to handle error return codes in most places,
> so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
> should behave exactly the same as before.

In dyngen you need to do:

if ( ptr == NULL )
	error("malloc failed");

error() will never return.

For the other places it depends, but it's ususally quite simple. Why not
have a stab and submit a seperate patch on top of this one?

Also - Abother low hanging fruit may be /tmp file races. You could
probably make sure mkstmp is being used where possible etc.. and/or use
of /tmp files elimated as much as possible.... Or try setup a
$(HOME)/.qemu dir for that stuff. I know QEMU_TMPDIR is checked in vl.c
but the standard TMPDIR probably ought to be aswell if we DO use /tmp.

I mean, if root saves log to /tmp/qemu.log any user on the system may
obliterate any file (ln -s /etc/passwrd /tmp/qemu.log) as /tmp is the
default choice, perhaps root should know better, but maybe we should use
sane defaults like $(HOME)/qemu.log.

If people are interested in janitorial stuff like this, please, go right
ahead :)

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> In dyngen you need to do:
> 
> if ( ptr == NULL )
> 	error("malloc failed");
> 
> error() will never return.
>
> For the other places it depends, but it's ususally quite simple. Why not
> have a stab and submit a seperate patch on top of this one?

You would prefer I submit a seperate patch for the malloc fixes?  I
could certainly do that, but I just figured it is all a general code
quality patch, which changes very few lines of code (though in many
files).  What would you prefer Fabrice?
 
> Also - Abother low hanging fruit may be /tmp file races. You could
> probably make sure mkstmp is being used where possible etc.. and/or use
> of /tmp files elimated as much as possible.... Or try setup a
> $(HOME)/.qemu dir for that stuff. I know QEMU_TMPDIR is checked in vl.c
> but the standard TMPDIR probably ought to be aswell if we DO use /tmp.
> 
> I mean, if root saves log to /tmp/qemu.log any user on the system may
> obliterate any file (ln -s /etc/passwrd /tmp/qemu.log) as /tmp is the
> default choice, perhaps root should know better, but maybe we should use
> sane defaults like $(HOME)/qemu.log.

Heh, funny you mention it... that was next on my list.  I noticed the
logging to /tmp a while back, but didn't want to open up that can until
I understood the codebase a little better.

For my purposes, I would like to run qemu as more of a daemon than
anything.  So, I will likely attempt some improvements to the command
line logging options, and pick safe defaults.  I'll try to eliminate
other temp files where possible, and make the others secure against
races.  We'll see, I haven't even looked at that code yet... 

> If people are interested in janitorial stuff like this, please, go right
> ahead :)

Yeah, well I certainly am.  Call me anal retentive if you like, but I
really prefer to use software that's rock-solid when it comes to
stability and security.

tim

Re: [Qemu-devel] [PATCH] security_20040618

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Sat, 2004-06-19 at 08:19 -0700, Tim wrote:
> > In dyngen you need to do:
> > 
> > if ( ptr == NULL )
> > 	error("malloc failed");
> > 
> > error() will never return.
> >
> > For the other places it depends, but it's ususally quite simple. Why not
> > have a stab and submit a seperate patch on top of this one?
> 
> You would prefer I submit a seperate patch for the malloc fixes?  I
> could certainly do that, but I just figured it is all a general code
> quality patch, which changes very few lines of code (though in many
> files).  What would you prefer Fabrice?

Well, I meant if you were to go ahead and implement all the TODOs and
handle all the malloc returns properly. I'd submit that as a seperate
patch, i mean, if you aren't 100% confident of your analyses.

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] security_20040618

[Fabrice Bellard]

Tim wrote:
> Updates to this, from the previous version two days ago, include
> additional strcpy replacements, as well as TODO comments pointing out
> unhealthy calls to {m,re}alloc, which don't check for NULL return
> values. (I am not sure how to handle error return codes in most places,
> so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
> should behave exactly the same as before.

Thank you for the fixes.

For malloc() returning NULL, if your patch just says "malloc error", I 
don't consider it is a good fix.

I see that you added many snprintf(). I don't like that because 
pstrcpy() does exactly the right thing and it is faster and simpler.

Fabrice.

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> Thank you for the fixes.

np.

> For malloc() returning NULL, if your patch just says "malloc error", I 
> don't consider it is a good fix.

Yeah, I figured it would be very context specific for these.  In
low-level functions, you'd just want to return an error value (that
would need to be checked as well) and at higher-level stuff, perhaps
you'd just bail out, and clean up before killing the process.

For now, I will leave my patch as it stands in this respect.  It doesn't
implement a single NULL check for {m,re}alloc, it merely adds TODO
comments.  Later on, after I address some other things, perhaps I will
come back and attack these with a different patch.

> I see that you added many snprintf(). I don't like that because 
> pstrcpy() does exactly the right thing and it is faster and simpler.

There were some cases where I switched sprintf() with snprintf().  For
those, I figure they should stay that way.  For the strcpy() ->
snprintf() changes, yes, I agree that your pstrcpy() is faster.  I'll
review those and change them where I can.  However, pstrcpy() may not be
available via includes to every file, so I'll probably let the
snprintf() stand if I run into that problem.

Were you able to look at my proposed changes to pstrcpy()?  What do you
think of them?

Thanks for the feedback!
tim

Re: [Qemu-devel] [PATCH] security_20040618

[Fabrice Bellard]

Tim wrote:

> Were you able to look at my proposed changes to pstrcpy()?  What do you
> think of them?

An advice: _never_ use strncpy, it never does what you think it does !

[Your patch is not correct as it adds a lot of zeros at the end of the 
buffer, which is not the intended behaviour of pstrcpy].

Fabrice.

Re: [Qemu-devel] [PATCH] security_20040618

[Vladimir N. Oleynik]

Tim,

 > Updates to this, from the previous version two days ago, include
 > additional strcpy replacements,

 > --- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
 > +++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
 >                  str = qemu_malloc(strlen(buf) + 1);
 > -                strcpy(str, buf);
 > +                pstrcpy(str, strlen(buf) + 1,  buf);

In my opinion, it already absolutely unnecessary correction.
There is in this place no problem.


--w
vodz

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> > --- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
> > +++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
> >                  str = qemu_malloc(strlen(buf) + 1);
> > -                strcpy(str, buf);
> > +                pstrcpy(str, strlen(buf) + 1,  buf);
> 
> In my opinion, it already absolutely unnecessary correction.
> There is in this place no problem.

Yeah, you are probably right.  I looked at that one on 3 seperate
occasions before making the change, since I recognized that there are
very few conditions where it could possibly be a problem, and come to
think of it, this fix doesn't mitigate those conditions.

That chunk of code makes me uncomfortable for other reasons though (does
qemu_malloc() return NULL ever?  could buf possibly be missing a
trailing '\0' ever?) so I'll re-visit it again and see what makes the
most sense.  The pstrcpy isn't hurting anything though.  Slightly slower
copy, due to the length checking, but it isn't in a critical piece of
code (monitor.c is just for the user interface command prompt, right?),
so I also don't see a reason to remove it, esp if changes in the future
open up the possibility of an overflow.

thanks for the comment,
tim