Date: Thu, 29 Oct 2020 18:45:45 +0000
From: Qualys Security Advisory via Security <security@exim.org>
To: "security@exim.org" <security@exim.org>
Subject: Re: [Exim-Security] Audit
Sender: Security <security-bounces+hs=nodmarc.schlittermann.de@exim.org>
Reply-To: Qualys Security Advisory <qsa@qualys.com>

Hi all,

Below are various non-security bugs that we found during our audit.
First, a few bugs that almost have security consequences.

========================================================================

1/ In src/transports/smtp.c:

2281       int n = sizeof(sx->buffer);
2282       uschar * rsp = sx->buffer;
2283
2284       if (sx->esmtp_sent && (n = Ustrlen(sx->buffer)) < sizeof(sx->buffer)/2)
2285         { rsp = sx->buffer + n + 1; n = sizeof(sx->buffer) - n; }

This should probably be either:

rsp = sx->buffer + n + 1; n = sizeof(sx->buffer) - n - 1;

or:

rsp = sx->buffer + n; n = sizeof(sx->buffer) - n;

(not sure which) to avoid an off-by-one.

========================================================================

2/ In src/spool_in.c:

 462   while (  (len = Ustrlen(big_buffer)) == big_buffer_size-1
 463         && big_buffer[len-1] != '\n'
 464         )
 465     {   /* buffer not big enough for line; certs make this possible */
 466     uschar * buf;
 467     if (big_buffer_size >= BIG_BUFFER_SIZE*4) goto SPOOL_READ_ERROR;
 468     buf = store_get_perm(big_buffer_size *= 2, FALSE);
 469     memcpy(buf, big_buffer, --len);

The --len in memcpy() chops off a useful byte (we know for sure that
big_buffer[len-1] is not a '\n' because we entered the while loop).

========================================================================

3/ In src/deliver.c:

 333 static int
 334 open_msglog_file(uschar *filename, int mode, uschar **error)
 335 {
 336 if (Ustrstr(filename, US"/../"))
 337   log_write(0, LOG_MAIN|LOG_PANIC,
 338     "Attempt to open msglog file path with upward-traversal: '%s'\n", filename);

Should this be LOG_PANIC_DIE instead of LOG_PANIC? Right now it will log
the /../ attempt but will open the file anyway.

========================================================================

4/ In src/smtp_in.c:

4966     case RCPT_CMD:
4967       HAD(SCH_RCPT);
4968       rcpt_count++;
....
5123       if (rcpt_count > recipients_max && recipients_max > 0)

In theory this recipients_max check can be bypassed, because the int
rcpt_count can overflow (become negative). In practice this would either
consume too much memory or generate too much network traffic, but maybe
it should be fixed anyway.

========================================================================

5/ receive_msg() calls dkim_exim_verify_finish(), which sets
dkim_collect_input to 0 and calls pdkim_feed_finish(), which calls
pdkim_header_complete(), which decreases dkim_collect_input to UINT_MAX,
which reactivates the DKIM code.

As a result, pdkim_feed() is called again (through receive_getc at the
end of receive_msg()), but functions like pdkim_finish_bodyhash() and
exim_sha_finish() have already been called (in pdkim_feed_finish()).
This suggests a use-after-free.

But it seems that a use-after-free would happen only with
EVP_DigestFinal() (in exim_sha_finish()), which does not seem to be
reachable via DKIM (no SHA3). But we checked OpenSSL only, not GnuTLS.

Here is a proof of concept that triggers the bug (which came very close
to a security vulnerability):

(sleep 10; echo 'EHLO test'; sleep 3; echo 'MAIL FROM:<>'; sleep 3; echo 'RCPT TO:postmaster'; sleep 3; echo 'BDAT 42 LAST'; date >&2; sleep 30; printf 'not a valid header line\r\nDKIM-Signature:\r\nXXX'; sleep 30) | nc -n -v 192.168.56.102 25

(gdb) print &dkim_collect_input
$2 = (unsigned int *) 0x55e180386d90 <dkim_collect_input>
(gdb) watch *(unsigned int *) 0x55e180386d90

Hardware watchpoint 1: *(unsigned int *) 0x55e180386d90
Old value = 0
New value = 4294967295
#0  0x000055e18031f805 in pdkim_header_complete (ctx=ctx@entry=0x55e181b9e8e0) at pdkim.c:1006
#1  0x000055e18032106c in pdkim_feed_finish (ctx=0x55e181b9e8e0, return_signatures=0x55e180386d78 <dkim_signatures>, err=err@entry=0x7ffe443e1d00) at pdkim.c:1490
#2  0x000055e1802a3280 in dkim_exim_verify_finish () at dkim.c:328
#3  0x000055e1802c9d1d in receive_msg (extract_recip=extract_recip@entry=0) at receive.c:3409

========================================================================

6/ In src/pdkim/pdkim.c, pdkim_update_ctx_bodyhash() is sometimes called
with a global orig_data and hence canon_data, and the following line can
therefore modify data that should be constant:

 773   canon_data->len = b->bodylength - b->signed_body_bytes;

For example, the following proof of concept sets lineending.len to 0
(this should not be possible):

(sleep 10; echo 'EHLO test'; sleep 3; echo 'MAIL FROM:<>'; sleep 3; echo 'RCPT TO:postmaster'; sleep 3; echo 'DATA'; date >&2; sleep 30; printf 'DKIM-Signature:a=rsa-sha1;c=simple/simple;l=0\r\n\r\n\r\nXXX\r\n.\r\n'; sleep 30) | nc -n -v 192.168.56.102 25

(gdb) print lineending
$1 = {data = 0x55e18035b2ad "\r\n", len = 2}
(gdb) print &lineending.len
$3 = (size_t *) 0x55e180385948 <lineending+8>
(gdb) watch *(size_t *) 0x55e180385948

Hardware watchpoint 1: *(size_t *) 0x55e180385948
Old value = 2
New value = 0
(gdb) print lineending
$5 = {data = 0x55e18035b2ad "\r\n", len = 0}

========================================================================

7/ In src/smtp_out.c, read_response_line(), inblock->ptr is not updated
when -1 is returned. This does not seem to have bad consequences, but is
maybe not the intended behavior.

========================================================================

8/ When gstring_grow() calls store_extend(), oldsize is g->size, and
store_extend() therefore applies its rounding to g->size. But initially,
the rounding was not applied to g->size but to sizeof(gstring) + g->size
(in string_get()). This luckily does not have bad consequences on 64-bit
(because sizeof(gstring) is rounded -- a multiple of alignment), but on
32-bit it may have unintended consequences. We were not able to exploit
this, however.

========================================================================

9/ In several cases a fixed gstring is built manually, but is then used
with string functions that may grow/extend the gstring and have
unintended consequences. For example, in src/exim.c:

 180 void
 181 set_process_info(const char *format, ...)
 182 {
 183 gstring gs = { .size = PROCESS_INFO_SIZE - 2, .ptr = 0, .s = process_info };
 184 gstring * g;
 185 int len;
 186 va_list ap;
 187
 188 g = string_fmt_append(&gs, "%5d ", (int)getpid());
 189 len = g->ptr;
 190 va_start(ap, format);
 191 if (!string_vformat(g, 0, format, ap))
 192   {
 193   gs.ptr = len;
 194   g = string_cat(&gs, US"**** string overflowed buffer ****");
 195   }
 196 g = string_catn(g, US"\n", 1);

string_fmt_append() uses the SVFMT_EXTEND flag, and string_cat() and
string_catn() call gstring_grow(). We were not able to exploit this,
however.

========================================================================

10/ string_vformat_trc() should always call die_tainted() if the format
string is tainted. Otherwise an attacker can exploit %n or overflow the
newformat[] stack buffer.

One note about untainted memory vs. memory corruption: the ideal
long-term solution would be to make all untainted memory read-only
(through mprotect()). Not sure if this would be possible or portable,
but we wanted to at least mention it.

========================================================================

Finally, a few minor bugs/questions.

========================================================================

11/ In src/pdkim/pdkim.c, pdkim_parse_sig_header(), there might be a
missing break at the end of the case 'a'.

========================================================================

12/ Same function, the call to pdkim_strtrim() seems to be useless
because whitespace is already skipped by:

 531     if (c == '\r' || c == '\n' || c == ' ' || c == '\t')
 532       goto NEXT_CHAR;

But we have not fully analyzed this, so we might be missing something.

========================================================================

13/ Same file, HEADER_BUFFER_FRAG_SIZE seems to be unused?

========================================================================

14/ In src/smtp_in.c, bdat_getc(), dkim_exim_verify_feed() is called but
does nothing because dkim_collect_input is always set to 0 at the
beginning of bdat_getc().

========================================================================

We are at your disposal for questions, comments, and further
discussions. Thank you very much!

With best regards,

--
the Qualys Security Advisory team


[https://d1dejaj6dcqv24.cloudfront.net/asset/image/email-banner-384-2x.png]<https://www.qualys.com/email-banner>


This message may contain confidential and privileged information. If it has been sent to you in error, please reply to advise the sender of the error and then immediately delete it. If you are not the intended recipient, do not read, copy, disclose or otherwise use this message. The sender disclaims any liability for such unauthorized use. NOTE that all incoming emails sent to Qualys email accounts will be archived and may be scanned by us and/or by external service providers to detect and prevent threats to our systems, investigate illegal or inappropriate behavior, and/or eliminate unsolicited promotional emails (“spam”). If you have any concerns about this process, please contact us.
_______________________________________________
Security mailing list
Security@exim.org
https://lists.exim.org/mailman/listinfo/security