* [U-Boot] Buffer overrun risk in UBI SPL for secure boot @ 2019-09-04 4:57 Joel Peshkin 2019-09-04 13:01 ` Heiko Schocher 0 siblings, 1 reply; 4+ messages in thread From: Joel Peshkin @ 2019-09-04 4:57 UTC (permalink / raw) To: u-boot It seems that, in the process of doing any sort of secure boot chain of trust, anything loading a UBI volume in preparation to authenticate it, will load a volume of unknown size into a buffer prior to checking the signature of that volume. Has anyone considered a solution for this? Should all implementations just carve out a buffer at the top of memory for ubispl_load_volume or should the ubispl_load data structure be amended to include a size? It would seem appropriate to include a size, but not clear how to do that without breaking compatibility with existing implementations. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] Buffer overrun risk in UBI SPL for secure boot 2019-09-04 4:57 [U-Boot] Buffer overrun risk in UBI SPL for secure boot Joel Peshkin @ 2019-09-04 13:01 ` Heiko Schocher 2019-09-04 14:19 ` Joel Peshkin 0 siblings, 1 reply; 4+ messages in thread From: Heiko Schocher @ 2019-09-04 13:01 UTC (permalink / raw) To: u-boot Hello Joel, Am 04.09.2019 um 06:57 schrieb Joel Peshkin: > It seems that, in the process of doing any sort of secure boot chain of > trust, anything loading a UBI volume in preparation to authenticate it, > will load a volume of unknown size into a buffer prior to checking the > signature of that volume. Hmm.. it is not an unknown size, it loads the complete UBI Volume, so it is the size of the UBI Volume ... but yes, if an attacker changes the size of UBI Volumes ... Could you please give us a reference to which piece of code you refer? is it this part: drivers/mtd/ubispl/ubispl.c static int ipl_load(struct ubi_scan_info *ubi, const u32 vol_id, uint8_t *laddr) yes ... no size... > Has anyone considered a solution for this? Should all implementations just > carve out a buffer at the top of memory for ubispl_load_volume or should > the ubispl_load data structure be amended to include a size? It would seem > appropriate to include a size, but not clear how to do that without > breaking compatibility with existing implementations. Hmm... yes may an option to add a maxsize to ubispl_load data struct ubispl_load. regarding backwards compatibility, there are not much boards yet, which use this feature: hs at threadripper:u-boot [master] $ grep -lr SPL_UBI | grep defconfig configs/am335x_igep003x_defconfig configs/igep00x0_defconfig hs at threadripper:u-boot [master] $ So this should be solveable problem. bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] Buffer overrun risk in UBI SPL for secure boot 2019-09-04 13:01 ` Heiko Schocher @ 2019-09-04 14:19 ` Joel Peshkin 2019-09-09 21:48 ` Joel Peshkin 0 siblings, 1 reply; 4+ messages in thread From: Joel Peshkin @ 2019-09-04 14:19 UTC (permalink / raw) To: u-boot Hi Heiko, The place where the issue comes up is in ubispl_load_volumes(), but that calls ipl_load() internally. I guess there are several options.... 1) Create a distinct ubispl_scan() function to do the scan without loading anything and then a new load volume function that takes offset and limit arguments. This would have the advantage that a SPL that needs to parse one volume before loading the next would only need to scan once, then could check sizes, then load the individual volumes as needed. At the same time, when we need to validate a FIT header before even deciding what we want to load from it, we would be able to read more incrementally. 2) Add the size limit to struct ubispl_load and not worry about anyone with non-upstreamed code forgetting to set it 3) Add the size limit to struct ubispl_load but have ubispl_load_volumes() set the size limit to 0 (unlimited) before calling a new ubispl_load_volumes_bounded() function. Which do you prefer? Do I presume correctly that your denx.de email address implies that an approach you approve will be acceptable if correctly implemented? Thanks, Joel Peshkin On Wed, Sep 4, 2019 at 6:01 AM Heiko Schocher <hs@denx.de> wrote: > Hello Joel, > > Am 04.09.2019 um 06:57 schrieb Joel Peshkin: > > It seems that, in the process of doing any sort of secure boot chain of > > trust, anything loading a UBI volume in preparation to authenticate it, > > will load a volume of unknown size into a buffer prior to checking the > > signature of that volume. > > Hmm.. it is not an unknown size, it loads the complete UBI Volume, so > it is the size of the UBI Volume ... > but yes, if an attacker changes the size of UBI Volumes ... > > Could you please give us a reference to which piece of code you refer? > > is it this part: > > drivers/mtd/ubispl/ubispl.c > static int ipl_load(struct ubi_scan_info *ubi, const u32 vol_id, uint8_t > *laddr) > > yes ... no size... > > > Has anyone considered a solution for this? Should all implementations > just > > carve out a buffer at the top of memory for ubispl_load_volume or should > > the ubispl_load data structure be amended to include a size? It would > seem > > appropriate to include a size, but not clear how to do that without > > breaking compatibility with existing implementations. > > Hmm... yes may an option to add a maxsize to ubispl_load data struct > ubispl_load. > > regarding backwards compatibility, there are not much boards yet, which > use this feature: > > hs at threadripper:u-boot [master] $ grep -lr SPL_UBI | grep defconfig > configs/am335x_igep003x_defconfig > configs/igep00x0_defconfig > hs at threadripper:u-boot [master] $ > > So this should be solveable problem. > > bye, > Heiko > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] Buffer overrun risk in UBI SPL for secure boot 2019-09-04 14:19 ` Joel Peshkin @ 2019-09-09 21:48 ` Joel Peshkin 0 siblings, 0 replies; 4+ messages in thread From: Joel Peshkin @ 2019-09-09 21:48 UTC (permalink / raw) To: u-boot Hi Heiko, Adding a size limit without breaking things turns out to be much more difficult that it would seem. So, instead of capping the size, we have changed the memory map we are using for uboot. It is probably worthwhile for others using UBISPL in a secure boot nevironment to do the same. Traditionally, uboot SPL or TPL loads or relocates to an address near the top of memory and then builds its stack downwards from the top of memory. That means that any address we use for a volume.load_address will eventually step on something if the volume is large enough. So, we move everything down by a size that is sufficient for any image that UBISPL may need to load (32M) and place the CONFIG_SPL_LOAD_FIT_ADDRESS Above the stack where it can grow without hitting anything until it causes an exception. I'm not sure if there is anything else to be done for this situation except to caution people implementing secure boot environments to be aware of their surroundings. Regards, Joel Peshkin ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-09 21:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-04 4:57 [U-Boot] Buffer overrun risk in UBI SPL for secure boot Joel Peshkin 2019-09-04 13:01 ` Heiko Schocher 2019-09-04 14:19 ` Joel Peshkin 2019-09-09 21:48 ` Joel Peshkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.