[Cryptech Tech] Cure53 security audit

[Paul Selkirk]

CT-01-005 MCU: OOB writes through dynamic stack allocations (Critical)

The report singles out pkey_get_attributes, where an overly large array
is allocated on the stack, and then attribute type codes are decoded
from the request buffer into some memory below the stack base. The task
stack is in SDRAM1, which goes from 0xc0000000 to 0xc3ffffff. There are
4 RPC dispatch tasks, but we always use the first available, so the
attacker who controls communication to the device can be assured that
RPC requests will be handled by task 'dispatch0'. With a map file and a
bit of math, he can corrupt memory anywhere in RAM. Eventually,
hal_xdr_decode_int will notice that it's running past the end of the
request buffer, and return an error without calling
hal_rpc_pkey_get_attributes, but the damage will already be done.

This could definitely be used to corrupt memory, but it doesn't look
like a good way of doing code injection, because it's only writing the
'type' field of the hal_pkey_attribute_t struct.

The report doesn't mention pkey_set_attributes or pkey_match, but a
similar situation exists. In both these cases, the type, length, and
value fields are written to the hal_pkey_attribute_t struct, so there is
a greater danger of code injection (although the value field is a
pointer into the request buffer, rather than arbitrary data). Again,
hal_xdr_decode_int or hal_xdr_decode_variable_opaque_ptr will detect an
overflow of the request buffer, and will return without calling
hal_rpc_pkey_set_attributes.

Solution: Add buffer overflow checks before allocating any stack arrays.

CT-01-006 MCU: Value cast allows a bypass of the size checks (Critical)

Ironically, this is the result of my cleaning up signed/unsigned
comparisons throughout the code base. Previously, the buffer overflow
checks were implicitly unsigned due to type promotion, but with a
compiler warning. Since I was taking the difference between two pointers
(limit - *outbuf), it seemed natural to cast the length to ptrdiff_t.

Solution: Change the explicitly signed check to explicitly unsigned.

CT-01-007 MCU: Buffer overflow via Set- and Get- attributes (Critical)

"A new attribute can be stored with a corrupted length value and value
pointer, specifically using the bypass shown in CT-01-006. This
corrupted attribute is then retrieved using the get_attributes()
endpoint and the corresponding pkey handle."

However, before the malicious attribute is stored, it has to pass a
length check in hal_ks_attribute_insert, so I'm not convinced this is a
viable attack.

Solution: Even if this was a viable attack, it would be prevented by the
XDR fixes for CT-01-006.

NOTE: These attacks all require that the attacker be logged into the
device. Such an attacker already has access to every key on the device,
except the master key.

				paul