Mailing List Archive

Hi,

in a short review of spa-auth-fixes, I found a few issues:

First, the code now tries to avoid reading outside the respective data
structures, but it still reads uninitialized parts of the structure itself,
which still can lead to information disclosure.

For example:

| SPAAuthResponse *responseptr = &response;
| [...]
| if (spa_base64_to_bits(CS &response, sizeof(response), CCS data) < 0)
| [...]
| int len = SVAL(&responseptr->uUser.len,0)/2;

spa_base64_to_bits() returns the number of bytes it has written to
response, which can be less than the end of the uUser field.

Then, the newly introduced code in the macro spa_bytes_add that's
presumably intended to do a bounds check has incorrect operator precedence
for that to work reliably:

| #define spa_bytes_add(ptr, header, buf, count) \
| { \
| -if (buf && (count) != 0) /* we hate -Wint-in-bool-contex */ \
| +if ( buf && (count) != 0 /* we hate -Wint-in-bool-contex */ \
| + && ptr->bufIndex + count < sizeof(ptr->buffer) \
| + ) \
| { \

Combined with another change:

| -spa_bytes_add (response, lmResponse, lmRespData, (cf & 0x200) ? 24 : 0);
| -spa_bytes_add (response, ntResponse, ntRespData, (cf & 0x8000) ? 24 : 0);
| +spa_bytes_add(response, lmResponse, lmRespData, cf & 0x200 ? 24 : 0);
| +spa_bytes_add(response, ntResponse, ntRespData, cf & 0x8000 ? 24 : 0);

Effectively uses the following expression to do the bounds check:

| ((response->bufIndex + cf) & 0x8000) ? 24 : (0 < sizeof(response->buffer))

As 0 < sizeof(response->buffer) is always true, and 24 is, too, this
supposed bounds check always succeeds for these call sites. Luckily, that
probably doesn't cause a vulnerability because the lengths here are not
under the control of the attacker. But my guess is that the code still is
broken because what looks like a flag check doesn't do a flag check
anymore.

And finally, the bounds checks in the newly introduced functions
get_challenge_unistr() and get_challenge_str() don't really work, for one
because they can exhibit undefined behaviour, but also because they don't
limit indices to acceptable values even when the behaviour is defined:

| +static inline uschar *
| +get_challenge_unistr(SPAAuthChallenge * challenge, SPAStrHeader * hdr)
| +{
| +int off = IVAL(&hdr->offset, 0);
| +int len = SVAL(&hdr->len, 0);
| +return off + len < sizeof(SPAAuthChallenge)
| + ? US unicodeToString(CS challenge + off, len/2) : US"";
| +}

Assuming that int is 32 bit, 'off' is 32 bit signed, IVAL() returns an
attacker-controlled 32 bit unsigned value, so via usual
implementation-defined behaviour, the attacker can make 'off' negative,
which isn't detected by the bounds check, and thus can be used to read data
that's stored before 'challenge'.

Undefined behaviour results if 'off' is INT_MAX and 'len' is non-zero.
Usually, that'll lead to overflow in the addition, also resulting in a
negative value that passes the bounds check, thus allowing to read data
roughly INT_MAX behind 'challenge', which presumably would be a different
location with 64 bit pointers.

Regards, Florian

--
## subscription configuration (requires account):
## https://lists.exim.org/mailman3/postorius/lists/exim-dev.lists.exim.org/
## unsubscribe (doesn't require an account):
## exim-dev-unsubscribe@lists.exim.org
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/