[OTR-dev] otrl_base64_otr_decode() function...

[Justin Ferguson]

While, I personally, don't feel an advisory is warranted, it seems
like if at least a CVE is not obtained that the vendors themselves are
lackadaisical about upgrading their packages, as such and due to the
fact that the original posting was obscure, the following 'advisory'
is included. There is no further actionable information from the
viewpoint of the OTR developers, this is strictly for reference by
Mitre et al.

-- snip --

The otrl_base64_otr_decode() function and similar functions within OTR
suffer from buffer overflows in the case of malformed input;
specifically if a message of the format of "?OTR:===." is received
then a zero-byte allocation is performed without a similar correlation
between the subsequent base64 decoding write, as such it becomes
possible to write between zero and three bytes incorrectly to the
heap, albeit only with a value of '='.

Because this code path is highly utilized, specifically in the
reception of instant messages over pidgin or similar, this
vulnerability is considered severe even though in many platforms and
circumstances the bug would yield an unexploitable state and result
simply in denial of service.

The developers of OTR promptly fixed the errors and users of OTR are
advised to upgrade the software at the next release cycle. Technical
details follow:


int otrl_base64_otr_decode(const char *msg, unsigned char **bufp,
	size_t *lenp)
{
    char *otrtag, *endtag;
    size_t msglen, rawlen;
    unsigned char *rawmsg;

[0]    otrtag = strstr(msg, "?OTR:");
    if (!otrtag) {
	return -2;
    }
[1]    endtag = strchr(otrtag, '.');
    if (endtag) {
[2]        msglen = endtag-otrtag;
    } else {
	return -2;
    }

    /* Base64-decode the message */
 [3]   rawlen = ((msglen-5) / 4) * 3;   /* maximum possible */
 [4]   rawmsg = malloc(rawlen);
    if (!rawmsg && rawlen > 0) {
	return -1;
    }
 [5]   rawlen = otrl_base64_decode(rawmsg, otrtag+5, msglen-5);  /*
actual size */

    *bufp = rawmsg;
    *lenp = rawlen;

    return 0;
}

Here, at (0) we see that the ortag pointer is initialized with the
return value of strstr() meaning that our message must contain the
"?OTR:" sequence somewhere within it, at this point (1) a second
pointer is initialized to point at a value after the initial "?OTR:"
string to the first occurrence of the character '.', which if it
exists then at (2) the variable msglen will be initialized to the
difference between the two values; the reader will be mindful to
consider the circumstance where a value subtracted from five yielding
a value less than four occurs. This eventually culminates in the
calculation of the rawlen variable at (3), which does not properly
consider the circumstances that msglen-5 / 4 might yield a zero value.
As such, when it is multiplied by 3, we of course obtain a value of
zero. This value is then passed unchecked to malloc() at (4),
performing a platform specific action. For instance, under windows a
buffer of 1 byte will be allocated, however the specifics will vary by
platform due to the behavior of a call to malloc(0). As such, a buffer
of up to 3 bytes is intended to be allocated, however, on paper, a
zero-byte buffer is allocated. This eventually culminates in an
incorrect size parameter being passed to the otrl_base64_decode()
function at (5). However, while it is not shown here, the function
will not decode sequences shorter than four bytes unless the character
is a base64 trailing character, as such it is only possible to
overwrite heap data with the '=' character.

This exact sequence of instructions were identified in three
additional locations within the OTR library; all of which have been
properly patched by the OTR developers, promptly. As stated
previously, it is thought that in most circumstances the end result is
simply denial of service, however despite the constraints and due to
the high flexibility of the code path to heap determinism, it is
thought that a clever hacker somewhere could absolutely yield remote
code execution, as such, users are encouraged to upgrade their
libotr/OTR instance, at the very least, at the next release cycle.

Best Regards,

Justin N. Ferguson



On Thu, Jul 19, 2012 at 11:46 AM, Ian Goldberg <ian at cypherpunks.ca> wrote:
> On Thu, Jul 19, 2012 at 11:37:47AM -0400, Paul Wouters wrote:
>> On Thu, 19 Jul 2012, Ian Goldberg wrote:
>>
>> >How's this for a proposed patch?  [Paul, I think this addresses your
>> >"magic numbers" concern as well.]
>>
>> It does, though I would have written:
>>
>> >+    /* Skip over the "?OTR:" */
>> >+    otrtag += 5;
>> >+    msglen -= 5;
>>
>>       otrtag += sizeof("?OTR:")
>>       msglen -= sizeof("?OTR:")
>>
>> Then you don't even need the comment :)
>
> But sizeof("?OTR:") is *6*, not 5, because of the NUL terminator.  You
> could use strlen() there, but I'm unconvinced about compilers
> consistently optimizing away library calls.
>
>    - Ian
> _______________________________________________
> OTR-dev mailing list
> OTR-dev at lists.cypherpunks.ca
> http://lists.cypherpunks.ca/mailman/listinfo/otr-dev