- 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@

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 <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 *, ...)