hunkob commented 4 months ago

01: Missing check of input/output vector count in crypto partition

Summary

When processing messages to the crypto partition in tfm_crypto_api_dispatcher, the number in input and output arguments is neither checked nor passed on to the different crypto interface functions (e.g. tfm_crypto_key_management_interface). This leads to the access of NULL pointers while writing output values.

Impact: Crash of SPE / Denial of Service
Severity: High

Technical Description

Message dispatching of the crypto partition is implemented in secure_fw/partitions/crypto/crypto_init.c:

tfm_crypto_sfn
- verifies it's an PSA_IPC_CALL
- passes message to tfm_crypto_call_srv
tfm_crypto_call_srv
tfm_crypto_api_dispatcher
- does not verify in_len and out_len
- in_len and out_len is not passed to child function
- => child functions cannot check for incorrect in_len and out_len
- => every access to in_vec[1]..in_vec[PSA_MAX_IOVEC] and out_vec[0]..out_vec[PSA_MAX_IOVEC] may lead to an NULL pointer deref
- this affects all child functions, see e.g. tfm_crypto_key_management_interface in secure_fw/partitions/crypto/crypto_key_management.c
- out_vec[0].base:
- in_vec[1].base:
  - https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/+/refs/tags/TF-Mv2.0.0/secure_fw/partitions/crypto/crypto_key_management.c#141
  - https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/+/refs/tags/TF-Mv2.0.0/secure_fw/partitions/crypto/crypto_key_management.c#167

Proposed fix

Overhaul the argument verification and remove implicit and undocumented assumptions of available arguments:

tfm_crypto_api_dispatcher should pass in_len and out_len to all child functions.
All child functions, such as tfm_crypto_key_management_interface, should verify the correct in_len/out_len prior to usage of in_vec/out_vec and return an error if len is insufficient.

Reproducer

Most messages with out_len = 0 will trigger an NULL pointer deref, e.g.:

mailbox_dispatch: CALL handle = 0xefa60140, type = 0x40000000
tfm_crypto_api_dispatcher: in_vec = 0x0800a890, in_len = 1, out_vec = 0x0800a8b0, out_len = 0

group_id = 04 (TFM_CRYPTO_GROUP_ID_CIPHER), function_id = 0304 (TFM_CRYPTO_CIPHER_DECRYPT_SETUP)

in_vec[0]: base = 0x0800a8d0, len = 48
[0800a8d0]  67 11 fa e8  67 11 fa e8  |g...  g...|
[0800a8d8]  67 11 fa e8  67 11 fa e8  |g...  g...|
[0800a8e0]  67 11 fa e8  32 08 ad 24  |g...  2..$|
[0800a8e8]  de 69 b0 a0  05 50 76 dc  |.i..  .Pv.|
[0800a8f0]  00 00 00 00  ab 44 8e 9f  |....  .D..|
[0800a8f8]  00 00 00 00  04 03 f8 ea  |....  ....|

This message leads to a NULL pointer deref:

Memory access (data) of unmapped address 0x00000000 at 0x10000848 (PC)

0x10000848 corresponds to secure_fw/partitions/crypto/crypto_cipher.c:78 which is an access to out_vec[0].base.

Attachments

crash.log: short log file of crashing input (including sent message)
crash.log: trace of all called function + context of crash
frametrace-full-8929149.log: detailed trace of all called function + memory accesses + context of crash

adeaarm commented 1 month ago

The fix for #4 will provide protection for writes on invalid pointers (i.e. NULL pointers and/or insufficient lengths). Protecting reads from invalid pointers adds overhead which is likely not increasing the security of the solution:

If the address being read is covered by MPU settings, it will be caught by the SPM framework and the platform can decide how to handle such accesses in the corresponding fault handler
If the address being read is not covered, an invalid read will just pass invalid data into the API call which will result in invalid output but not a security issue.

It's worth noting that TF-M Threat model does not protect against Denial-of-Service attacks and it is always possible for an NS application to maliciously generate an invalid read or write that will cause DoS, without involving the secure FW at all.