templates/static/doc/security/CVE-2020-qualys/minor-issues.txt

   1 Date: Thu, 29 Oct 2020 18:45:45 +0000
   2 From: Qualys Security Advisory via Security <security@exim.org>
   3 To: "security@exim.org" <security@exim.org>
   4 Subject: Re: [Exim-Security] Audit
   5 Sender: Security <security-bounces+hs=nodmarc.schlittermann.de@exim.org>
   6 Return-Path: <security-bounces+hs=nodmarc.schlittermann.de@exim.org>
   7 Authentication-Results: mx10.schlittermann.de; spf=pass
   8  smtp.mailfrom=exim.org; dkim=pass header.d=exim.org header.s=d202008
   9  header.a=rsa-sha256; dmarc=none header.from=exim.org
  10 Authentication-Results: exim.org; iprev=pass (mx0a-001ca501.pphosted.com)
  11  smtp.remote-ip=148.163.156.198; spf=pass smtp.mailfrom=qualys.com; dkim=pass
  12  header.d=qualys.com header.s=qualyscom header.a=rsa-sha256; dkim=pass
  13  header.d=qualys.onmicrosoft.com header.s=selector2-qualys-onmicrosoft-com
  14  header.a=rsa-sha256;  dmarc=pass header.from=qualys.com; arc=pass (i=1)
  15  header.s=arcselector9901 arc.oldest-pass=1 smtp.remote-ip=148.163.156.198
  16 Authentication-Results: ppops.net; spf=pass smtp.mailfrom=qsa@qualys.com
  17 authentication-results: exim.org; dkim=none (message not signed)
  18  header.d=none;exim.org; dmarc=none action=none header.from=qualys.com;
  19 X-Spam-Score: 0.0 (/)
  20 Reply-To: Qualys Security Advisory <qsa@qualys.com>
  21
  22 Hi all,
  23
  24 Below are various non-security bugs that we found during our audit.
  25 First, a few bugs that almost have security consequences.
  26
  27 ========================================================================
  28
  29 1/ In src/transports/smtp.c:
  30
  31 2281       int n = sizeof(sx->buffer);
  32 2282       uschar * rsp = sx->buffer;
  33 2283
  34 2284       if (sx->esmtp_sent && (n = Ustrlen(sx->buffer)) < sizeof(sx->buffer)/2)
  35 2285         { rsp = sx->buffer + n + 1; n = sizeof(sx->buffer) - n; }
  36
  37 This should probably be either:
  38
  39 rsp = sx->buffer + n + 1; n = sizeof(sx->buffer) - n - 1;
  40
  41 or:
  42
  43 rsp = sx->buffer + n; n = sizeof(sx->buffer) - n;
  44
  45 (not sure which) to avoid an off-by-one.
  46
  47 ========================================================================
  48
  49 2/ In src/spool_in.c:
  50
  51  462   while (  (len = Ustrlen(big_buffer)) == big_buffer_size-1
  52  463         && big_buffer[len-1] != '\n'
  53  464         )
  54  465     {   /* buffer not big enough for line; certs make this possible */
  55  466     uschar * buf;
  56  467     if (big_buffer_size >= BIG_BUFFER_SIZE*4) goto SPOOL_READ_ERROR;
  57  468     buf = store_get_perm(big_buffer_size *= 2, FALSE);
  58  469     memcpy(buf, big_buffer, --len);
  59
  60 The --len in memcpy() chops off a useful byte (we know for sure that
  61 big_buffer[len-1] is not a '\n' because we entered the while loop).
  62
  63 ========================================================================
  64
  65 3/ In src/deliver.c:
  66
  67  333 static int
  68  334 open_msglog_file(uschar *filename, int mode, uschar **error)
  69  335 {
  70  336 if (Ustrstr(filename, US"/../"))
  71  337   log_write(0, LOG_MAIN|LOG_PANIC,
  72  338     "Attempt to open msglog file path with upward-traversal: '%s'\n", filename);
  73
  74 Should this be LOG_PANIC_DIE instead of LOG_PANIC? Right now it will log
  75 the /../ attempt but will open the file anyway.
  76
  77 ========================================================================
  78
  79 4/ In src/smtp_in.c:
  80
  81 4966     case RCPT_CMD:
  82 4967       HAD(SCH_RCPT);
  83 4968       rcpt_count++;
  84 ....
  85 5123       if (rcpt_count > recipients_max && recipients_max > 0)
  86
  87 In theory this recipients_max check can be bypassed, because the int
  88 rcpt_count can overflow (become negative). In practice this would either
  89 consume too much memory or generate too much network traffic, but maybe
  90 it should be fixed anyway.
  91
  92 ========================================================================
  93
  94 5/ receive_msg() calls dkim_exim_verify_finish(), which sets
  95 dkim_collect_input to 0 and calls pdkim_feed_finish(), which calls
  96 pdkim_header_complete(), which decreases dkim_collect_input to UINT_MAX,
  97 which reactivates the DKIM code.
  98
  99 As a result, pdkim_feed() is called again (through receive_getc at the
 100 end of receive_msg()), but functions like pdkim_finish_bodyhash() and
 101 exim_sha_finish() have already been called (in pdkim_feed_finish()).
 102 This suggests a use-after-free.
 103
 104 But it seems that a use-after-free would happen only with
 105 EVP_DigestFinal() (in exim_sha_finish()), which does not seem to be
 106 reachable via DKIM (no SHA3). But we checked OpenSSL only, not GnuTLS.
 107
 108 Here is a proof of concept that triggers the bug (which came very close
 109 to a security vulnerability):
 110
 111 (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
 112
 113 (gdb) print &dkim_collect_input
 114 $2 = (unsigned int *) 0x55e180386d90 <dkim_collect_input>
 115 (gdb) watch *(unsigned int *) 0x55e180386d90
 116
 117 Hardware watchpoint 1: *(unsigned int *) 0x55e180386d90
 118 Old value = 0
 119 New value = 4294967295
 120 #0  0x000055e18031f805 in pdkim_header_complete (ctx=ctx@entry=0x55e181b9e8e0) at pdkim.c:1006
 121 #1  0x000055e18032106c in pdkim_feed_finish (ctx=0x55e181b9e8e0, return_signatures=0x55e180386d78 <dkim_signatures>, err=err@entry=0x7ffe443e1d00) at pdkim.c:1490
 122 #2  0x000055e1802a3280 in dkim_exim_verify_finish () at dkim.c:328
 123 #3  0x000055e1802c9d1d in receive_msg (extract_recip=extract_recip@entry=0) at receive.c:3409
 124
 125 ========================================================================
 126
 127 6/ In src/pdkim/pdkim.c, pdkim_update_ctx_bodyhash() is sometimes called
 128 with a global orig_data and hence canon_data, and the following line can
 129 therefore modify data that should be constant:
 130
 131  773   canon_data->len = b->bodylength - b->signed_body_bytes;
 132
 133 For example, the following proof of concept sets lineending.len to 0
 134 (this should not be possible):
 135
 136 (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
 137
 138 (gdb) print lineending
 139 $1 = {data = 0x55e18035b2ad "\r\n", len = 2}
 140 (gdb) print &lineending.len
 141 $3 = (size_t *) 0x55e180385948 <lineending+8>
 142 (gdb) watch *(size_t *) 0x55e180385948
 143
 144 Hardware watchpoint 1: *(size_t *) 0x55e180385948
 145 Old value = 2
 146 New value = 0
 147 (gdb) print lineending
 148 $5 = {data = 0x55e18035b2ad "\r\n", len = 0}
 149
 150 ========================================================================
 151
 152 7/ In src/smtp_out.c, read_response_line(), inblock->ptr is not updated
 153 when -1 is returned. This does not seem to have bad consequences, but is
 154 maybe not the intended behavior.
 155
 156 ========================================================================
 157
 158 8/ When gstring_grow() calls store_extend(), oldsize is g->size, and
 159 store_extend() therefore applies its rounding to g->size. But initially,
 160 the rounding was not applied to g->size but to sizeof(gstring) + g->size
 161 (in string_get()). This luckily does not have bad consequences on 64-bit
 162 (because sizeof(gstring) is rounded -- a multiple of alignment), but on
 163 32-bit it may have unintended consequences. We were not able to exploit
 164 this, however.
 165
 166 ========================================================================
 167
 168 9/ In several cases a fixed gstring is built manually, but is then used
 169 with string functions that may grow/extend the gstring and have
 170 unintended consequences. For example, in src/exim.c:
 171
 172  180 void
 173  181 set_process_info(const char *format, ...)
 174  182 {
 175  183 gstring gs = { .size = PROCESS_INFO_SIZE - 2, .ptr = 0, .s = process_info };
 176  184 gstring * g;
 177  185 int len;
 178  186 va_list ap;
 179  187
 180  188 g = string_fmt_append(&gs, "%5d ", (int)getpid());
 181  189 len = g->ptr;
 182  190 va_start(ap, format);
 183  191 if (!string_vformat(g, 0, format, ap))
 184  192   {
 185  193   gs.ptr = len;
 186  194   g = string_cat(&gs, US"**** string overflowed buffer ****");
 187  195   }
 188  196 g = string_catn(g, US"\n", 1);
 189
 190 string_fmt_append() uses the SVFMT_EXTEND flag, and string_cat() and
 191 string_catn() call gstring_grow(). We were not able to exploit this,
 192 however.
 193
 194 ========================================================================
 195
 196 10/ string_vformat_trc() should always call die_tainted() if the format
 197 string is tainted. Otherwise an attacker can exploit %n or overflow the
 198 newformat[] stack buffer.
 199
 200 One note about untainted memory vs. memory corruption: the ideal
 201 long-term solution would be to make all untainted memory read-only
 202 (through mprotect()). Not sure if this would be possible or portable,
 203 but we wanted to at least mention it.
 204
 205 ========================================================================
 206
 207 Finally, a few minor bugs/questions.
 208
 209 ========================================================================
 210
 211 11/ In src/pdkim/pdkim.c, pdkim_parse_sig_header(), there might be a
 212 missing break at the end of the case 'a'.
 213
 214 ========================================================================
 215
 216 12/ Same function, the call to pdkim_strtrim() seems to be useless
 217 because whitespace is already skipped by:
 218
 219  531     if (c == '\r' || c == '\n' || c == ' ' || c == '\t')
 220  532       goto NEXT_CHAR;
 221
 222 But we have not fully analyzed this, so we might be missing something.
 223
 224 ========================================================================
 225
 226 13/ Same file, HEADER_BUFFER_FRAG_SIZE seems to be unused?
 227
 228 ========================================================================
 229
 230 14/ In src/smtp_in.c, bdat_getc(), dkim_exim_verify_feed() is called but
 231 does nothing because dkim_collect_input is always set to 0 at the
 232 beginning of bdat_getc().
 233
 234 ========================================================================
 235
 236 We are at your disposal for questions, comments, and further
 237 discussions. Thank you very much!
 238
 239 With best regards,
 240
 241 --
 242 the Qualys Security Advisory team
 243
 244
 245 [https://d1dejaj6dcqv24.cloudfront.net/asset/image/email-banner-384-2x.png]<https://www.qualys.com/email-banner>
 246
 247
 248
 249 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.
 250 _______________________________________________
 251 Security mailing list
 252 Security@exim.org
 253 https://lists.exim.org/mailman/listinfo/security