X-Git-Url: https://git.exim.org/exim-website.git/blobdiff_plain/a2de87c485edb179c59783c693781c93819a1a5d..34fcdc20a1610c9c171db62fcd694613e4cd09c0:/templates/static/doc/security/CVE-2020-qualys/patches1.txt?ds=sidebyside diff --git a/templates/static/doc/security/CVE-2020-qualys/patches1.txt b/templates/static/doc/security/CVE-2020-qualys/patches1.txt new file mode 100644 index 0000000..89e47b1 --- /dev/null +++ b/templates/static/doc/security/CVE-2020-qualys/patches1.txt @@ -0,0 +1,234 @@ +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 "(". */ + 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