3 On Fri, Nov 13, 2020 at 04:54:44PM +0000, Phil Pennock wrote:
4 > The PP/03 argv item length limits was laborious but overdue and should
5 > provide class-of-attacks protection; PP/05 inside the memory allocator
6 > cuts off various other paths. The fact we had our second BDAT state
7 > confusion bug led to my PP/10 changes reworking the handling there to be
8 > more robust overall. In theory.
10 We started to review the first patches. Before we comment on each patch
11 separately, we would like to raise an issue that exists in Exim overall:
12 int versus size_t. There are many places in Exim where an int is used to
13 represent a size, or a length, where clearly a size_t should be used (we
14 will discuss two examples below).
16 We understand that this is certainly historical baggage (qmail has it
17 too, and this is how we exploited it), but in today's 64-bit world this
18 is an infinite source of problems: int is 32-bit signed, and size_t is
19 64-bit unsigned; this leads to integer truncations, integer overflows,
20 and signedness errors.
22 ========================================================================
23 commit 0f57feb40a719cb7f50485ebb1f2d826d46f443d
25 SECURITY: fix Qualys CVE-2020-SLCWD
27 Unfortunately this does not fix the bug; two reasons: 1/ the vulnerable
28 code itself is not fixed (the combination of strncpy() and strlen()) and
29 2/ the int versus size_t problem mentioned above.
31 ------------------------------------------------------------------------
33 1/ The added check on the length of initial_cwd is a good idea
34 (defense-in-depth is always a good idea), but the code itself should
35 also be fixed; two reasons:
37 1a/ If the added check does not work as expected (and it does not, here)
38 or if something changes in the code one day, then the same code is still
41 1b/ When reading/auditing that code, one has to spend quite some time to
42 make sure that it is actually safe: first find the origin of initial_cwd
43 and then PATH_MAX versus BIG_BUFFER_SIZE. It would be easier, and safer,
44 if the code were self-secure. To avoid too many changes, maybe replace:
47 Ustrncpy(p + 4, initial_cwd, big_buffer_size-5);
48 p += 4 + Ustrlen(initial_cwd);
49 /* in case p is near the end and we don't provide enough space for
50 * string_format to be willing to write. */
54 with (warning, untested):
58 snprintf(p, big_buffer_size - (p - big_buffer), "%s", initial_cwd);
62 (or, instead of "p += strlen(p);" maybe "while (*p) p++;" (or the other
63 way around), so it is consistent with the code a few lines below).
65 snprintf() instead of strncpy() guarantees that the destination string
66 is always null-terminated (and is also more efficient: strncpy() fills
67 the entire big_buffer with useless null bytes).
69 Note: the use of Ustrlen() instead of strlen() would be safe here,
70 because p points into big_buffer, which is of known/reasonable size; but
73 ------------------------------------------------------------------------
75 2/ int versus size_t. The added PATH_MAX check is actually broken and
76 does not fix the vulnerability, because Ustrlen() is (int)strlen(), and
77 since strlen() returns a size_t, this truncates the length and/or
80 Example: if the length of initial_cwd is 2GB (INT_MIN), then
81 Ustrlen(initial_cwd) is indeed less than PATH_MAX (it is negative,
82 because cast to a signed int), and "p += 4 + Ustrlen(initial_cwd);"
83 decreases p out-of-bounds (2GB below big_buffer).
85 ------------------------------------------------------------------------
87 One small typo, there is an extra " in the comment:
89 "reasonable" situations".
91 (note: if minor typos etc are too much detail for now, please let us
92 know and we will not report them in our future mails).
94 ========================================================================
95 commit 0d75e8d865032ce3b9998bbe12ee9836c444e2e7
97 SECURITY: length limits on many cmdline options
99 This is a very good idea. But it has the same int versus size_t problem
100 discussed above (Ustrlen() versus strlen()): the checks can in theory be
101 bypassed via very long strings (negative lengths). itemlen and maxlen in
102 exim_str_fail_toolong() and exim_len_fail_toolong() should be size_t not
103 int, and Ustrlen() should not cast (i.e., truncate) from size_t to int.
105 This is "in theory", because we do not know of any OS that accepts a 2GB
106 command-line argument. But this might change in the future, and in any
107 case we should not depend on the OS to do the right thing for us.
109 Side note/question: deliver_selectstring is now limited to 256 chars
110 (EXIM_EMAILADDR_MAX), but since it can also be a regex, could this
111 possibly break some use case somewhere?
113 ------------------------------------------------------------------------
115 +PP/03 Impose security length checks on various command-line options.
116 + Fixes CVE-2020-SPRSS reported by Qualys.
118 While all these checks are a very good idea (defense-in-depth, again),
119 we believe that the vulnerable code itself should also be fixed (same
120 reasons as mentioned above). Maybe, for example, by replacing the two
123 if (deliver_selectstring)
124 p += sprintf(CS p, " -R%s %s", f.deliver_selectstring_regex? "r" : "",
125 deliver_selectstring);
127 with (warning, untested):
129 if (deliver_selectstring)
131 snprintf(p, big_buffer_size - (p - big_buffer),
132 " -R%s %s", f.deliver_selectstring_regex? "r" : "",
133 deliver_selectstring);
137 ========================================================================
138 commit 8a50c88a3fa52ef526777472d7788ffbfc507125
140 SECURITY: fix Qualys CVE-2020-PFPSN
142 Just a thought, but it seems that the "ss = s;" is useless, because ss
143 is never used again after that; maybe simply replace:
147 /* Someone has ended the string with "<punct>(". */
152 Ustrncpy(t, s, ss-s);
157 with (warning, untested):
161 Ustrncpy(t, s, ss-s);
166 ========================================================================
167 commit 76a1ce77519aec06c001070b734d7b230a8558f1
169 SECURITY: fix Qualys CVE-2020-PFPZA
171 Just a thought (this function is really convoluted and hard to follow,
172 apologies if we are wrong), but the "len*4" sounds like each input char
173 can generate up to 4 output chars (in theory); maybe replace:
175 ------------------------------------------------------------------------
178 return string_copy_taint(US"", is_tainted(phrase));
181 buffer = store_get(len*4, is_tainted(phrase));
182 ------------------------------------------------------------------------
184 with (warning, untested):
186 ------------------------------------------------------------------------
187 buffer = store_get((len+1)*4, is_tainted(phrase));
188 ------------------------------------------------------------------------
190 which would guarantee that there is always room for the null byte and
191 the initial "buffer + 1;" (even when len is not 0)?
193 ========================================================================
194 commit b34d3046751bbf5a37f1c439bc974e8661fb4895
196 SECURITY: refuse too small store allocations
198 Unfortunately, this fix can be bypassed. Example: the attacker calls
199 store_get_3() with a size exactly equal to INT_MAX, which passes the
200 test (it is positive), but then the alignment code rounds it up to
201 INT_MIN (negative) and re-enables the forward-overflow/back-jump
202 attacks. To make it safe, one solution would be to replace:
206 with (warning, untested -- this limits the size of a single allocation
209 if (size < 0 || size >= INT_MAX/2)
211 Note: as mentioned earlier, the allocation functions should use size_t
212 arguments, not int sizes.
214 ========================================================================
215 commit e3b441f7ad997052164eab3a3e9c61b5222dccfa
217 SECURITY: Avoid integer overflow on too many recipients
219 This does not actually fix the vulnerability, because LOG_PANIC is used
220 instead of LOG_PANIC_DIE: it logs the exploit attempt but then triggers
221 the vulnerability anyway.
223 ========================================================================
225 Sorry for the extensive review; hopefully you will find our comments
226 useful. We will review the remaining commits tomorrow.
228 Thank you very much! We are at your disposal for questions, comments,
229 and further discussions.
234 the Qualys Security Advisory team