[U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities
Simon Goldschmidt
simon.k.r.goldschmidt at gmail.com
Wed Nov 14 19:07:03 UTC 2018
On 14.11.2018 16:51, Simon Goldschmidt wrote:
> On 14.11.2018 16:35, Daniele Bianco wrote:
>> On Wed, Nov 14, 2018 at 04:26:17PM +0100, Andrea Barisani wrote:
>>> On Wed, Nov 14, 2018 at 04:13:00PM +0100, Simon Goldschmidt wrote:
>>>> On 14.11.2018 15:45, Andrea Barisani wrote:
>>>>> 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();
>>>>> ^^^^^^^^^^^^^^^
>>>> See some lines below...
>>>>
>>>>> #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 */
>>>> If tftp_cur_block is 0 for the first block, we stop right away. No chance to
>>>> reach store_block() at that time.
>>>>
>>> CC'ing my colleague Daniele whom can better reply further on this.
>> Hi Simon,
>> the 'if (tftp_cur_block != 1)' is not triggered if 'tftp_mcast_active'
>> is set (and the CONFIG_MCAST_TFTP is defined).
>>
>> Please note the code indentation does not help in this case as it is
>> misleading, but this is because of the #ifdef.
> Ah, now I do see it, thanks for the hint! Indeed, the indentation of
> that else totally hid it from my eyes that the next block wasn't
> executed always!
>
> Luckily, searching through the whole mainline codebase shows no users of
> this option (CONFIG_MCAST_TFTP), so I guess this is not a real world
> problem, currently :-)
+ Joe
Getting better still: multicast tftp (CONFIG_MCAST_TFTP) does not
compile and it's broken since changing from IPaddr_t (an u32) to struct
in_addr four and a half years ago. So we're lucky that this definitively
is not a real world problem!
Joe, should we remove CONFIG_MCAST_TFTP or fix it? Given that it hasn't
been used more than 4 years?
Simon
>
> Thanks for your explanation and your fast response!
>
> Cheers,
> Simon
>
>> Cheers,
>> Daniele
>>
>>>
>>>>> 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.
>>>> Yes. Thanks for that. I'm working on fixing them :-)
>>>>
>>> And that's much appreciated :)
>>>
>>>>> 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.
>>>> In my opinion as well these are critical, yes.
>>>>
>>>>> 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.
>>>> Of course. I'm working on fixing the actual bug and while debugging it tried
>>>> to fix the other thing you mentioned. I could not reproduce it in a test
>>>> setup (where I can freely send tftp packets). That's why I asked. The other
>>>> bugs are of course not affected by this one not being valid.
>>>>
>>> Understood.
>>>
>>> Cheers
>>>
>>>> Thanks for confirming this.
>>>>
>>>> Simon
>>>>
>>>>> 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"
>> --
>> Daniele Bianco
>> Hardware Security | F-Secure
>>
>> <daniele.bianco at f-secure.com> | https://www.f-secure.com
>> GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
>
More information about the U-Boot
mailing list