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

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

On 09.11.2018 11:24, Simon Goldschmidt wrote:
> On Fri, Nov 9, 2018 at 10:46 AM Andrea Barisani
> <andrea.barisani@f-secure.com> wrote:
>> On Fri, Nov 09, 2018 at 07:11:36AM +0100, Simon Goldschmidt wrote:
>>> On Fri, Nov 9, 2018 at 1:37 AM Fabio Estevam <festevam@gmail.com> wrote:
>>>> Hi Andrea,
>>>>
>>>> On Tue, Nov 6, 2018 at 12:57 PM Andrea Barisani
>>>> <andrea.barisani@f-secure.com> wrote:
>>>>
>>>>> # load large file
>>>>> => ext2load mmc 0 0x60000000 fitimage.itb
>>>> Does this change work for you?
>>>> http://dark-code.bulix.org/u6gw3b-499924
>>> My understanding was U-Boot text or stack could get overwritten which
>>> leads to the loaded bytes being executed as code.
>>> So you would have to check that the loaded range is within ram but not
>>> within that reserved range of code or stack (or heap).
>>>
>> Exactly, merely checking RAM size is not sufficient. The specific memory
>> layout would need to be accounted for which means understanding where the
>> stack and heap are located, their direction of growth and to ensure that the
>> loaded payload can never overwrite them along with all other U-Boot data
>> segments.
>>
>> This is not easy given that the stack and heap size I think can only be
>> guessed and not precisely limited, additionally board configurations have the
>> ability to set arbitrary stack, relocation and load addresses which
>> complicates things even further in understanding exactly how the memory
>> layout is set.
> It's not easy, but in my opinion, it should already be solved by the
> code in 'boot_start_lmb' mentioned in my last mail.
> This function includes arch and board callbacks that should be able to
> return a safe memory range.

I have a patched version running here with the above mentioned changes 
that should fix this issue. A few cleanups are missing before sending a 
patch though (changes are in fs.c and lmb.c only).

Simon

>
> The only thing that cannot be controlled here is stack size, that's
> true. The ARM port tries to solve this by getting the current stack
> pointer and subtracting "4K to be safe". As far as I know, there are
> no methods in U-Boot currently to ensure this is safe, though. And
> depending on the RAM size, we could just subtract more. Personally, I
> wouldn't mind subtracting some MBytes on my board. Actually using such
> a stack would definively be another bug that needs fixing.
>
> But it seems a good start to use these functions to limit loading from fs, too.
>
> Simon
>
>>> Getting this reserved range is what 'boot_start_lmb' does (in
>>> bootm.c). Maybe this code can be refactored and reused in fs.c to get
>>> a valid range for loading?
>>>
>>> Additionally, your patch checks the loaded file's size without taking
>>> the load address into account. So unless I read that wrong, your check
>>> is only valid for 'addr == 0'.
>>> Plus, the 'bytes' parameter should probably be a restriction to the
>>> file's size when checking for a valid load range.
>>>
>>> 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"