From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.33) id 1BeNN3-0006Te-AE for qemu-devel@nongnu.org; Sat, 26 Jun 2004 20:18:09 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.33) id 1BeNN2-0006TS-Iz for qemu-devel@nongnu.org; Sat, 26 Jun 2004 20:18:09 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.33) id 1BeNN2-0006TP-Bd for qemu-devel@nongnu.org; Sat, 26 Jun 2004 20:18:08 -0400 Received: from [206.72.67.39] (helo=claudius.sentinelchicken.org) by monty-python.gnu.org with smtp (Exim 4.34) id 1BeNLE-0004bV-Sr for qemu-devel@nongnu.org; Sat, 26 Jun 2004 20:16:18 -0400 Date: Sat, 26 Jun 2004 17:16:14 -0700 From: Tim Message-ID: <20040627001614.GA2775@sentinelchicken.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="3V7upXqbjpZ4EhLz" Content-Disposition: inline Subject: [Qemu-devel] [PATCH] security_20040626 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 --3V7upXqbjpZ4EhLz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline I have been pretty busy lately, so it took some time to get this version out. I took Charlie's advice and created a version of strdup which uses qemu_malloc() instead of malloc(). Since Fabrice already applied a number of fixes a while back, this patch doesn't have a whole lot of real code changes, outside of stuff in slirp and the pstrdup() function. I'll be turning my attention to the tmp file handling issues next. Ok, feel free to fire away... tim --3V7upXqbjpZ4EhLz Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="security.patch" --- qemu-current/hw/cirrus_vga.c 2004-06-26 16:56:25.000000000 -0700 +++ qemu-dev/hw/cirrus_vga.c 2004-06-26 11:30:07.000000000 -0700 @@ -2861,7 +2861,7 @@ CirrusVGAState *s; s = qemu_mallocz(sizeof(CirrusVGAState)); - + /* TODO: check for s == NULL */ vga_common_init((VGAState *)s, ds, vga_ram_base, vga_ram_offset, vga_ram_size); cirrus_init_common(s, CIRRUS_ID_CLGD5430, 0); --- qemu-current/hw/vga.c 2004-06-26 16:56:25.000000000 -0700 +++ qemu-dev/hw/vga.c 2004-06-26 11:30:07.000000000 -0700 @@ -1850,6 +1850,7 @@ { s->linesize = w * 4; s->data = qemu_malloc(h * s->linesize); + /* TODO: add check for s->data == NULL ?? */ vga_save_w = w; vga_save_h = h; } --- 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/exec.c 2004-06-26 16:56:25.000000000 -0700 +++ qemu-dev/exec.c 2004-06-23 20:09:25.000000000 -0700 @@ -156,6 +156,7 @@ if (!p) { /* allocate if not found */ p = qemu_malloc(sizeof(PageDesc) * L2_SIZE); + /* TODO: check for p == NULL */ memset(p, 0, sizeof(PageDesc) * L2_SIZE); *lp = p; } @@ -181,6 +182,7 @@ if (!p) { /* allocate if not found */ p = qemu_malloc(sizeof(PhysPageDesc) * L2_SIZE); + /* TODO: check for p == NULL */ memset(p, 0, sizeof(PhysPageDesc) * L2_SIZE); *lp = p; } @@ -210,6 +212,7 @@ if (!p) { /* allocate if not found */ p = qemu_malloc(sizeof(VirtPageDesc) * L2_SIZE); + /* TODO: check for p == NULL */ memset(p, 0, sizeof(VirtPageDesc) * L2_SIZE); *lp = p; } @@ -1914,6 +1917,7 @@ /* alloc dirty bits array */ phys_ram_dirty = qemu_malloc(phys_ram_size >> TARGET_PAGE_BITS); + /* TODO: check for phys_ram_dirty == NULL */ } /* mem_read and mem_write are arrays of functions containing the --- qemu-current/monitor.c 2004-06-26 16:56:25.000000000 -0700 +++ qemu-dev/monitor.c 2004-06-20 18:39:09.000000000 -0700 @@ -1179,8 +1179,7 @@ len = p - pstart; if (len > sizeof(cmdname) - 1) len = sizeof(cmdname) - 1; - memcpy(cmdname, pstart, len); - cmdname[len] = '\0'; + pstrcpy(cmdname, len, pstart); /* find the command */ for(cmd = term_cmds; cmd->name != NULL; cmd++) { @@ -1227,8 +1226,9 @@ term_printf("%s: string expected\n", cmdname); goto fail; } - str = qemu_malloc(strlen(buf) + 1); - strcpy(str, buf); + str = pstrdup(buf); + if (!str) + goto fail; str_allocated[nb_args] = str; add_str: if (nb_args >= MAX_ARGS) { --- 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-26 16:56:25.000000000 -0700 +++ qemu-dev/vl.c 2004-06-26 11:30:06.000000000 -0700 @@ -293,6 +293,16 @@ return buf; } +char *pstrdup(char *buf) +{ + unsigned int size = strlen(buf) + 1; + char* ret_val = (char*)qemu_malloc(size); + if (ret_val == NULL) + return NULL; + pstrcpy(ret_val, size, buf); + return ret_val; +} + /* return the size or -1 if error */ int get_image_size(const char *filename) { @@ -618,6 +628,7 @@ QEMUTimer *ts; ts = qemu_mallocz(sizeof(QEMUTimer)); + /* TODO: check for ts == NULL */ ts->clock = clock; ts->cb = cb; ts->opaque = opaque; --- qemu-current/vl.h 2004-06-26 16:56:25.000000000 -0700 +++ qemu-dev/vl.h 2004-06-26 13:41:16.000000000 -0700 @@ -215,6 +215,7 @@ void pstrcpy(char *buf, int buf_size, const char *str); char *pstrcat(char *buf, int buf_size, const char *s); +char *pstrdup(char *buf); int serial_open_device(void); --- 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-26 16:56:26.000000000 -0700 +++ qemu-dev/linux-user/syscall.c 2004-06-19 13:12:14.000000000 -0700 @@ -1410,6 +1410,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; @@ -2473,7 +2474,7 @@ tnamelen = treclen - (2 * sizeof(target_long) + 2); if (tnamelen > 256) tnamelen = 256; - strncpy(tde->d_name, de->d_name, tnamelen); + pstrcpy(tde->d_name, tnamelen, 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-20 18:06:22.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 @@ -485,11 +487,11 @@ if (s < 0) slirp_exit(1); sock_un.sun_family = AF_UNIX; - strcpy(sock_un.sun_path, socket_path); + snprintf(sock_un.sun_path, sizeof(sock_un.sun_path), "%s", socket_path); 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-20 17:45:46.000000000 -0700 @@ -449,14 +449,14 @@ type = omsg->type; OTOSIN(omsg, ctl_addr)->sin_port = addr.sin_port; OTOSIN(omsg, ctl_addr)->sin_addr = our_addr; - strncpy(omsg->l_name, getlogin(), NAME_SIZE_OLD); + snprintf(omsg->l_name, NAME_SIZE_OLD, "%s", getlogin()); } else { /* new talk */ omsg = (CTL_MSG_OLD *) buff; nmsg = mtod(m, CTL_MSG *); type = nmsg->type; OTOSIN(nmsg, ctl_addr)->sin_port = addr.sin_port; OTOSIN(nmsg, ctl_addr)->sin_addr = our_addr; - strncpy(nmsg->l_name, getlogin(), NAME_SIZE_OLD); + snprintf(nmsg->l_name, NAME_SIZE_OLD, "%s", getlogin()); } if (type == LOOK_UP) @@ -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, --3V7upXqbjpZ4EhLz Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=README This patch contains fixes for calls to potentially unsafe string manipulation functions. 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): dyngen.c exec.c monitor.c sparc-dis.c thunk.c vl.c vl.h hw/cirrus_vga.c hw/vga.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 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. --3V7upXqbjpZ4EhLz--