--- /dev/null
+Hi Phil, all,
+
+On Fri, Nov 13, 2020 at 04:54:44PM +0000, Phil Pennock wrote:
+> The PP/03 argv item length limits was laborious but overdue and should
+> provide class-of-attacks protection; PP/05 inside the memory allocator
+> cuts off various other paths. The fact we had our second BDAT state
+> confusion bug led to my PP/10 changes reworking the handling there to be
+> more robust overall. In theory.
+
+We started to review the first patches. Before we comment on each patch
+separately, we would like to raise an issue that exists in Exim overall:
+int versus size_t. There are many places in Exim where an int is used to
+represent a size, or a length, where clearly a size_t should be used (we
+will discuss two examples below).
+
+We understand that this is certainly historical baggage (qmail has it
+too, and this is how we exploited it), but in today's 64-bit world this
+is an infinite source of problems: int is 32-bit signed, and size_t is
+64-bit unsigned; this leads to integer truncations, integer overflows,
+and signedness errors.
+
+========================================================================
+commit 0f57feb40a719cb7f50485ebb1f2d826d46f443d
+
+ SECURITY: fix Qualys CVE-2020-SLCWD
+
+Unfortunately this does not fix the bug; two reasons: 1/ the vulnerable
+code itself is not fixed (the combination of strncpy() and strlen()) and
+2/ the int versus size_t problem mentioned above.
+
+------------------------------------------------------------------------
+
+1/ The added check on the length of initial_cwd is a good idea
+(defense-in-depth is always a good idea), but the code itself should
+also be fixed; two reasons:
+
+1a/ If the added check does not work as expected (and it does not, here)
+or if something changes in the code one day, then the same code is still
+vulnerable.
+
+1b/ When reading/auditing that code, one has to spend quite some time to
+make sure that it is actually safe: first find the origin of initial_cwd
+and then PATH_MAX versus BIG_BUFFER_SIZE. It would be easier, and safer,
+if the code were self-secure. To avoid too many changes, maybe replace:
+
+ {
+ Ustrncpy(p + 4, initial_cwd, big_buffer_size-5);
+ p += 4 + Ustrlen(initial_cwd);
+ /* in case p is near the end and we don't provide enough space for
+ * string_format to be willing to write. */
+ *p = '\0';
+ }
+
+with (warning, untested):
+
+ {
+ p += 4;
+ snprintf(p, big_buffer_size - (p - big_buffer), "%s", initial_cwd);
+ p += strlen(p);
+ }
+
+(or, instead of "p += strlen(p);" maybe "while (*p) p++;" (or the other
+way around), so it is consistent with the code a few lines below).
+
+snprintf() instead of strncpy() guarantees that the destination string
+is always null-terminated (and is also more efficient: strncpy() fills
+the entire big_buffer with useless null bytes).
+
+Note: the use of Ustrlen() instead of strlen() would be safe here,
+because p points into big_buffer, which is of known/reasonable size; but
+please see below.
+
+------------------------------------------------------------------------
+
+2/ int versus size_t. The added PATH_MAX check is actually broken and
+does not fix the vulnerability, because Ustrlen() is (int)strlen(), and
+since strlen() returns a size_t, this truncates the length and/or
+changes its sign.
+
+Example: if the length of initial_cwd is 2GB (INT_MIN), then
+Ustrlen(initial_cwd) is indeed less than PATH_MAX (it is negative,
+because cast to a signed int), and "p += 4 + Ustrlen(initial_cwd);"
+decreases p out-of-bounds (2GB below big_buffer).
+
+------------------------------------------------------------------------
+
+One small typo, there is an extra " in the comment:
+
+ "reasonable" situations".
+
+(note: if minor typos etc are too much detail for now, please let us
+know and we will not report them in our future mails).
+
+========================================================================
+commit 0d75e8d865032ce3b9998bbe12ee9836c444e2e7
+
+ SECURITY: length limits on many cmdline options
+
+This is a very good idea. But it has the same int versus size_t problem
+discussed above (Ustrlen() versus strlen()): the checks can in theory be
+bypassed via very long strings (negative lengths). itemlen and maxlen in
+exim_str_fail_toolong() and exim_len_fail_toolong() should be size_t not
+int, and Ustrlen() should not cast (i.e., truncate) from size_t to int.
+
+This is "in theory", because we do not know of any OS that accepts a 2GB
+command-line argument. But this might change in the future, and in any
+case we should not depend on the OS to do the right thing for us.
+
+Side note/question: deliver_selectstring is now limited to 256 chars
+(EXIM_EMAILADDR_MAX), but since it can also be a regex, could this
+possibly break some use case somewhere?
+
+------------------------------------------------------------------------
+
++PP/03 Impose security length checks on various command-line options.
++ Fixes CVE-2020-SPRSS reported by Qualys.
+
+While all these checks are a very good idea (defense-in-depth, again),
+we believe that the vulnerable code itself should also be fixed (same
+reasons as mentioned above). Maybe, for example, by replacing the two
+sprintf()s:
+
+ if (deliver_selectstring)
+ p += sprintf(CS p, " -R%s %s", f.deliver_selectstring_regex? "r" : "",
+ deliver_selectstring);
+
+with (warning, untested):
+
+ if (deliver_selectstring)
+ {
+ snprintf(p, big_buffer_size - (p - big_buffer),
+ " -R%s %s", f.deliver_selectstring_regex? "r" : "",
+ deliver_selectstring);
+ p += strlen(p);
+ }
+
+========================================================================
+commit 8a50c88a3fa52ef526777472d7788ffbfc507125
+
+ SECURITY: fix Qualys CVE-2020-PFPSN
+
+Just a thought, but it seems that the "ss = s;" is useless, because ss
+is never used again after that; maybe simply replace:
+
+ if (ss < s)
+ {
+ /* Someone has ended the string with "<punct>(". */
+ ss = s;
+ }
+ else
+ {
+ Ustrncpy(t, s, ss-s);
+ t += ss-s;
+ s = ss;
+ }
+
+with (warning, untested):
+
+ if (ss > s)
+ {
+ Ustrncpy(t, s, ss-s);
+ t += ss-s;
+ s = ss;
+ }
+
+========================================================================
+commit 76a1ce77519aec06c001070b734d7b230a8558f1
+
+ SECURITY: fix Qualys CVE-2020-PFPZA
+
+Just a thought (this function is really convoluted and hard to follow,
+apologies if we are wrong), but the "len*4" sounds like each input char
+can generate up to 4 output chars (in theory); maybe replace:
+
+------------------------------------------------------------------------
+if (!len)
+ {
+ return string_copy_taint(US"", is_tainted(phrase));
+ }
+
+buffer = store_get(len*4, is_tainted(phrase));
+------------------------------------------------------------------------
+
+with (warning, untested):
+
+------------------------------------------------------------------------
+buffer = store_get((len+1)*4, is_tainted(phrase));
+------------------------------------------------------------------------
+
+which would guarantee that there is always room for the null byte and
+the initial "buffer + 1;" (even when len is not 0)?
+
+========================================================================
+commit b34d3046751bbf5a37f1c439bc974e8661fb4895
+
+ SECURITY: refuse too small store allocations
+
+Unfortunately, this fix can be bypassed. Example: the attacker calls
+store_get_3() with a size exactly equal to INT_MAX, which passes the
+test (it is positive), but then the alignment code rounds it up to
+INT_MIN (negative) and re-enables the forward-overflow/back-jump
+attacks. To make it safe, one solution would be to replace:
+
+if (size < 0)
+
+with (warning, untested -- this limits the size of a single allocation
+to 1GB):
+
+if (size < 0 || size >= INT_MAX/2)
+
+Note: as mentioned earlier, the allocation functions should use size_t
+arguments, not int sizes.
+
+========================================================================
+commit e3b441f7ad997052164eab3a3e9c61b5222dccfa
+
+ SECURITY: Avoid integer overflow on too many recipients
+
+This does not actually fix the vulnerability, because LOG_PANIC is used
+instead of LOG_PANIC_DIE: it logs the exploit attempt but then triggers
+the vulnerability anyway.
+
+========================================================================
+
+Sorry for the extensive review; hopefully you will find our comments
+useful. We will review the remaining commits tomorrow.
+
+Thank you very much! We are at your disposal for questions, comments,
+and further discussions.
+
+With best regards,
+
+--
+the Qualys Security Advisory team
git://git.exim.org / exim-website.git / blobdiff