[kan@dcit.cz: bugs in lib/base64.c]

Comments?

----- Forwarded message from Pavel Kankovský <kan@dcit.cz> -----

Subject: bugs in lib/base64.c
To: squid-bugs@squid-cache.org
X-Mailer: Lotus Notes Release 5.0.5 September 22, 2000
From: "Pavel Kankovský" <kan@dcit.cz>
Date: Tue, 12 Jun 2001 20:04:04 +0200

Version of Squid: 2.4.STABLE1
OS type and version: various

I.

There is this piece of code in base64_encode():

    static char result[BASE64_RESULT_SZ];
    [...]
    while ((c = *decoded_str++) && out_cnt < sizeof(result) - 1) {
        bits += c;
        char_count++;
        if (char_count == 3) {
            result[out_cnt++] = base64_code[bits >> 18];
            result[out_cnt++] = base64_code[(bits >> 12) & 0x3f];
            result[out_cnt++] = base64_code[(bits >> 6) & 0x3f];
            result[out_cnt++] = base64_code[bits & 0x3f];
            bits = 0;
            char_count = 0;
        } else {
            bits <<= 8;
        }
    }
    if (char_count != 0) {
        [...]
    }
    result[out_cnt] = '\0'; /* terminate */

If the input text is long enough, the loop will be executed for (out_cnt,
char_count) = (0, 0), (0, 1), (0, 2), (0, 3), (4, 0), ..., (BASE64_RESULT_SZ-4,
2), and (BASE64_RESULT_SZ-4, 3). After the last mentioned iteration, the loop
will terminated because out_cnt will be equal to BASE64_RESULT_SZ (8192). When
this happens, the last quoted command will zero a byte that lies *OUT* of space
allocated for result, probably corrupting the value of another static variable
in BSS.

Proposed fix: Replace "out_cnt < sizeof(result) - 1" condition with "out_cnt <
sizeof(result) - 5" in base64_encode().

The condition in base64_decode() ("j + 3 < BASE64_RESULT_SZ") is slighly broken
too. It should add 4 rather than 3 in order to guarantee enough space for a new
chunk of data as well as the terminator). Fortunately BASE64_RESULT_SZ is not
divisible by 3, therefore the the loop will always terminate with j <
BASE64_RESULT_SZ.

II.

Both base64_encode() and base64_decode() misinterpret characters with ASCII
codes >= 128 on platforms where char is a signed type (this covers most--if not
all--unix-like platforms).

base64_decode() does:

    unsigned int k;
    [...]
        k = (int) *p % BASE64_VALUE_SZ;

This is wrong. If *p whose type is (signed) char is negative, a cast to int
does nothing and the result of % is implementation-defined but it will be
negative on many platforms (AFAIK, the new ISO C standard makes this behaviour
of %, i.e. -1 % 2 == -1, mandatory). The assignment to k casts this negative
value to unsigned int and the result is a very large and completely nonsensical
number.

base64_encode() does:

    int bits = 0;
    [...]
    int c;
    [...]
    while ((c = *decoded_str++) && out_cnt < sizeof(result) - 1) {
        bits += c;

This is even simpler: a negative value is loaded into c and added to bits,
corrupting characters already accumulated in bits because rather than adding a
value between 0 and 255, the addition might *subtract* a value between 1 and
128.

Proposed fix: Replace all occurences to "*pointer_to_char" with "* (unsigned
char *) pointer_to_char". Also, it might be a good idea to change the type of
some local variables to unsigned int.

--
Pavel Kankovsky, DCIT s.r.o., J. Martiho 2/407, 160 41 Praha 6, CZ
tel (+420 2) 3536 3342, fax (+420 2) 3536 1543, url http://www.dcit.cz/
----- End forwarded message -----
-- 
Adrian Chadd			<Liedra> Don't worry, I know who *you* are ;)
<adrian@creative.net.au> 	<spiv> We know everything about you.