[U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities
Hello everyone, the following advisory has been posted last week to oss-security, per discussion with Tom Rini, whom helped us in the pre-notification phase, I also post it here for discussion.
Cheers
-------------------------------------------------------------------------------
Security advisory: U-Boot verified boot bypass ==============================================
The Universal Boot Loader - U-Boot [1] verified boot feature allows cryptographic authentication of signed kernel images, before their execution.
This feature is essential in maintaining a full chain of trust on systems which are secure booted by means of an hardware anchor.
Multiple techniques have been identified that allow to execute arbitrary code, within a running U-Boot instance, by means of externally provided unauthenticated data.
All such techniques spawn from the lack of memory allocation protection within the U-Boot architecture, which results in several means of providing excessively large images during the boot process.
Some implementers might find the following issues as an intrinsic characteristic of the U-Boot memory model, and consequently a mere aspect of correct U-Boot configuration and command restrictions.
However in our opinion the inability of U-Boot to protect itself when loading binaries is an unexpected result of non trivial understanding, particularly important to emphasize in trusted boot scenarios.
This advisory details two specific techniques that allow to exploit U-Boot lack of memory allocation restrictions, with the most severe case also detailing a workaround to mitigate the issue.
It must be emphasized that cases detailed in the next sections only represent two possible occurrences of such architectural limitation, other U-Boot image loading functions are extremely likely to suffer from the same validation issues.
To a certain extent the identified issues are similar to one of the findings reported as CVE-2018-1000205 [2], however they concern different functions which in some cases are at a lower level, therefore earlier in the boot image loading stage.
Again all such issues are a symptom of the same core architectural limitation, being the lack of memory allocation constraints for received images.
It is highly recommended, for implementers of trusted boot schemes, to review use of all U-Boot booting/loading commands, and not merely the two specific ones involved in the findings below, to apply limitations (where applicable/possible) to the size of loaded images in relation to the available RAM.
It should also be emphasized that any trusted boot scheme must also rely on an appropriate lockdown of all possibilities for interactive consoles, by boot process interruption or failure, to ever be prompted.
U-Boot insufficient boundary checks in filesystem image load ------------------------------------------------------------
The U-Boot bootloader supports kernel loading from a variety of filesystem formats, through the `load` command or its filesystem specific equivalents (e.g. `ext2load`, `ext4load`, `fatload`, etc.)
These commands do not protect system memory from being overwritten when loading files of a length that exceeds the boundaries of the relocated U-Boot memory region, filled with the loaded file starting from the passed `addr` variable.
Therefore an excessively large boot image, saved on the filesystem, can be crafted to overwrite all U-Boot static and runtime memory segments, and in general all device addressable memory starting from the `addr` load address argument.
The memory overwrite can directly lead to arbitrary code execution, fully controlled by the contents of the loaded image.
When verified boot is implemented, the issue allows to bypass its intended validation as the memory overwrite happens before any validation can take place.
The following example illustrates the issue, triggered with a 129MB file on a machine with 128MB or RAM:
``` U-Boot 2018.09-rc1 (Oct 10 2018 - 10:52:54 +0200)
DRAM: 128 MiB Flash: 128 MiB MMC: MMC: 0
# print memory information => bdinfo arch_number = 0x000008E0 boot_params = 0x60002000 DRAM bank = 0x00000000 -> start = 0x60000000 -> size = 0x08000000 DRAM bank = 0x00000001 -> start = 0x80000000 -> size = 0x00000004 eth0name = smc911x-0 ethaddr = 52:54:00:12:34:56 current eth = smc911x-0 ip_addr = <NULL> baudrate = 38400 bps TLB addr = 0x67FF0000 relocaddr = 0x67F96000 reloc off = 0x07796000 irq_sp = 0x67EF5EE0 sp start = 0x67EF5ED0
# load large file => ext2load mmc 0 0x60000000 fitimage.itb
# In this specific example U-Boot falls in an infinite loop, results vary # depending on the test case and filesystem/device driver used. A debugging # session demonstrates memory being overwritten: (gdb) p gd $28 = (volatile gd_t *) 0x67ef5ef8 (gdb) p *gd $27 = {bd = 0x7f7f7f7f, flags = 2139062143, baudrate = 2139062143, ... } (gdb) x/300x 0x67ef5ef8 0x67ef5ef8: 0x7f7f7f7f 0x7f7f7f7f 0x7f7f7f7f 0x7f7f7f7f ```
It can be seen that memory address belonging to U-Boot data segments, in this specific case the global data structure `gd`, is overwritten with payload originating from `fitimage.itb` (filled with `0x7f7f7f7f`).
### Impact
Arbitrary code execution can be achieved within a U-Boot instance by means of unauthenticated binary images, loaded through the `load` command or its filesystem specific equivalents.
It should be emphasized that all load commands are likely to be affected by the same underlying root cause of this vulnerability.
### Workaround
The optional `bytes` argument can be passed to all load commands to restrict the maximum size of the retrieved data.
The issue can be therefore mitigated by passing a `bytes` argument with a value consistent with the U-Boot memory regions mapping and size.
U-Boot insufficient boundary checks in network image boot ---------------------------------------------------------
The U-Boot bootloader supports kernel loading from a variety of network sources, such as TFTP via the `tftpboot` command.
This command does not protect system memory from being overwritten when loading files of a length that exceeds the boundaries of the relocated U-Boot memory region, filled with the loaded file starting from the passed `loadAddr` variable.
Therefore an excessively large boot image, served over TFTP, can be crafted to overwrite all U-Boot static and runtime memory segments, and in general all device addressable memory starting from the `loadAddr` load address argument.
The memory overwrite can directly lead to arbitrary code execution, fully controlled by the contents of the loaded image.
When verified boot is implemented, the issue allows to bypass its intended validation as the memory overwrite happens before any validation can take place.
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.
The following example illustrates the issue, triggered with a 129MB file on a machine with 128MB or RAM:
``` U-Boot 2018.09-rc1 (Oct 10 2018 - 10:52:54 +0200)
DRAM: 128 MiB Flash: 128 MiB MMC: MMC: 0
# print memory information => bdinfo arch_number = 0x000008E0 boot_params = 0x60002000 DRAM bank = 0x00000000 -> start = 0x60000000 -> size = 0x08000000 DRAM bank = 0x00000001 -> start = 0x80000000 -> size = 0x00000004 eth0name = smc911x-0 ethaddr = 52:54:00:12:34:56 current eth = smc911x-0 ip_addr = <NULL> baudrate = 38400 bps TLB addr = 0x67FF0000 relocaddr = 0x67F96000 reloc off = 0x07796000 irq_sp = 0x67EF5EE0 sp start = 0x67EF5ED0
# configure environment => setenv loadaddr 0x60000000 => dhcp smc911x: MAC 52:54:00:12:34:56 smc911x: detected LAN9118 controller smc911x: phy initialized smc911x: MAC 52:54:00:12:34:56 BOOTP broadcast 1 DHCP client bound to address 10.0.0.20 (1022 ms) Using smc911x-0 device TFTP from server 10.0.0.1; our IP address is 10.0.0.20 Filename 'fitimage.bin'. Load address: 0x60000000 Loading: ################################################################# ... ####################################
R00=7f7f7f7f R01=67fedf6e R02=00000000 R03=7f7f7f7f R04=7f7f7f7f R05=7f7f7f7f R06=7f7f7f7f R07=7f7f7f7f R08=7f7f7f7f R09=7f7f7f7f R10=0000d677 R11=67fef670 R12=00000000 R13=67ef5cd0 R14=02427f7f R15=7f7f7f7e PSR=400001f3 -Z-- T S svc32 ```
It can be seen that the program counter (PC, r15) is set to an address originating from `fitimage.itb` (filled with `0x7f7f7f7f`), as the result of the U-Boot memory overwrite.
### Impact
Arbitrary code execution can be achieved within a U-Boot instance by means of unauthenticated binary images, passed through TFTP and loaded through the `tftpboot` command, or by a malicious TFTP server capable of sending arbitrary response packets.
It should be emphasized that all network boot commands are likely to be affected by the same underlying root cause of this vulnerability.
### Workaround
The `tftpboot` command lacks any optional argument to restrict the maximum size of downloaded images, therefore the only workaround at this time is to avoid using this command on environments that require trusted boot.
Affected version ----------------
All released U-Boot versions, at the time of this advisory release, are believed to be vulnerable.
All tests have been performed against U-Boot version 2018.09-rc1.
Credit ------
Vulnerabilities discovered and reported by the Inverse Path team at F-Secure, in collaboration with Quarkslab.
CVE ---
CVE-2018-18440: U-Boot insufficient boundary checks in filesystem image load CVE-2018-18439: U-Boot insufficient boundary checks in network image boot
Timeline --------
2018-10-05: network boot finding identified during internal security audit by Inverse Path team at F-Secure in collaboration with Quarkslab.
2018-10-10: filesystem load finding identified during internal security audit by Inverse Path team at F-Secure.
2018-10-12: vulnerability reported by Inverse Path team at F-Secure to U-Boot core maintainer and Google security, embargo set to 2018-11-02.
2018-10-16: Google closes ticket reporting that ChromeOS is not affected due to their specific environment customizations.
2018-10-17: CVE IDs requested to MITRE and assigned.
2018-11-02: advisory release.
References ----------
[1] https://www.denx.de/wiki/U-Boot [2] https://lists.denx.de/pipermail/u-boot/2018-June/330487.html
Permalink ---------
https://github.com/inversepath/usbarmory/blob/master/software/secure_boot/Se...
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
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).
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
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.
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
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.
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"
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"
Hi Simon,
On Fri, Nov 9, 2018 at 7:25 PM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
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).
That's good news, thanks.
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
On 11/11/18 3:22 PM, Wolfgang Denk wrote:
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
For a board with a device tree all reserved memory areas should be secured against overwriting.
Best regards
Heinrich
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
On Mon, Nov 12, 2018 at 12:22 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/11/18 3:22 PM, Wolfgang Denk wrote:
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
For a board with a device tree all reserved memory areas should be secured against overwriting.
That's why I proposed to use the already existing memory reservation scheme 'lmb' (used in loading boot images).
In your case, 'board_lmb_reserve' should make sure the secure monitor does not get overwritten. The 'arch_lmb_reserve' function for arm already ensures U-Boot text, heap and stack don't get overwritten. It could be improved to reserve +1M to the current stack pointer where it does reserve +4K now.
I am working on a patch for the 'load' issue (which could be reused for the tftp issue). There are some problems with the existing lmb code though, which delayed me a bit. However, given that this doesn't make it into the 2018.11 release, anyway, I figured some more days to get it cleaner won't hurt...
Simon
Best regards
Heinrich
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 11/12/18 7:56 AM, Simon Goldschmidt wrote:
On Mon, Nov 12, 2018 at 12:22 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/11/18 3:22 PM, Wolfgang Denk wrote:
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
For a board with a device tree all reserved memory areas should be secured against overwriting.
That's why I proposed to use the already existing memory reservation scheme 'lmb' (used in loading boot images).
In your case, 'board_lmb_reserve' should make sure the secure monitor does not get overwritten. The 'arch_lmb_reserve' function for arm already ensures U-Boot text, heap and stack don't get overwritten. It could be improved to reserve +1M to the current stack pointer where it does reserve +4K now.
If board_lmb_reserve() should be the solution I would prefer that not each individual board calls board_lmb_reserve() but that some common code is used to iterate over the memory reservations in the device tree.
Cheers
Heinrich
I am working on a patch for the 'load' issue (which could be reused for the tftp issue). There are some problems with the existing lmb code though, which delayed me a bit. However, given that this doesn't make it into the 2018.11 release, anyway, I figured some more days to get it cleaner won't hurt...
Simon
Best regards
Heinrich
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 12.11.2018 19:03, Heinrich Schuchardt wrote:
On 11/12/18 7:56 AM, Simon Goldschmidt wrote:
On Mon, Nov 12, 2018 at 12:22 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/11/18 3:22 PM, Wolfgang Denk wrote:
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
For a board with a device tree all reserved memory areas should be secured against overwriting.
That's why I proposed to use the already existing memory reservation scheme 'lmb' (used in loading boot images).
In your case, 'board_lmb_reserve' should make sure the secure monitor does not get overwritten. The 'arch_lmb_reserve' function for arm already ensures U-Boot text, heap and stack don't get overwritten. It could be improved to reserve +1M to the current stack pointer where it does reserve +4K now.
If board_lmb_reserve() should be the solution I would prefer that not each individual board calls board_lmb_reserve() but that some common code is used to iterate over the memory reservations in the device tree.
I know that the efi loader has its own scheme of memory reservation. I just thought it cleaner to stay with lmb for fs_load and tftp as lmb is already used in image loading/booting.
But using the memory reservations from U-Boot device tree definitively makes sense, thanks for the hint. This should probably be done for the bootm code, too...
Simon
Cheers
Heinrich
I am working on a patch for the 'load' issue (which could be reused for the tftp issue). There are some problems with the existing lmb code though, which delayed me a bit. However, given that this doesn't make it into the 2018.11 release, anyway, I figured some more days to get it cleaner won't hurt...
Simon
Best regards
Heinrich
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Dear Heinrich,
In message 450f8b6e-b2c0-0a5f-14e0-50c58103aec5@gmx.de you wrote:
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
Urgh... Is there a (technical, say hardware) reason for such a unlucky design? Who would willingly fragment memory like that?
For a board with a device tree all reserved memory areas should be secured against overwriting.
True.
Best regards,
Wolfgang Denk
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
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 }
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...
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.
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.
}
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...
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
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
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.
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 :-)
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.
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
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.
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
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.
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@f-secure.com | https://www.f-secure.com GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
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@f-secure.com | https://www.f-secure.com GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
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@f-secure.com> | https://www.f-secure.com GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
Hi Simon, On Wed, Nov 14, 2018 at 1:07 PM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
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?
Seems reasonable to remove MCAST_TFTP.
Cheers, -Joe
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@f-secure.com> | https://www.f-secure.com GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
participants (7)
-
Andrea Barisani -
Daniele Bianco -
Fabio Estevam -
Heinrich Schuchardt -
Joe Hershberger -
Simon Goldschmidt -
Wolfgang Denk