- djm@cvs.openbsd.org 2006/03/25 01:13:23

[buffer.c channels.c deattack.c misc.c scp.c session.c sftp-client.c] [sftp-server.c ssh-agent.c ssh-rsa.c xmalloc.c xmalloc.h auth-pam.c] [uidswap.c] change OpenSSH's xrealloc() function from being xrealloc(p, new_size) to xrealloc(p, new_nmemb, new_itemsize). realloc is particularly prone to integer overflows because it is almost always allocating "n * size" bytes, so this is a far safer API; ok deraadt@
2006-03-26 14:22:47 +11:00 · 2006-03-26 14:22:47 +11:00 · 36812092ec
parent 07d86bec5e
commit 36812092ec
16 changed files with 48 additions and 30 deletions
--- a/12
+++ b/12
@ -118,6 +118,16 @@
     to die

     feedback and ok deraadt@
+   - djm@cvs.openbsd.org 2006/03/25 01:13:23
+     [buffer.c channels.c deattack.c misc.c scp.c session.c sftp-client.c]
+     [sftp-server.c ssh-agent.c ssh-rsa.c xmalloc.c xmalloc.h auth-pam.c]
+     [uidswap.c]
+     change OpenSSH's xrealloc() function from being xrealloc(p, new_size)
+     to xrealloc(p, new_nmemb, new_itemsize).
+
+     realloc is particularly prone to integer overflows because it is
+     almost always allocating "n * size" bytes, so this is a far safer 
+     API; ok deraadt@

 20060325
 - OpenBSD CVS Sync
@ -4375,4 +4385,4 @@
   - (djm) Trim deprecated options from INSTALL. Mention UsePAM
   - (djm) Fix quote handling in sftp; Patch from admorten AT umich.edu

-$Id: ChangeLog,v 1.4273 2006/03/26 03:19:21 djm Exp $
+$Id: ChangeLog,v 1.4274 2006/03/26 03:22:47 djm Exp $
--- a/auth-pam.c
+++ b/auth-pam.c
@ -703,7 +703,7 @@ sshpam_query(void *ctx, char **name, char **info,
 		case PAM_PROMPT_ECHO_OFF:
 			*num = 1;
 			len = plen + mlen + 1;
-			**prompts = xrealloc(**prompts, len);
+			**prompts = xrealloc(**prompts, 1, len);
 			strlcpy(**prompts + plen, msg, len - plen);
 			plen += mlen;
 			**echo_on = (type == PAM_PROMPT_ECHO_ON);
@ -713,7 +713,7 @@ sshpam_query(void *ctx, char **name, char **info,
 		case PAM_TEXT_INFO:
 			/* accumulate messages */
 			len = plen + mlen + 2;
-			**prompts = xrealloc(**prompts, len);
+			**prompts = xrealloc(**prompts, 1, len);
 			strlcpy(**prompts + plen, msg, len - plen);
 			plen += mlen;
 			strlcat(**prompts + plen, "\n", len - plen);
--- a/buffer.c
+++ b/buffer.c
@ -109,7 +109,7 @@ restart:
 	if (newlen > BUFFER_MAX_LEN)
 		fatal("buffer_append_space: alloc %u not supported",
 		    newlen);
-	buffer->buf = xrealloc(buffer->buf, newlen);
+	buffer->buf = xrealloc(buffer->buf, 1, newlen);
 	buffer->alloc = newlen;
 	goto restart;
 	/* NOTREACHED */
--- a/channels.c
+++ b/channels.c
@ -266,8 +266,8 @@ channel_new(char *ctype, int type, int rfd, int wfd, int efd,
 		if (channels_alloc > 10000)
 			fatal("channel_new: internal error: channels_alloc %d "
 			    "too big.", channels_alloc);
-		channels = xrealloc(channels,
-		    (channels_alloc + 10) * sizeof(Channel *));
+		channels = xrealloc(channels, channels_alloc + 10,
+		    sizeof(Channel *));
 		channels_alloc += 10;
 		debug2("channel: expanding %d", channels_alloc);
 		for (i = found; i < channels_alloc; i++)
@ -1789,15 +1789,20 @@ void
 channel_prepare_select(fd_set **readsetp, fd_set **writesetp, int *maxfdp,
    u_int *nallocp, int rekeying)
 {
-	u_int n, sz;
+	u_int n, sz, nfdset;

 	n = MAX(*maxfdp, channel_max_fd);

-	sz = howmany(n+1, NFDBITS) * sizeof(fd_mask);
+	nfdset = howmany(n+1, NFDBITS);
+	/* Explicitly test here, because xrealloc isn't always called */
+	if (nfdset && SIZE_T_MAX / nfdset < sizeof(fd_mask))
+		fatal("channel_prepare_select: max_fd (%d) is too large", n);
+	sz = nfdset * sizeof(fd_mask);
+
 	/* perhaps check sz < nalloc/2 and shrink? */
 	if (*readsetp == NULL || sz > *nallocp) {
-		*readsetp = xrealloc(*readsetp, sz);
-		*writesetp = xrealloc(*writesetp, sz);
+		*readsetp = xrealloc(*readsetp, nfdset, sizeof(fd_mask));
+		*writesetp = xrealloc(*writesetp, nfdset, sizeof(fd_mask));
 		*nallocp = sz;
 	}
 	*maxfdp = n;
--- a/deattack.c
+++ b/deattack.c
@ -97,7 +97,7 @@ detect_attack(u_char *buf, u_int32_t len)
 		n = l;
 	} else {
 		if (l > n) {
-			h = (u_int16_t *) xrealloc(h, l * HASH_ENTRYSIZE);
+			h = (u_int16_t *)xrealloc(h, l, HASH_ENTRYSIZE);
 			n = l;
 		}
 	}
--- a/misc.c
+++ b/misc.c
@ -425,7 +425,7 @@ addargs(arglist *args, char *fmt, ...)
 	} else if (args->num+2 >= nalloc)
 		nalloc *= 2;

-	args->list = xrealloc(args->list, nalloc * sizeof(char *));
+	args->list = xrealloc(args->list, nalloc, sizeof(char *));
 	args->nalloc = nalloc;
 	args->list[args->num++] = cp;
 	args->list[args->num] = NULL;
--- a/scp.c
+++ b/scp.c
@ -1190,7 +1190,7 @@ allocbuf(BUF *bp, int fd, int blksize)
 	if (bp->buf == NULL)
 		bp->buf = xmalloc(size);
 	else
-		bp->buf = xrealloc(bp->buf, size);
+		bp->buf = xrealloc(bp->buf, 1, size);
 	memset(bp->buf, 0, size);
 	bp->cnt = size;
 	return (bp);
--- a/session.c
+++ b/session.c
@ -837,7 +837,7 @@ child_set_env(char ***envp, u_int *envsizep, const char *name,
 			if (envsize >= 1000)
 				fatal("child_set_env: too many env vars");
 			envsize += 50;
-			env = (*envp) = xrealloc(env, envsize * sizeof(char *));
+			env = (*envp) = xrealloc(env, envsize, sizeof(char *));
 			*envsizep = envsize;
 		}
 		/* Need to set the NULL pointer at end of array beyond the new slot. */
@ -1941,8 +1941,8 @@ session_env_req(Session *s)
 	for (i = 0; i < options.num_accept_env; i++) {
 		if (match_pattern(name, options.accept_env[i])) {
 			debug2("Setting env %d: %s=%s", s->num_env, name, val);
-			s->env = xrealloc(s->env, sizeof(*s->env) *
-			    (s->num_env + 1));
+			s->env = xrealloc(s->env, s->num_env + 1,
+			    sizeof(*s->env));
 			s->env[s->num_env].name = name;
 			s->env[s->num_env].val = val;
 			s->num_env++;
--- a/sftp-client.c
+++ b/sftp-client.c
@ -393,8 +393,7 @@ do_lsreaddir(struct sftp_conn *conn, char *path, int printflag,
 				printf("%s\n", longname);

 			if (dir) {
-				*dir = xrealloc(*dir, sizeof(**dir) *
-				    (ents + 2));
+				*dir = xrealloc(*dir, ents + 2, sizeof(**dir));
 				(*dir)[ents] = xmalloc(sizeof(***dir));
 				(*dir)[ents]->filename = xstrdup(filename);
 				(*dir)[ents]->longname = xstrdup(longname);
--- a/sftp-server.c
+++ b/sftp-server.c
@ -716,7 +716,7 @@ process_readdir(void)
 		while ((dp = readdir(dirp)) != NULL) {
 			if (count >= nstats) {
 				nstats *= 2;
-				stats = xrealloc(stats, nstats * sizeof(Stat));
+				stats = xrealloc(stats, nstats, sizeof(Stat));
 			}
 /* XXX OVERFLOW ? */
 			snprintf(pathname, sizeof pathname, "%s%s%s", path,
--- a/ssh-agent.c
+++ b/ssh-agent.c
@ -803,7 +803,7 @@ new_socket(sock_type type, int fd)
 		}
 	old_alloc = sockets_alloc;
 	new_alloc = sockets_alloc + 10;
-	sockets = xrealloc(sockets, new_alloc * sizeof(sockets[0]));
+	sockets = xrealloc(sockets, new_alloc, sizeof(sockets[0]));
 	for (i = old_alloc; i < new_alloc; i++)
 		sockets[i].type = AUTH_UNUSED;
 	sockets_alloc = new_alloc;
--- a/ssh-rand-helper.c
+++ b/ssh-rand-helper.c
@ -768,7 +768,7 @@ prng_read_commands(char *cmdfilename)
 		 */
 		if (cur_cmd == num_cmds) {
 			num_cmds *= 2;
-			entcmd = xrealloc(entcmd, num_cmds *
+			entcmd = xrealloc(entcmd, num_cmds,
 			    sizeof(entropy_cmd_t));
 		}
 	}
@ -777,7 +777,7 @@ prng_read_commands(char *cmdfilename)
 	memset(&entcmd[cur_cmd], '\0', sizeof(entropy_cmd_t));

 	/* trim to size */
-	entropy_cmds = xrealloc(entcmd, (cur_cmd + 1) *
+	entropy_cmds = xrealloc(entcmd, (cur_cmd + 1),
 	    sizeof(entropy_cmd_t));

 	debug("Loaded %d entropy commands from %.100s", cur_cmd,
--- a/ssh-rsa.c
+++ b/ssh-rsa.c
@ -144,7 +144,7 @@ ssh_rsa_verify(const Key *key, const u_char *signature, u_int signaturelen,
 		u_int diff = modlen - len;
 		debug("ssh_rsa_verify: add padding: modlen %u > len %u",
 		    modlen, len);
-		sigblob = xrealloc(sigblob, modlen);
+		sigblob = xrealloc(sigblob, 1, modlen);
 		memmove(sigblob + diff, sigblob, len);
 		memset(sigblob, 0, diff);
 		len = modlen;
--- a/uidswap.c
+++ b/uidswap.c
@ -76,7 +76,7 @@ temporarily_use_uid(struct passwd *pw)
 		fatal("getgroups: %.100s", strerror(errno));
 	if (saved_egroupslen > 0) {
 		saved_egroups = xrealloc(saved_egroups,
-		    saved_egroupslen * sizeof(gid_t));
+		    saved_egroupslen, sizeof(gid_t));
 		if (getgroups(saved_egroupslen, saved_egroups) < 0)
 			fatal("getgroups: %.100s", strerror(errno));
 	} else { /* saved_egroupslen == 0 */
@ -95,7 +95,7 @@ temporarily_use_uid(struct passwd *pw)
 			fatal("getgroups: %.100s", strerror(errno));
 		if (user_groupslen > 0) {
 			user_groups = xrealloc(user_groups,
-			    user_groupslen * sizeof(gid_t));
+			    user_groupslen, sizeof(gid_t));
 			if (getgroups(user_groupslen, user_groups) < 0)
 				fatal("getgroups: %.100s", strerror(errno));
 		} else { /* user_groupslen == 0 */
--- a/xmalloc.c
+++ b/xmalloc.c
@ -35,7 +35,7 @@ xcalloc(size_t nmemb, size_t size)
 {
 	void *ptr;

-        if (nmemb && size && SIZE_T_MAX / nmemb < size)
+	if (nmemb && size && SIZE_T_MAX / nmemb < size)
 		fatal("xcalloc: nmemb * size > SIZE_T_MAX");
 	if (size == 0 || nmemb == 0)
 		fatal("xcalloc: zero size");
@ -47,10 +47,13 @@ xcalloc(size_t nmemb, size_t size)
 }

 void *
-xrealloc(void *ptr, size_t new_size)
+xrealloc(void *ptr, size_t nmemb, size_t size)
 {
 	void *new_ptr;
+	size_t new_size = nmemb * size;

+	if (nmemb && size && SIZE_T_MAX / nmemb < size)
+		fatal("xrealloc: nmemb * size > SIZE_T_MAX");
 	if (new_size == 0)
 		fatal("xrealloc: zero size");
 	if (ptr == NULL)
@ -58,7 +61,8 @@ xrealloc(void *ptr, size_t new_size)
 	else
 		new_ptr = realloc(ptr, new_size);
 	if (new_ptr == NULL)
-		fatal("xrealloc: out of memory (new_size %lu bytes)", (u_long) new_size);
+		fatal("xrealloc: out of memory (new_size %lu bytes)",
+		    (u_long) new_size);
 	return new_ptr;
 }

--- a/xmalloc.h
+++ b/xmalloc.h
@ -1,4 +1,4 @@
-/*	$OpenBSD: xmalloc.h,v 1.10 2006/03/25 00:05:41 djm Exp $	*/
+/*	$OpenBSD: xmalloc.h,v 1.11 2006/03/25 01:13:23 djm Exp $	*/

 /*
 * Author: Tatu Ylonen <ylo@cs.hut.fi>
@ -21,7 +21,7 @@

 void	*xmalloc(size_t);
 void	*xcalloc(size_t, size_t);
-void	*xrealloc(void *, size_t);
+void	*xrealloc(void *, size_t, size_t);
 void     xfree(void *);
 char	*xstrdup(const char *);
 int	 xasprintf(char **, const char *, ...)