From 36812092ecb11a25ca9d6d87fdeaf53e371c5043 Mon Sep 17 00:00:00 2001 From: Damien Miller Date: Sun, 26 Mar 2006 14:22:47 +1100 Subject: [PATCH] - 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@ --- ChangeLog | 12 +++++++++++- auth-pam.c | 4 ++-- buffer.c | 2 +- channels.c | 17 +++++++++++------ deattack.c | 2 +- misc.c | 2 +- scp.c | 2 +- session.c | 6 +++--- sftp-client.c | 3 +-- sftp-server.c | 2 +- ssh-agent.c | 2 +- ssh-rand-helper.c | 4 ++-- ssh-rsa.c | 2 +- uidswap.c | 4 ++-- xmalloc.c | 10 +++++++--- xmalloc.h | 4 ++-- 16 files changed, 48 insertions(+), 30 deletions(-) diff --git a/ChangeLog b/ChangeLog index 20d034a6e..9d129a183 100644 --- a/ChangeLog +++ b/ChangeLog @@ -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 $ diff --git a/auth-pam.c b/auth-pam.c index 3d64de76a..c12f413e7 100644 --- 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); diff --git a/buffer.c b/buffer.c index 08682e0f1..1666f742e 100644 --- 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 */ diff --git a/channels.c b/channels.c index 0e7d5cf58..5706833a9 100644 --- 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; diff --git a/deattack.c b/deattack.c index 746ff5d43..ff9ca4dd5 100644 --- 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; } } diff --git a/misc.c b/misc.c index bf7b1ed66..96d90dec9 100644 --- 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; diff --git a/scp.c b/scp.c index bf9db97cf..3068b8d32 100644 --- 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); diff --git a/session.c b/session.c index b00caa547..f0a0bdd2f 100644 --- 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++; diff --git a/sftp-client.c b/sftp-client.c index c34f919a4..8b4d67b58 100644 --- 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); diff --git a/sftp-server.c b/sftp-server.c index a6add52aa..52b7323c2 100644 --- 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, diff --git a/ssh-agent.c b/ssh-agent.c index 67bde5560..042b18f54 100644 --- 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; diff --git a/ssh-rand-helper.c b/ssh-rand-helper.c index bdf73ec48..662f70080 100644 --- 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, diff --git a/ssh-rsa.c b/ssh-rsa.c index ce4195fea..55fb7ba59 100644 --- 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; diff --git a/uidswap.c b/uidswap.c index ca0894806..305895a44 100644 --- 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 */ diff --git a/xmalloc.c b/xmalloc.c index 6d56781d9..d5d7b6bc5 100644 --- 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; } diff --git a/xmalloc.h b/xmalloc.h index b6d521a66..ef29787bd 100644 --- 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 @@ -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 *, ...)