[U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

On 14.11.2018 12:52, Andrea Barisani wrote:
> On Tue, Nov 13, 2018 at 09:57:23PM +0100, Simon Goldschmidt wrote:
>> On 06.11.2018 15:51, Andrea Barisani wrote:
>>> [..]
>>> The issue can be exploited by several means:
>>>
>>>     - An excessively large crafted boot image file is parsed by the
>>>       `tftp_handler` function which lacks any size checks, allowing the memory
>>>       overwrite.
>>>
>>>     - A malicious server can manipulate TFTP packet sequence numbers to store
>>>       downloaded file chunks at arbitrary memory locations, given that the
>>>       sequence number is directly used by the `tftp_handler` function to calculate
>>>       the destination address for downloaded file chunks.
>>>
>>>       Additionally the `store_block` function, used to store downloaded file
>>>       chunks in memory, when invoked by `tftp_handler` with a `tftp_cur_block`
>>>       value of 0, triggers an unchecked integer underflow.
>>>
>>>       This allows to potentially erase memory located before the `loadAddr` when
>>>       a packet is sent with a null, following at least one valid packet.
>> Do you happen to have more details on this suggested integer underflow? I
>> have tried to reproduce it, but I failed to get a memory write address
>> before 'load_addr'. This is because the 'store_block' function does not
>> directly use the underflowed integer as a block counter, but adds
>> 'tcp_block_wrap_offset' to this offset.
>>
>> To me it seems like alternating between '0' and 'not 0' for the block
>> counter could increase memory overwrites, but I fail to see how you can use
>> this to store chunks at arbitrary memory locations. All you can do is
>> subtract one block size from 'tftp_block_wrap_offset'...
>>
>> Simon
>>
> Hello Simon,
>
> the integer underflow can happen if a malicious TFTP server, able to control
> the TFTP packets sequence number, sends a crafted packet with sequence number
> set to 0 during a flow.
>
> This happens because, within the store_block() function, the 'block' argument
> is declared as 'int' and when it is invoked inside tftp_handler() (case
> TFTP_DATA) this value is passed by doing 'tftp_cur_block - 1' (where
> tftp_cur_block is the sequence number extracted from the tftp packet without
> any previous check):
>
> static inline void store_block(int block, uchar *src, unsigned len)
>                                 ^^^^^^^^^ can have negative values (e.g.  -1)
> {
>          ulong offset = block * tftp_block_size + tftp_block_wrap_offset;
>          ^^^^^
>          here if block is -1 the result stored onto offset would be a very
>          large unsigned number, due to type conversions

And this is exatclty my point. This might be bad coding style, but for 
me it works: 'block' is an 'int' and is '-1', so 'block * 
tftp_block_size' is '-512'. Now from the code flow in tftp_handler(), 
it's clear that if we come here with tftp_cur_block == 0 (so 'block' is 
-1), 'tftp_block_wrap_offset' is not 0 but some positive value 'x * 
tftp_block_size' (see function 'update_block_number').

So the resulting 'offset' is '-512 + (x * 512)' where 'x > 0'. I still 
fail to see how this can be a very large positive number resulting in an 
effective negative offset or arbitrary write.

> }
>
> static void tftp_handler(...){
>
> case TFTP_DATA:
>          ...
>                  if (tftp_cur_block == tftp_prev_block) {
>                          /* Same block again; ignore it. */
>                          break;
>                  }
>
>                  tftp_prev_block = tftp_cur_block;
>                  timeout_count_max = tftp_timeout_count_max;
>                  net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
>
>                  store_block(tftp_cur_block - 1, pkt + 2, len);
>                              ^^^^^^^^^^^^^^^^^^
> }
>
> For these reasons the issue does not appear to be merely a "one block size"
> substraction, but rather offset can reach very large values. Unless I am
> missing something that I don't see of course...

So I take it this "bug" report is from reading the code only, not from 
actually testing it and seeing the arbitrary memory write? I wouldn't 
have expected this in a CVE report...

> You should probably prevent the underflow by placing a check against
> tftp_cur_block before the store_block() invocation, but I defer to you for a
> better implementation of the fix as you certainly know the overall logic much
> better.

Don't get me wrong: I'm just yet another user of U-Boot and I don't know 
the code better than you do. In fact, I looked at the tftp code for the 
first time yesterday after reading you report on the tftp issue in detail.


Simon