Re: CVE request - mcrypt buffer overflow flaw

[Eygene Ryabinkin <rea-sec () codelabs ru>]

Thu, Sep 06, 2012 at 08:37:14AM -0600, Vincent Danen wrote:
> A buffer overflow was reported [1],[2] in mcrypt version 2.6.8 and
> earlier due to a boundary error in the processing of an encrypted file
> (via the check_file_head() function in src/extra.c).  If a user were
> tricked into attempting to decrypt a specially-crafted .nc encrypted
> flie, this flaw would cause a stack-based buffer overflow that could
> potentially lead to arbitrary code execution.
>
> References:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=855029
> https://secunia.com/advisories/50507/
> https://bugs.gentoo.org/show_bug.cgi?id=434112
> http://packetstormsecurity.org/files/116268/mcrypt-2.6.8-Buffer-Overflow-Proof-Of-Concept.html

Unfortunately, mcrypt's check_file_head() in combination with
decrypt_general() is a bit worse: it allows to overwrite up to 50
bytes of stack buffers from decrypt_general(), namely local_algorithm,
local_mode, local_keymode.  And in some curcumstances to overwrite
even 2-3 extra bytes (not more, since buf[3] will contain '\0'), though
it is not very much controllable path.

The problem is that no length checks are done in combos
read_until_null/strcpy.  Function read_until_null() allows for up to
100 bytes to be read and it won't NUL-terminate the buffer, so strcpy
can do perform access even further (read from tmp_buf and writes to
the said buffers; but this is the uncontrolled way I was talking
about).

The modified PoC is at
  http://codelabs.ru/security/mcrypt/poc-cve-2012-4409.py
With it I was able to overwrite the salt_size@decrypt_general()
and to trigger the call to malloc() for the chunk of 0x42424242 bytes
via _mcrypt_malloc() that lead to bus error because of subsequent
memmove():
{{{
      salt = _mcrypt_malloc(salt_size);
      memmove(salt, local_salt, salt_size);
}}}

I wasn't yet able to smash the stack of decrypt_general(), because
BUFFER_SIZE is 1024 and tmp_buf prevents me to reach the top of the
stack frame (provided that compiler won't rearrange local variables),
so I was not able to go past it.  Thus it looks like a temporary
memory consumption/DoS.
-- 
Eygene