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

[Andrea Barisani <andrea.barisani@f-secure.com>]

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
}

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...

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.

-- 
Andrea Barisani     Head of Hardware Security |     F-Secure
                                      Founder | Inverse Path

https://www.f-secure.com             https://inversepath.com
0x864C9B9E 0A76 074A 02CD E989 CE7F AC3F DA47 578E 864C 9B9E
       "Pluralitas non est ponenda sine necessitate"