Re: Bug#211434: more ssh memory management fun
brian@ristuccia.com said:
> Package: ssh
> Version: 1:3.4p1-1.woody.2
>
> It looks like even after the two updates in the past two days there's still
> more suspect memory management code in OpenSSH. See yet another OpenSSH
> security advisory at
>
> http://www.openpkg.org/security/OpenPKG-SA-2003.040-openssh.html
>
> As if you folks haven't been busy enough lately. Good luck.
Here's the patch. I extracted it from the file openssh.patch in the
source rpm at:
ftp://ftp.openpkg.org/release/1.3/UPD/openssh-3.6.1p2-1.3.2.src.rpm
I have not reviewed this patch for correctness.
These fixes are not in debian's 3.6.1p2-8 package; I didn't check the
woody.2 version.
Jason
These patches adjust (re)allocation procedures so they do not
alter context structures unless the (re)allocation was successful.
Otherwise the fatal cleanup functions (trigged from within the
failing (re)allocation functions) will be confused and especially
(for some instances) incorrectly clear (smaller than recorded) memory
buffers with NUL bytes. This patch is based on work by Solar Designer
<solar@openwall.com>.
Index: deattack.c
--- deattack.c.orig 2002-03-05 02:53:05.000000000 +0100
+++ deattack.c 2003-09-17 09:30:09.000000000 +0200
@@ -100,12 +100,12 @@
if (h == NULL) {
debug("Installing crc compensation attack detector.");
+ h = (u_int16_t *) xmalloc(l * HASH_ENTRYSIZE);
n = l;
- h = (u_int16_t *) xmalloc(n * HASH_ENTRYSIZE);
} else {
if (l > n) {
+ h = (u_int16_t *) xrealloc(h, l * HASH_ENTRYSIZE);
n = l;
- h = (u_int16_t *) xrealloc(h, n * HASH_ENTRYSIZE);
}
}
Index: misc.c
--- misc.c.orig 2003-08-25 03:16:21.000000000 +0200
+++ misc.c 2003-09-17 09:30:09.000000000 +0200
@@ -308,18 +308,21 @@
{
va_list ap;
char buf[1024];
+ int nalloc;
va_start(ap, fmt);
vsnprintf(buf, sizeof(buf), fmt, ap);
va_end(ap);
+ nalloc = args->nalloc;
if (args->list == NULL) {
- args->nalloc = 32;
+ nalloc = 32;
args->num = 0;
- } else if (args->num+2 >= args->nalloc)
- args->nalloc *= 2;
+ } else if (args->num+2 >= nalloc)
+ nalloc *= 2;
- args->list = xrealloc(args->list, args->nalloc * sizeof(char *));
+ args->list = xrealloc(args->list, nalloc * sizeof(char *));
+ args->nalloc = nalloc;
args->list[args->num++] = xstrdup(buf);
args->list[args->num] = NULL;
}
Index: session.c
--- session.c.orig 2003-09-16 03:52:19.000000000 +0200
+++ session.c 2003-09-17 09:34:20.000000000 +0200
@@ -800,6 +800,7 @@
{
u_int i, namelen;
char **env;
+ u_int envsize;
/*
* If we're passed an uninitialized list, allocate a single null
@@ -826,12 +827,14 @@
xfree(env[i]);
} else {
/* New variable. Expand if necessary. */
- if (i >= (*envsizep) - 1) {
- if (*envsizep >= 1000)
+ envsize = *envsizep;
+ if (i >= envsize - 1) {
+ if (envsize >= 1000)
fatal("child_set_env: too many env vars,"
" skipping: %.100s", name);
- (*envsizep) += 50;
- env = (*envp) = xrealloc(env, (*envsizep) * sizeof(char *));
+ envsize += 50;
+ env = (*envp) = xrealloc(env, envsize * sizeof(char *));
+ *envsizep = envsize;
}
/* Need to set the NULL pointer at end of array beyond the new slot. */
env[i + 1] = NULL;
Index: ssh-agent.c
--- ssh-agent.c.orig 2003-08-22 01:34:41.000000000 +0200
+++ ssh-agent.c 2003-09-17 09:30:09.000000000 +0200
@@ -784,7 +784,7 @@
static void
new_socket(sock_type type, int fd)
{
- u_int i, old_alloc;
+ u_int i, old_alloc, new_alloc;
if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0)
error("fcntl O_NONBLOCK: %s", strerror(errno));
@@ -795,25 +795,26 @@
for (i = 0; i < sockets_alloc; i++)
if (sockets[i].type == AUTH_UNUSED) {
sockets[i].fd = fd;
- sockets[i].type = type;
buffer_init(&sockets[i].input);
buffer_init(&sockets[i].output);
buffer_init(&sockets[i].request);
+ sockets[i].type = type;
return;
}
old_alloc = sockets_alloc;
- sockets_alloc += 10;
+ new_alloc = sockets_alloc + 10;
if (sockets)
- sockets = xrealloc(sockets, sockets_alloc * sizeof(sockets[0]));
+ sockets = xrealloc(sockets, new_alloc * sizeof(sockets[0]));
else
- sockets = xmalloc(sockets_alloc * sizeof(sockets[0]));
- for (i = old_alloc; i < sockets_alloc; i++)
+ sockets = xmalloc(new_alloc * sizeof(sockets[0]));
+ for (i = old_alloc; i < new_alloc; i++)
sockets[i].type = AUTH_UNUSED;
- sockets[old_alloc].type = type;
+ sockets_alloc = new_alloc;
sockets[old_alloc].fd = fd;
buffer_init(&sockets[old_alloc].input);
buffer_init(&sockets[old_alloc].output);
buffer_init(&sockets[old_alloc].request);
+ sockets[old_alloc].type = type;
}
static int
Reply to: