Memory corruption when reading public key modulus from SPI flash to RAM

[jeremyncc]

In the following function, a 16-bit key_length value is read from SPI flash, and is then used as the size argument to a subsequent pfr_spi_read() call.

https://github.com/opencomputeproject/Tektagon-OpenEdition/blob/f7350a3a175373532f2ecafc82745a827397d4a8/zephyr/FunctionalBlocks/Pfr/cerberus/cerberus_pfr_common.c#L381-L388

If a physical attacker has tampered with the contents of SPI flash, they could replace the key_length field with an excessively large value. If this value was larger than RSA_KEY_LENGTH_4K (512 bytes), then memory corruption will occur when copying the public key modulus from flash into RAM.

Between the first and second calls to pfr_spi_read() check key_length to ensure it is not larger than sizeof(public_key->modulus).

Edited to add: I noticed that the AST1060 uses internal flash, so it may not be possible for an adversary to tamper with the public key as described in this report. Are there other platform configurations that use external flash?

[jeremyncc]

I just noticed that a similar problem occurs in another function, shown below. This time, the first argument (the flash ID) is 0 (or BMC_SPI), so unlike above, we are certain that the function is reading from external flash:

https://github.com/opencomputeproject/Tektagon-OpenEdition/blob/f7350a3a175373532f2ecafc82745a827397d4a8/zephyr/FunctionalBlocks/Pfr/cerberus/cerberus_pfr_verification.c#L85-L94

Once again an attacker can tamper with SPI flash to control public_key->mod_length and exponent_length, which leads to memory corruption.

[jeremyncc]

Another example of an attacker-controlled modulus length is shown in these few lines of code:

https://github.com/opencomputeproject/Tektagon-OpenEdition/blob/f7350a3a175373532f2ecafc82745a827397d4a8/zephyr/FunctionalBlocks/Pfr/cerberus/cerberus_pfr_verification.c#L156-L161

[jeremyncc]

This same pattern with an attacker-controlled public_key->mod_length is also repeated here:

https://github.com/opencomputeproject/Tektagon-OpenEdition/blob/f7350a3a175373532f2ecafc82745a827397d4a8/zephyr/ApplicationLayer/tektagon/src/engineManager/engine_manager.c#L98-L103

[jeremyncc]

Finally, one last example of trusting the modulus size is here:

https://github.com/opencomputeproject/Tektagon-OpenEdition/blob/f7350a3a175373532f2ecafc82745a827397d4a8/zephyr/ApplicationLayer/tektagon/src/keystore/KeystoreManager.c#L321-L328

[presannar]

Hi Jeremyncc, Tx for the update. We are working on addressing the issue by proper validation before use. As mentioned AST1060 uses an internal flash but there may be others in future who can have external flash.

[jeremyncc]

Thanks.

BTW, as I showed in the second comment above (#3 (comment)), some of these examples do, in fact, appear to be reading from external flash instead of the internal one.

[jeremyncc]

any update on these?

[vigneshs88]

Hi Jeremy Boone,

We are working on this. We will close this issue by End of Jan 2023.

Regards,
Vignesh

[jeremyncc]

Hi @vigneshs88, any update on this?

[vigneshs88]

Hi Jeremy Boone, Next release is planned in end of Feb/early March. These fixes will be part of the release.

[jeremyncc]

Hi @vigneshs88 , your target date of late-Feb/early-March has passed. When is the next release planned?

[vigneshs88]

Hi @jeremyncc,

The release is planned on or before the 2nd week of Apr 2023.

Regards,
Vignesh

[jeremyncc]

Hi @vigneshs88. I just reviewed the commit (d6d935e) that refactored the repo. I noticed that not all of these memory safety problems have been resolved. I've summarized the results of my review below.

Do you intend to fix these vulnerabilities? I intend to write a blog post about PFR and HRoT security, and will reference these bugs in that blog post, so I would appreciate if you could provide an ETA so that I can coordinate publication.

Issue 1:

File: cerberus/cerberus_pfr_common.c
Function: get_rsa_public_key
Status: Fixed.

The code now reads the entire pubkey in one shot.

Issue 2:

File: cerberus_pfr_verification.c
Function: cerberus_read_public_key
Status: Not fixed.

This function has not changed. The input validation problems still exist.

1. The manifest.Flash.header.length value is read from flash and used as the flash offset in subsequent reads without being validated. This can lead to out-of-bounds reads.
2. The module_length is read from flash and is not validated before being used as the length argument in a subsequent read. This can lead to out-of-bounds writes of the modulus buffer.
3. The exponent_offset is calculated using the tainted module_length and is not validated before use.
4. The exponent_length is read from flash without being verified before use.

Issue 3:

File: cerberus/cerberus_pfr_verification.c
Function: cerberus_verify_regions
Status: Fixed.

The code now reads the entire pubkey in one shot.

Issue 4:

File: engineManager/engine_manager.c
Function: read_rsa_public_key
Status: Not Fixed.

The function has not changed. This code is very similar to cerberus_read_public_key (issue #2 above), and exhibits the same problems with manifest.header.length, module_length, exponent_offset and exponent_length.

Issue 5:

File: keystore/KeystoreManager.c
Function: keystore_get_root_key
Status: Not Fixed.

The function has not changed. This code continues to read key_length from flash without validating it. The key_length is then passed as the length argument to a following SPI flash read operation, leading to an oversized read and out-of-bounds write of the pub_key->modulus buffer.

[presannar]

Hi @jeremyncc,
Thank you for the update. We are working on addressing these issues and will ensure that they are updated to the repository by May 12th 2023. Thank you for your patience.

With Regards,

[jeremyncc]

Hi again. May 12th has past. I'm wondering when this patch will land as I plan on publishing a security advisory (blog post) for these vulnerabilities.

[presannar]

Hi @jeremyncc , We have reviewed the code and the three functions cerberus_read_public_key, read_rsa_public_key, and keystore_get_root_key are not part of the code flow. These functions will be removed. Thank you for your patience.

With Regards,

[jeremyncc]

Thanks. I appreciate the update.