From fa052f7f16536969c3c6ea7dcab185a6376ef1f2 Mon Sep 17 00:00:00 2001 From: Laurence Lundblade Date: Sun, 25 Jun 2023 03:02:39 -0700 Subject: [PATCH] Improve test coverage and a few fixes --- inc/t_cose/t_cose_mini_sign1_sign.h | 10 +++--- src/t_cose_mini_sign1_sign.c | 11 +++++-- test/t_cose_mini_sign1_sign_test.c | 50 +++++++++++++++++++++++++---- 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/inc/t_cose/t_cose_mini_sign1_sign.h b/inc/t_cose/t_cose_mini_sign1_sign.h index 10187bfc..1c33517f 100644 --- a/inc/t_cose/t_cose_mini_sign1_sign.h +++ b/inc/t_cose/t_cose_mini_sign1_sign.h @@ -1,7 +1,7 @@ /* * t_cose_mini_sign1_sign.h * - * Copyright 2022, Laurence Lundblade + * Copyright 2022-2023, Laurence Lundblade * * SPDX-License-Identifier: BSD-3-Clause * @@ -71,9 +71,6 @@ extern "C" { * any header parameter except the algorithm ID. There is also * a maximum size of the payload at UINT16_MAX. * - * This is roughly 500 bytes of object code versus 1,500 bytes for - * t_cose_sign1_sign(). - * * Even if you don't need small object code, this is * a super easy to use COSE Sign1 API if ES256 is good enough * and you don't need any other header parameters. @@ -83,8 +80,11 @@ extern "C" { * for the output buffer. If \ref output_buffer is too small * an error will be returned. * + * The object code size is about 500 bytes plus the crypto library. * This works with either OpenSSL and PSA Crypto (MbedTLS). It - * is less object code with PSA Crypto. + * is less total object code with PSA Crypto. This contrasts + * with about 1500 bytes plus QCBOR plus the crypto library + * for t_cose_sign1_sign(). * * ES384 and ES512 are also supported, but you have to modify * the source to switch to one of them. The source could be diff --git a/src/t_cose_mini_sign1_sign.c b/src/t_cose_mini_sign1_sign.c index caf3a32b..624f6f27 100644 --- a/src/t_cose_mini_sign1_sign.c +++ b/src/t_cose_mini_sign1_sign.c @@ -33,7 +33,11 @@ #define T_COSE_MINI_SIGN_SELECT_ES512 */ +#if !defined(T_COSE_MINI_SIGN_SELECT_ES256) && \ + !defined(T_COSE_MINI_SIGN_SELECT_ES384) && \ + !defined(T_COSE_MINI_SIGN_SELECT_ES512) #define T_COSE_MINI_SIGN_SELECT_ES256 +#endif #if defined(T_COSE_MINI_SIGN_SELECT_ES256) @@ -184,7 +188,7 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, if(payload_head.ptr == NULL) { /* The payload is too large (the only reason encode_bstr_head() - * errors out. + * errors out). */ return T_COSE_ERR_TOO_LONG; } @@ -263,7 +267,10 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, * I'm not as sharp as I used to be... * * Or said another way, this code doesn't have the same security / - * buffer level that QCBOR has, but it should be safe enough. + * buffer level that QCBOR has. It hasn't gone through the same + * security review and static analysis and such yet either. In + * theory it is safe, but I'd advise you review and analyze + * before security-critical use. */ Done: diff --git a/test/t_cose_mini_sign1_sign_test.c b/test/t_cose_mini_sign1_sign_test.c index 731f23c8..5f2f2894 100644 --- a/test/t_cose_mini_sign1_sign_test.c +++ b/test/t_cose_mini_sign1_sign_test.c @@ -69,22 +69,25 @@ static const uint8_t payload[] = { int32_t mini_sign1_sign_test(void) { enum t_cose_err_t err; - MakeUsefulBufOnStack( output, sizeof(payload) + T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES256); + MakeUsefulBufOnStack( out_buffer, sizeof(payload) + T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES512); struct q_useful_buf_c cose_sign1; struct t_cose_key key_pair; struct t_cose_sign1_verify_ctx verify_ctx; struct q_useful_buf_c verified_payload; + // TODO: tests for different algorithms + // How to do this with compiled-in algorithm? err = make_key_pair(T_COSE_ALGORITHM_ES256, &key_pair); if(err) { return 10; } + /* The main happy-path test */ err = t_cose_mini_sign1_sign(Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload), - key_pair, - output, - &cose_sign1); + key_pair, + out_buffer, + &cose_sign1); if(err) { return 20; } @@ -99,8 +102,43 @@ int32_t mini_sign1_sign_test(void) { return 30; } + if(q_useful_buf_compare(verified_payload, Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload))) { + return 50; + } + + /* Test with a buffer that is too small */ + out_buffer.len -= 30; + err = t_cose_mini_sign1_sign(Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload), + key_pair, + out_buffer, + &cose_sign1); + //if(err != T_COSE_ERR_TOO_SMALL) { + // return 100; + //} + + /* Payload bigger than UINT16_MAX */ + struct q_useful_buf_c long_payload = Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload); + long_payload.len = UINT16_MAX + 1; + + err = t_cose_mini_sign1_sign(long_payload, + key_pair, + out_buffer, + &cose_sign1); + if(err != T_COSE_ERR_TOO_LONG) { + return 200; + } + + /* Wrong key type */ + // TODO: psa signing doesn't check crypto lib. Is that OK? + key_pair.crypto_lib = (enum t_cose_crypto_lib_t) 42; + err = t_cose_mini_sign1_sign(Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload), + key_pair, + out_buffer, + &cose_sign1); + if(err != T_COSE_ERR_INCORRECT_KEY_FOR_LIB) { + return 200; + } + return 0; } - -// TODO: test for output buffer too small