From d670a0bdfae1703b8c2f2a683ece6ca232482a7b Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 24 Nov 2023 08:39:28 +0100 Subject: [PATCH] feat(core): Fix malformed signatures from Optiga. --- core/.changelog.d/3411.fixed | 1 + core/SConscript.firmware | 2 + core/SConscript.unix | 2 + core/embed/trezorhal/optiga/optiga_commands.c | 94 ++++++++++++++----- crypto/der.c | 35 +++++++ crypto/der.h | 1 + 6 files changed, 109 insertions(+), 26 deletions(-) create mode 100644 core/.changelog.d/3411.fixed diff --git a/core/.changelog.d/3411.fixed b/core/.changelog.d/3411.fixed new file mode 100644 index 0000000000..2a112d3e42 --- /dev/null +++ b/core/.changelog.d/3411.fixed @@ -0,0 +1 @@ +Fix invalid encoding of signatures from Optiga. diff --git a/core/SConscript.firmware b/core/SConscript.firmware index a2919d4e02..d68eee38c8 100644 --- a/core/SConscript.firmware +++ b/core/SConscript.firmware @@ -96,12 +96,14 @@ SOURCE_MOD += [ 'vendor/trezor-crypto/blake256.c', 'vendor/trezor-crypto/blake2b.c', 'vendor/trezor-crypto/blake2s.c', + 'vendor/trezor-crypto/buffer.c', 'vendor/trezor-crypto/chacha20poly1305/chacha20poly1305.c', 'vendor/trezor-crypto/chacha20poly1305/chacha_merged.c', 'vendor/trezor-crypto/chacha20poly1305/poly1305-donna.c', 'vendor/trezor-crypto/chacha20poly1305/rfc7539.c', 'vendor/trezor-crypto/chacha_drbg.c', 'vendor/trezor-crypto/curves.c', + 'vendor/trezor-crypto/der.c', 'vendor/trezor-crypto/ecdsa.c', 'vendor/trezor-crypto/ed25519-donna/curve25519-donna-32bit.c', 'vendor/trezor-crypto/ed25519-donna/curve25519-donna-helpers.c', diff --git a/core/SConscript.unix b/core/SConscript.unix index 7be0f694dd..7cc5e1fc12 100644 --- a/core/SConscript.unix +++ b/core/SConscript.unix @@ -96,12 +96,14 @@ SOURCE_MOD += [ 'vendor/trezor-crypto/blake256.c', 'vendor/trezor-crypto/blake2b.c', 'vendor/trezor-crypto/blake2s.c', + 'vendor/trezor-crypto/buffer.c', 'vendor/trezor-crypto/chacha20poly1305/chacha20poly1305.c', 'vendor/trezor-crypto/chacha20poly1305/chacha_merged.c', 'vendor/trezor-crypto/chacha20poly1305/poly1305-donna.c', 'vendor/trezor-crypto/chacha20poly1305/rfc7539.c', 'vendor/trezor-crypto/chacha_drbg.c', 'vendor/trezor-crypto/curves.c', + 'vendor/trezor-crypto/der.c', 'vendor/trezor-crypto/ecdsa.c', 'vendor/trezor-crypto/ed25519-donna/curve25519-donna-32bit.c', 'vendor/trezor-crypto/ed25519-donna/curve25519-donna-helpers.c', diff --git a/core/embed/trezorhal/optiga/optiga_commands.c b/core/embed/trezorhal/optiga/optiga_commands.c index 183054cafa..0e69ac40ef 100644 --- a/core/embed/trezorhal/optiga/optiga_commands.c +++ b/core/embed/trezorhal/optiga/optiga_commands.c @@ -25,6 +25,7 @@ #include "optiga_commands.h" #include +#include "der.h" #include "ecdsa.h" #include "hmac.h" #include "memzero.h" @@ -49,27 +50,7 @@ const optiga_metadata_item OPTIGA_META_KEY_USE_KEYAGREE = { static const optiga_metadata_item OPTIGA_META_VERSION_DEFAULT = { (const uint8_t *)"\xC1\x02\x00\x00", 4}; -static optiga_result process_output_fixedlen(uint8_t *data, size_t data_size) { - // Expecting data_size bytes of output data in the response. - if (tx_size != 4 + data_size || - (tx_buffer[2] << 8) + tx_buffer[3] != tx_size - 4) { - return OPTIGA_ERR_UNEXPECTED; - } - - if (tx_buffer[0] != 0) { - return OPTIGA_ERR_CMD; - } - - if (data_size != 0) { - memcpy(data, tx_buffer + 4, data_size); - memzero(tx_buffer, tx_size); - } - - return OPTIGA_SUCCESS; -} - -static optiga_result process_output_varlen(uint8_t *data, size_t max_data_size, - size_t *data_size) { +static optiga_result process_output(uint8_t **out_data, size_t *out_size) { // Check that there is no trailing output data in the response. if (tx_size < 4 || (tx_buffer[2] << 8) + tx_buffer[3] != tx_size - 4) { return OPTIGA_ERR_UNEXPECTED; @@ -80,13 +61,48 @@ static optiga_result process_output_varlen(uint8_t *data, size_t max_data_size, return OPTIGA_ERR_CMD; } - // Return result. - if (tx_size - 4 > max_data_size) { + *out_data = tx_buffer + 4; + *out_size = tx_size - 4; + return OPTIGA_SUCCESS; +} + +static optiga_result process_output_fixedlen(uint8_t *data, size_t data_size) { + uint8_t *out_data = NULL; + size_t out_size = 0; + optiga_result ret = process_output(&out_data, &out_size); + if (ret != OPTIGA_SUCCESS) { + return ret; + } + + // Expecting data_size bytes of output data in the response. + if (out_size != data_size) { + return OPTIGA_ERR_UNEXPECTED; + } + + if (data_size != 0) { + memcpy(data, out_data, data_size); + memzero(tx_buffer, tx_size); + } + + return OPTIGA_SUCCESS; +} + +static optiga_result process_output_varlen(uint8_t *data, size_t max_data_size, + size_t *data_size) { + uint8_t *out_data = NULL; + size_t out_size = 0; + optiga_result ret = process_output(&out_data, &out_size); + if (ret != OPTIGA_SUCCESS) { + return ret; + } + + if (out_size > max_data_size) { return OPTIGA_ERR_SIZE; } - *data_size = tx_size - 4; - memcpy(data, tx_buffer + 4, tx_size - 4); + + memcpy(data, out_data, out_size); memzero(tx_buffer, tx_size); + *data_size = out_size; return OPTIGA_SUCCESS; } @@ -544,7 +560,33 @@ optiga_result optiga_calc_sign(uint16_t oid, const uint8_t *digest, return ret; } - return process_output_varlen(signature, max_sig_size, sig_size); + uint8_t *out_data = NULL; + size_t out_size = 0; + ret = process_output(&out_data, &out_size); + if (ret != OPTIGA_SUCCESS) { + return ret; + } + + // On average 1 in every 256 signatures produced by Optiga has an invalid DER + // encoding. We reencode the signature to ensure correctness. + BUFFER_READER sig_reader = {0}; + BUFFER_WRITER sig_writer = {0}; + buffer_reader_init(&sig_reader, out_data, out_size); + buffer_writer_init(&sig_writer, signature, max_sig_size); + + for (int i = 0; i < 2; ++i) { + if (!der_reencode_int(&sig_reader, &sig_writer)) { + return OPTIGA_ERR_PROCESS; + } + } + + if (buffer_remaining(&sig_reader) != 0) { + // Unexpected trailing data. + return OPTIGA_ERR_UNEXPECTED; + } + + *sig_size = buffer_written_size(&sig_writer); + return OPTIGA_SUCCESS; } /* diff --git a/crypto/der.c b/crypto/der.c index d6a585556c..2813978aef 100644 --- a/crypto/der.c +++ b/crypto/der.c @@ -99,3 +99,38 @@ bool der_read_item(BUFFER_READER *buf, DER_ITEM *item) { return buffer_read_buffer(buf, &item->cont, len); } + +// Reencode a positive integer which violates the encoding rules in Rec. ITU-T +// X.690, section 8.3.2 (the bits of the first octet and bit 8 of the second +// octet shall not all be zero). +bool der_reencode_int(BUFFER_READER *reader, BUFFER_WRITER *writer) { + // Read a DER-encoded integer. + DER_ITEM item = {0}; + if (!der_read_item(reader, &item) || item.id != DER_INTEGER) { + return false; + } + + // Strip any leading 0x00 bytes. + buffer_lstrip(&item.cont, 0x00); + size_t len = buffer_remaining(&item.cont); + + // Positive integers should start with one 0x00 byte if and only if the most + // significant byte is >= 0x80. + uint8_t msb = 0; + bool prepend_null = (!buffer_peek(&item.cont, &msb) || msb >= 0x80); + if (prepend_null) { + len += 1; + } + + if (!buffer_put(writer, DER_INTEGER) || !der_write_length(writer, len)) { + return false; + } + + if (prepend_null) { + if (!buffer_put(writer, 0x00)) { + return false; + } + } + + return buffer_write_buffer(writer, &item.cont); +} diff --git a/crypto/der.h b/crypto/der.h index c55bdefd83..4d249c287a 100644 --- a/crypto/der.h +++ b/crypto/der.h @@ -41,5 +41,6 @@ typedef struct { bool __wur der_read_length(BUFFER_READER *buf, size_t *len); bool __wur der_write_length(BUFFER_WRITER *buf, size_t len); bool __wur der_read_item(BUFFER_READER *buf, DER_ITEM *item); +bool __wur der_reencode_int(BUFFER_READER *reader, BUFFER_WRITER *writer); #endif // __DER_H