templates/static/doc/security/CVE-2020-qualys/patches1.txt

   1 Hi Phil, all,
   2
   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.
   9
  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).
  15
  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.
  21
  22 ========================================================================
  23 commit 0f57feb40a719cb7f50485ebb1f2d826d46f443d
  24
  25     SECURITY: fix Qualys CVE-2020-SLCWD
  26
  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.
  30
  31 ------------------------------------------------------------------------
  32
  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:
  36
  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
  39 vulnerable.
  40
  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:
  45
  46     {
  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. */
  51     *p = '\0';
  52     }
  53
  54 with (warning, untested):
  55
  56     {
  57     p += 4;
  58     snprintf(p, big_buffer_size - (p - big_buffer), "%s", initial_cwd);
  59     p += strlen(p);
  60     }
  61
  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).
  64
  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).
  68
  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
  71 please see below.
  72
  73 ------------------------------------------------------------------------
  74
  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
  78 changes its sign.
  79
  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).
  84
  85 ------------------------------------------------------------------------
  86
  87 One small typo, there is an extra " in the comment:
  88
  89   "reasonable" situations".
  90
  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).
  93
  94 ========================================================================
  95 commit 0d75e8d865032ce3b9998bbe12ee9836c444e2e7
  96
  97     SECURITY: length limits on many cmdline options
  98
  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.
 104
 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.
 108
 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?
 112
 113 ------------------------------------------------------------------------
 114
 115 +PP/03 Impose security length checks on various command-line options.
 116 +      Fixes CVE-2020-SPRSS reported by Qualys.
 117
 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
 121 sprintf()s:
 122
 123   if (deliver_selectstring)
 124     p += sprintf(CS p, " -R%s %s", f.deliver_selectstring_regex? "r" : "",
 125       deliver_selectstring);
 126
 127 with (warning, untested):
 128
 129   if (deliver_selectstring)
 130     {
 131     snprintf(p, big_buffer_size - (p - big_buffer),
 132       " -R%s %s", f.deliver_selectstring_regex? "r" : "",
 133       deliver_selectstring);
 134     p += strlen(p);
 135     }
 136
 137 ========================================================================
 138 commit 8a50c88a3fa52ef526777472d7788ffbfc507125
 139
 140     SECURITY: fix Qualys CVE-2020-PFPSN
 141
 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:
 144
 145             if (ss < s)
 146               {
 147               /* Someone has ended the string with "<punct>(". */
 148               ss = s;
 149               }
 150             else
 151               {
 152               Ustrncpy(t, s, ss-s);
 153               t += ss-s;
 154               s = ss;
 155               }
 156
 157 with (warning, untested):
 158
 159             if (ss > s)
 160               {
 161               Ustrncpy(t, s, ss-s);
 162               t += ss-s;
 163               s = ss;
 164               }
 165
 166 ========================================================================
 167 commit 76a1ce77519aec06c001070b734d7b230a8558f1
 168
 169     SECURITY: fix Qualys CVE-2020-PFPZA
 170
 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:
 174
 175 ------------------------------------------------------------------------
 176 if (!len)
 177   {
 178   return string_copy_taint(US"", is_tainted(phrase));
 179   }
 180
 181 buffer = store_get(len*4, is_tainted(phrase));
 182 ------------------------------------------------------------------------
 183
 184 with (warning, untested):
 185
 186 ------------------------------------------------------------------------
 187 buffer = store_get((len+1)*4, is_tainted(phrase));
 188 ------------------------------------------------------------------------
 189
 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)?
 192
 193 ========================================================================
 194 commit b34d3046751bbf5a37f1c439bc974e8661fb4895
 195
 196     SECURITY: refuse too small store allocations
 197
 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:
 203
 204 if (size < 0)
 205
 206 with (warning, untested -- this limits the size of a single allocation
 207 to 1GB):
 208
 209 if (size < 0 || size >= INT_MAX/2)
 210
 211 Note: as mentioned earlier, the allocation functions should use size_t
 212 arguments, not int sizes.
 213
 214 ========================================================================
 215 commit e3b441f7ad997052164eab3a3e9c61b5222dccfa
 216
 217     SECURITY: Avoid integer overflow on too many recipients
 218
 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.
 222
 223 ========================================================================
 224
 225 Sorry for the extensive review; hopefully you will find our comments
 226 useful. We will review the remaining commits tomorrow.
 227
 228 Thank you very much! We are at your disposal for questions, comments,
 229 and further discussions.
 230
 231 With best regards,
 232
 233 --
 234 the Qualys Security Advisory team