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