[U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities
Andrea Barisani
andrea.barisani at f-secure.com
Wed Nov 14 14:45:38 UTC 2018
On Wed, Nov 14, 2018 at 01:03:12PM +0100, Simon Goldschmidt wrote:
> 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.
>
I understand your point, however what does happen when we enter the 'case
TFTP_DATA' and we are in the first block received, so we trigger
new_transfer() that sets the tftp_block_wrap_offset to 0 *and*
tftp_mcast_active is set?
I don't see any protection for this case for the underflow, am I wrong?
static void new_transfer(void)
{
tftp_prev_block = 0;
tftp_block_wrap = 0;
tftp_block_wrap_offset = 0;
#ifdef CONFIG_CMD_TFTPPUT
tftp_put_final_block_sent = 0;
#endif
}
...
case TFTP_DATA:
if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK ||
tftp_state == STATE_RECV_WRQ) {
/* first block received */
tftp_state = STATE_DATA;
tftp_remote_port = src;
new_transfer();
^^^^^^^^^^^^^^^
#ifdef CONFIG_MCAST_TFTP
if (tftp_mcast_active) { /* start!=1 common if mcast */ <<<< HERE
tftp_prev_block = tftp_cur_block - 1;
} else
#endif
if (tftp_cur_block != 1) { /* Assertion */
puts("\nTFTP error: ");
printf("First block is not block 1 (%ld)\n",
tftp_cur_block);
puts("Starting again\n\n");
net_start_again();
break;
}
}
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);
^^^^^^^^^^^^^^^^^^
This should result in having -1 and thus -512 as result of the 'offset' math
that converted to ulong would result in a very large value.
> > }
> >
> > 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...
>
As you see from our report the core issues have been fully tested and
reproduced.
It is true however that the additional remark on the `store_block' function
has only been evaluated by code analysis, in the context of the advisory it
seemed something worth notice in relation to the code structure but again, as
you say we didn't practically test that specific aspect, while everything
else was tested and reproduced.
The vulnerability report highlights two (in our opinion) critical
vulnerabilities, one of which described a secondary aspect only checked by
means of source code analysis.
The secondary aspect that we are discussing does not change the overall
impact of the TFTP bugs, which remains unchanged as arbitrary code execution
can anyway be achieved.
Thanks!
> > 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
--
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"
More information about the U-Boot
mailing list