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