From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.33) id 1BbOJT-0001Gh-OE for qemu-devel@nongnu.org; Fri, 18 Jun 2004 14:42:07 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.33) id 1BbOJS-0001Fp-TH for qemu-devel@nongnu.org; Fri, 18 Jun 2004 14:42:07 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.33) id 1BbOJR-0001Fl-8P for qemu-devel@nongnu.org; Fri, 18 Jun 2004 14:42:06 -0400 Received: from [206.72.67.39] (helo=claudius.sentinelchicken.org) by monty-python.gnu.org with smtp (Exim 4.34) id 1BbOI3-0006uC-4K for qemu-devel@nongnu.org; Fri, 18 Jun 2004 14:40:39 -0400 Date: Fri, 18 Jun 2004 11:40:36 -0700 From: Tim Message-ID: <20040618184036.GA1874@sentinelchicken.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="IS0zKkzwUGydFO0o" Content-Disposition: inline Subject: [Qemu-devel] [PATCH] security_20040618 Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org --IS0zKkzwUGydFO0o Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --IS0zKkzwUGydFO0o Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=README 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. --IS0zKkzwUGydFO0o Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="security.patch" --- 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", --IS0zKkzwUGydFO0o--