[Cryptech Tech] Cure53 security audit

[Paul Selkirk]

Phil pointed out that I wasn't clear on what action I was taking on the
first three MCU vulnerabilities, and that I never got around to publicly
addressing the last two.

As of my previous email, I had committed and pushed changes to fix
CT-01-005, CT-01-006, and (implicitly) CT-01-007. I now consider those
closed.


CT-01-008 MCU: Session reuse via weak client handles (High)

The scenario is that an attacker can piggy-back on an existing logged-in
client connection, by guessing the 32-bit client handle.

Whether (and how well) this works depends on how the client or mux
connects to the USB serial driver. On my Ubuntu box, multiple clients
can connect to /dev/ttyUSB0 without requiring special privileges.
However, uncoordinated writes will collide, and uncoordinated reads will
go to whoever calls read() on the file descriptor. If the mux is being
used, it will read all, some, or none of a response from a non-mux-using
client. In practical terms, this makes it unlikely that the attacker
could, say, export the keys, because the mux would unwittingly intercept
at least part of the response.

Any short-term solution would involve locking the USB serial device
against multiple access, but I'm not enough of a Linux/BSD/MacOS/Windows
guru to know if that's possible. Or, y'know, keep attackers off the host
in the first place?

The long-term solution is, and always has been, Rob's secure channel
proposal: https://wiki.cryptech.is/wiki/SecureChannel


CT-01-009 MCU: OOB read/write via ASN.1 key files with large numbers (High)

> It was discovered that the function that parses ASN.1 keys does not
> check the size of the key's numbers. This makes it possible to
> corrupt memory and potentially execute arbitrary code by providing a
> key with large numbers. The bug can be triggered by importing a
> crafted key file.
> ...
> It is assumed that one of the ENDIAN_LITTLE or ENDIAN_BIG pair is
> always defined, but in case FP_64BIT is defined as well, the length
> in c is not checked and a->used can grow to a value that exceeds the
> size of a->dp.  In turn, this results in an OOB read/write in
> fp_mul_2d().

On the face of it, this is bogus, because the STM32 is 32-bit
little-endian. However, on digging into it, I discovered that
endian-ness wasn't defined, so fp_read_unsigned_bin() was falling
through to the code they highlighted. When I defined ENDIAN_LITTLE,
fp_read_unsigned_bin() was compiled with the optimized 32-bit code,
which includes a length check.

(I don't know why the non-endian and/or 64-bit version wouldn't have a
length check, but that's in third-party code that we don't want to
change without good reason, and that we don't need to change now that
it's no longer relevant.)

fp_read_unsigned_bin() is the only function in libtfm that cares about
endian-ness, or even mentions the ENDIAN_LITTLE and ENDIAN_BIG symbols,
so I'm not surprised we missed it in the initial porting.

In addition to fixing this potential vulnerability, the optimized code
runs 485us faster. Over the course of 13000 calls/RSA signature, this
adds up to 6.3ms/sig. Every little bit helps.

The libtfm Makefile has been patched and committed, so I consider this
issue closed.

				paul