Argh, all these answers would just be wrong if the wrong thing had not already been done within AOLserver. As the ChangeLog states,
Ns_HtuuDecode does not do an adequate job of preventing data overflow.
The fix isn't in serv.c, the fix is
in htuu.c.
If you make this fix in serv.c, you will still leave NsTclHTUUDecodeCmd
(ns_uudecode) broken, which would lead to the same buffer overflow.
Oh. Except that someone already put the kluge (overmalloc the buffer)
in there.
Late breaking news: it appears the suggested fix still leaves AOLserver vulnerable, but maybe not since no one has reported it yet.
Still, what do you do about the next poor slob to call Ns_HtuuDecode?
My analysis shows two buffer overflows occuring due to bugs within
Ns_HtuuDecode. One is an overflow of the output buffer by
one or two bytes that may occur within Ns_HtuuDecode. The other is a
potential overflow of the output buffer by an arbitrary number of
bytes caused when Ns_HtuuDecode returns the wrong number of bytes
allocated.
The suggested fix (mallocing the buffer on the fly) may still cause an
AOLserver crash, presumably, your wiley h4x0r can just fill that input
buffer up with a gig or two more data than your AOLserver process can
malloc and the system will still die.
In fact, Ns_HtuuDecode looks as though it is written to decode a fixed
amount of data at a time, and users of Ns_HtuuDecode are supposed to
loop on the return value, decoding all the data in their buffer by
repeatedly calling Ns_HtuuDecode with just a bit at a time and then
copying it out.
The two users of this within AOLserver 3.2, don't do that though.
int
Ns_HtuuDecode(char * bufcoded, unsigned char * bufplain, int outbufsize)
{
. . .
int nbytesdecoded, j;
register char *bufin = bufcoded;
register unsigned char *bufout = bufplain;
register int nprbytes;
. . .
/*
* Figure out how many characters are in the input buffer. If this would
* decode into more bytes than would fit into the output buffer, adjust
* the number of input bytes downwards.
*/
bufin = bufcoded;
while (pr2six[(int) *(bufin++)] <= MAXVAL);
nprbytes = bufin - bufcoded - 1;
This looks odd but is correct, the number of bytes in the input
buffer in bufin - bufcoded -1 as the while has run once too many times.
nbytesdecoded = ((nprbytes + 3) / 4) * 3;
I am not sure what nytesdecoded is at this point. In the end, it
is to be the number of bytes actually decoded. Does it represent
that here, or is it just an interim value?
Whatever, here below is the check and correction to make sure all
we only decode the right number of characters. Only the correction
(or the while loop below) is wrong.
if (nbytesdecoded > outbufsize) {
nprbytes = (outbufsize * 4) / 3;
}
bufin = bufcoded;
while (nprbytes > 0) {
*(bufout++) = (unsigned char) (DEC(*bufin) << 2 | DEC(bufin[1]) >> 4);
*(bufout++) = (unsigned char) (DEC(bufin[1]) << 4 |
DEC(bufin[2]) >> 2);
*(bufout++) = (unsigned char) (DEC(bufin[2]) << 6 | DEC(bufin[3]));
bufin += 4;
nprbytes -= 4;
}
So the loop runs through ceil(int(4/3*nprbytes)/4) times. Each time
it reads 4 input bytes and creates 3 output bytes.
Because of the way the loop is constructed, this appears to always
overflow the buffer unless nprbytes is a multiple of 3.
| n (nprbytes) | int(4n/3)/4 | bytes written | bytes overflowed |
|---|
| 0 | 0 | 0 | 0 |
| 1 | 0.25 | 3 | 2 |
| 2 | 0.5 | 3 | 1 |
| 3 | 1 | 3 | 0 |
| 4 | 1.25 | 6 | 2 |
| 5 | 1.5 | 6 | 1 |
| 6 | 2 | 6 | 0 |
It looks as though you could change the comparison to 2 and get the
right behavior.
From
while (nprbytes > 0) {
To
while (nprbytes > 2) {
But I don't think that's enough, because regardless of how many chars
you actually decoded, the routine returns the number of bytes that
would have been decoded if all the woodchucks had decoded all the
bytes.
if (nprbytes & 03) {
if (pr2six[(int) bufin[-2]] > MAXVAL) {
nbytesdecoded -= 2;
} else {
nbytesdecoded -= 1;
}
}
return (nbytesdecoded);
}
If I didn't think I had pneumonia, I would probably try to fix this. As it is, I'm a bit too woozy to figure out the exact right thing to do. But, the fix is here, and not in serv.c.
If this analysis is wrong, I'd blame it on the cold medicine, but
that's probably not true, rather I just fucked up the analysis.
Happens.
