Forum OpenACS Q&A: Response to Anyone using AOLserver < 3.4 with nsperm module?

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.