[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 15:51:33 UTC 2018
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 :-)
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