From 6d6a45c73cb17c7e94cb6e253c62ba21c8a0df4a Mon Sep 17 00:00:00 2001 From: Laurence Lundblade Date: Tue, 8 Aug 2023 00:49:41 -0700 Subject: [PATCH] Documentation and test improvements --- Makefile.psa | 4 +- inc/t_cose/t_cose_mini_sign1_sign.h | 55 ++++++----- src/t_cose_mini_sign1_sign.c | 70 ++++++++------ test/t_cose_mini_sign1_sign_test.c | 142 ++++++++++++++++++++-------- 4 files changed, 179 insertions(+), 92 deletions(-) diff --git a/Makefile.psa b/Makefile.psa index 3f9c28f1..038cea58 100644 --- a/Makefile.psa +++ b/Makefile.psa @@ -56,7 +56,7 @@ C_OPTS=-Os -fPIC # ---- T_COSE Config and test options ---- TEST_CONFIG_OPTS= -TEST_OBJ=test/t_cose_test.o test/run_tests.o test/t_cose_sign_verify_test.o test/t_cose_make_test_messages.o $(CRYPTO_TEST_OBJ) +TEST_OBJ=test/t_cose_test.o test/run_tests.o test/t_cose_sign_verify_test.o test/t_cose_make_test_messages.o test/t_cose_mini_sign1_sign_test.o $(CRYPTO_TEST_OBJ) # ---- the main body that is invariant ---- @@ -64,7 +64,7 @@ INC=-I inc -I test -I src ALL_INC=$(INC) $(CRYPTO_INC) $(QCBOR_INC) CFLAGS=$(CMD_LINE) $(ALL_INC) $(C_OPTS) $(TEST_CONFIG_OPTS) $(CRYPTO_CONFIG_OPTS) -SRC_OBJ=src/t_cose_sign1_verify.o src/t_cose_sign1_sign.o src/t_cose_util.o src/t_cose_parameters.o src/t_cose_short_circuit.o +SRC_OBJ=src/t_cose_sign1_verify.o src/t_cose_sign1_sign.o src/t_cose_mini_sign1_sign.o src/t_cose_util.o src/t_cose_parameters.o src/t_cose_short_circuit.o .PHONY: all install install_headers install_so uninstall clean diff --git a/inc/t_cose/t_cose_mini_sign1_sign.h b/inc/t_cose/t_cose_mini_sign1_sign.h index 1c33517f..435f329f 100644 --- a/inc/t_cose/t_cose_mini_sign1_sign.h +++ b/inc/t_cose/t_cose_mini_sign1_sign.h @@ -1,5 +1,5 @@ /* - * t_cose_mini_sign1_sign.h + * t_cose_mini_sign1_sign.h * * Copyright 2022-2023, Laurence Lundblade * @@ -29,7 +29,7 @@ extern "C" { /* The output buffer must be this much larger than the payload size. */ #define T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES256 \ 1 + /* Open the array */ \ - 6 + /* The header parameters */ \ + 5 + /* The header parameters */ \ 3 + /* The CBOR head of the payload */ \ /* The payload -- add this in yourself */ \ 2 + /* CBOR head of signature */ \ @@ -49,7 +49,7 @@ extern "C" { 3 + /* The CBOR head of the payload */ \ /* The payload -- add this in yourself */ \ 2 + /* CBOR head of signature */ \ - 128 /* T_COSE_EC_P512_SIG_SIZE */ + 132 /* T_COSE_EC_P512_SIG_SIZE */ /** @@ -65,34 +65,37 @@ extern "C" { * payload. * Other errors related to invocation of the crypto algorithms. * - * This is a small object code version of t_cose_sign1_sign(). The - * small object code is achieved by fixing the algorithm at compile - * time (the default is COSE ES256) and by not allowing or outputting - * any header parameter except the algorithm ID. There is also - * a maximum size of the payload at UINT16_MAX. + * This signs a payload to make a COSE_Sign1 in the simplest possible + * way. The object code for this is very small. This is achieved by + * fixing the algorithm at compile time, not allowing any header + * parameters but the signing algorithm and limiting the payload size + * to \c UINT16_MAX. The default algorithm is COSE ES256 (EC with the + * secp256r1 curve). + * + * See t_cose_sign1_sign() for full-featured signing. * - * 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. + * The inputs are a payload to sign and a signing key. The signing key + * is a handle or pointer to a key usable with the crypto library this + * is linked against (probably OpenSSL or Mbed TLS). The key + * set up is the same as in the t_cose examples. * - * See \ref T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES256 for a compile time - * constant that gives the size over and above the payload size - * for the output buffer. If \ref output_buffer is too small - * an error will be returned. + * An output buffer must be given sized large enough to hold the + * COSE_Sign1 message produced. The size of this is \ref + * T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES256 larger than the payload. If + * \c 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 total object code with PSA Crypto. This contrasts - * with about 1500 bytes plus QCBOR plus the crypto library - * for t_cose_sign1_sign(). + * This does NOT need to link with a CBOR encoder. It does need to + * link with a cryptographic library. OpenSSL and Mbed TLS are + * supported. It uses the t_cose_crypto.h layer to interface with the + * cryptographic library. It should be easy adapt this to other + * cryptographic libraries. * - * ES384 and ES512 are also supported, but you have to modify - * the source to switch to one of them. The source could be - * further modified to support RSA. + * ES384 and ES512 are also supported, but you have to modify the + * source to switch to one of them. The source could be further + * modified to support RSA. * - * See comments in the source code for changing the algorithm that - * is supported, adding support for headers or reducing the object - * code even futher. + * See comments in the source code to change the algorithm and + * discussion about other modifications. */ enum t_cose_err_t t_cose_mini_sign1_sign(struct q_useful_buf_c payload, diff --git a/src/t_cose_mini_sign1_sign.c b/src/t_cose_mini_sign1_sign.c index 624f6f27..454619c6 100644 --- a/src/t_cose_mini_sign1_sign.c +++ b/src/t_cose_mini_sign1_sign.c @@ -1,7 +1,7 @@ /* * t_cose_mini_sign1_sign.c * - * Copyright 2022, Laurence Lundblade + * Copyright 2022-2023, Laurence Lundblade * * SPDX-License-Identifier: BSD-3-Clause * @@ -12,15 +12,28 @@ #include "t_cose/t_cose_common.h" #include "t_cose_crypto.h" -#include "qcbor/UsefulBuf.h" /* + * This depends on several t_cose header files for definitions + * of COSE constants, error codes and such. It indirectly + * depends on one QCBOR header file for useful_buf. + * + * This depends on a crypto library for the hash and + * public key cryptography. Access to the crypto library + * goes through t_cose_crypto and thus requires the + * C source file for the crypto adaptor for the particular + * crypto library. + * * This could be modified to support other header parameters like the - * kid and the object code would still be bigger, but still very + * kid and the object code would be bigger, but still very * small. * - * Or on the other hand, this could be modified to support only a - * fixed-size payload to make the object code even smaller. + * There's lots that could be done to make this smaller + * if more is known about the use context. For example, + * if the payload is fixed size the CBOR head encoder + * can be removed. The crypto interface is also + * a candidate for optimization. Maybe you call your + * crypto library direct. */ @@ -60,7 +73,7 @@ #define MINI_SIGN_HASH COSE_ALGORITHM_SHA_512 #define MINI_SIGN_HASH_LEN T_COSE_CRYPTO_SHA512_SIZE -#define MINI_SIGN_ALG T_COSE_ALGORITHM_ES521 +#define MINI_SIGN_ALG T_COSE_ALGORITHM_ES512 #define MINI_SIGN_ALG_ID_BYTES 0x38, 0x23 /* The literal byte that appears in the CBOR encoding */ #define MINI_SIGN_SIG_LEN T_COSE_EC_P512_SIG_SIZE @@ -82,7 +95,7 @@ /* * This is hard-coded bytes for the start of the CBOR for the following * CBOR that are the to-be-signed bytes. Hard coding like this saves - * writing code to create it. + * writing code to create it and linking in a CBOR encoder. * * Sig_structure = [ * context : "Signature" / "Signature1" / "CounterSignature", @@ -94,8 +107,8 @@ */ static const uint8_t start_sig_struct[] = { 0x84, - 0x6A,'S', 'i', 'g', 'n', 'a', 't', 'u', 'r', 'e', '1', - PROT_HEADERS, // bstr wrapped protected header wtih algorithm ID + 0x6A, 'S', 'i', 'g', 'n', 'a', 't', 'u', 'r', 'e', '1', + PROT_HEADERS, /* bstr wrapped protected header wtih algorithm ID */ 0x40, /* Empty bstr for aad */ }; @@ -104,12 +117,13 @@ static const uint8_t start_sig_struct[] = { */ static const uint8_t start_cose_sign1[] = { 0x84, - PROT_HEADERS, // bstr wrapped protected header wtih algorithm ID + PROT_HEADERS, /* bstr wrapped protected header wtih algorithm ID */ 0xa0, /* no unprotected headers, put some here if you want */ }; -/* The Hard coded bytes for the CBOR head for the signature. It is less - * code to hard code than to encode using encode_bstr_head() */ +/* The hard coded bytes for the CBOR head for the signature. It is less + * code to hard code than to encode using encode_bstr_head(). Sig + * is always between 24 and 255 bytes so 0x58. */ static const uint8_t cose_sign1_sig_start[] = { 0x58, MINI_SIGN_SIG_LEN }; @@ -187,8 +201,8 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, payload_head = encode_bstr_head(payload.len, payload_head_buffer); if(payload_head.ptr == NULL) { - /* The payload is too large (the only reason encode_bstr_head() - * errors out). + /* The payload is too large, the only reason + * encode_bstr_head() errors out. */ return T_COSE_ERR_TOO_LONG; } @@ -197,6 +211,8 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, /* --- hash the Sig_structure --- */ /* Don't actually have to create the Sig_structure fully in * memory. Just have to compute the hash of it. */ + /* If you are really confident in your crypto library hash , you + * could remove these error checks and save more object code. */ err = t_cose_crypto_hash_start(&hash_ctx, MINI_SIGN_HASH); if(err != T_COSE_SUCCESS) { goto Done; @@ -212,9 +228,9 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, } /* ---- Size check ---- */ - /* Calculate the length of the output buffer required. It is - * just the payload plus a constant. This one check covers - * all the memcpy() calls below. + /* Calculate the length of the output buffer required. It is just + * the payload plus a constant. This one check covers all the + * memcpy() calls and signing operation below. */ const size_t required_len = sizeof(start_cose_sign1) + MAX_CBOR_HEAD + @@ -242,15 +258,15 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, copy_ptr += sizeof(cose_sign1_sig_start); const size_t u_len = (size_t)(copy_ptr - (uint8_t *)output_buffer.ptr); - - - /* This won't go negative because of the check against required_len above - * so the cast is safe. + /* This subtraction won't go negative because of the check against + required_len above */ + + /* Set up for t_cose_crypto_sign to write signature directly + * into output buffer. */ signature_buffer.len = output_buffer.len - u_len; signature_buffer.ptr = copy_ptr; - err = t_cose_crypto_sign(MINI_SIGN_ALG, signing_key, computed_hash, @@ -266,11 +282,11 @@ t_cose_mini_sign1_sign(const struct q_useful_buf_c payload, * UsefulBuf that took a few hours of debugging to find. Or maybe * 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. 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. + * Or said another way, this code doesn't have the same buffer + * security level that QCBOR has. It hasn't gone through the same + * security review and static analysis and such yet either. I am + * pretty confident 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 5f2f2894..2ce58532 100644 --- a/test/t_cose_mini_sign1_sign_test.c +++ b/test/t_cose_mini_sign1_sign_test.c @@ -1,7 +1,7 @@ /* - * t_cose_mini_sign1_sign_test.c + * t_cose_mini_sign1_sign_test.c * - * Copyright 2022, Laurence Lundblade + * Copyright 2022-2023, Laurence Lundblade * * SPDX-License-Identifier: BSD-3-Clause * @@ -14,7 +14,7 @@ #include "t_cose/t_cose_mini_sign1_sign.h" #include "t_cose/t_cose_sign1_verify.h" -static const uint8_t payload[] = { +static const uint8_t sample_payload[] = { 0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x02, 0x03, 0x00, 0x01, 0x02, 0x03, @@ -66,25 +66,17 @@ static const uint8_t payload[] = { }; -int32_t mini_sign1_sign_test(void) { - - enum t_cose_err_t err; - 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; +int32_t +mini_sign1_verify_one_payload(struct q_useful_buf_c payload, + struct t_cose_key key_pair, + struct q_useful_buf out_buffer) +{ struct t_cose_sign1_verify_ctx verify_ctx; struct q_useful_buf_c verified_payload; + enum t_cose_err_t err; + struct q_useful_buf_c cose_sign1; - // 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), + err = t_cose_mini_sign1_sign(payload, key_pair, out_buffer, &cose_sign1); @@ -92,7 +84,6 @@ int32_t mini_sign1_sign_test(void) { return 20; } - t_cose_sign1_verify_init(&verify_ctx, 0); t_cose_sign1_set_verification_key(&verify_ctx, key_pair); @@ -102,22 +93,109 @@ int32_t mini_sign1_sign_test(void) { return 30; } - if(q_useful_buf_compare(verified_payload, Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(payload))) { + if(q_useful_buf_compare(verified_payload, payload)) { return 50; } + return 0; +} + + + +/* Compile the library and this test case with each of these to + * test the compile-time selection of the algorithm. Run the test + * each time. */ +#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) + + #define MINI_SIGN_ALG T_COSE_ALGORITHM_ES256 + #define SIZE_OVERHEAD T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES256 + +#elif defined(T_COSE_MINI_SIGN_SELECT_ES384) + + #define MINI_SIGN_ALG T_COSE_ALGORITHM_ES384 + #define SIZE_OVERHEAD T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES384 + +#elif defined(T_COSE_MINI_SIGN_SELECT_ES512) + + #define MINI_SIGN_ALG T_COSE_ALGORITHM_ES512 + #define SIZE_OVERHEAD T_COSE_MINI_SIGN_SIZE_OVERHEAD_ES512 + +#endif + + +int32_t mini_sign1_sign_test(void) { + + enum t_cose_err_t err; + MakeUsefulBufOnStack( out_buffer, sizeof(sample_payload) + SIZE_OVERHEAD); + struct q_useful_buf_c cose_sign1; + struct t_cose_key key_pair; + struct t_cose_key null_key_pair; + int32_t test_result; + struct q_useful_buf_c payload; + + + // TODO: tests for different algorithms + // How to do this with compiled-in algorithm? + + err = make_key_pair(MINI_SIGN_ALG, &key_pair); + if(err) { + return 10; + } + + /* The main happy-path test for serveral payload sizes */ + payload = Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(sample_payload); + test_result = mini_sign1_verify_one_payload(payload, + key_pair, + out_buffer); + if(test_result) { + return test_result; + } + + payload.len = 254; + test_result = mini_sign1_verify_one_payload(payload, + key_pair, + out_buffer); + if(test_result) { + return test_result; + } + + payload.len = 2; + test_result = mini_sign1_verify_one_payload(payload, + key_pair, + out_buffer); + if(test_result) { + return test_result; + } + + /* With a bad key pair */ + memset(&null_key_pair, 0, sizeof(struct t_cose_key)); + err = t_cose_mini_sign1_sign(Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(sample_payload), + null_key_pair, + out_buffer, + &cose_sign1); + if(err != T_COSE_ERR_SIG_FAIL && err != T_COSE_ERR_INCORRECT_KEY_FOR_LIB) { + return 120; + } + /* 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), + err = t_cose_mini_sign1_sign(Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(sample_payload), key_pair, out_buffer, &cose_sign1); - //if(err != T_COSE_ERR_TOO_SMALL) { - // return 100; - //} + 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); + struct q_useful_buf_c long_payload = Q_USEFUL_BUF_FROM_BYTE_ARRAY_LITERAL(sample_payload); long_payload.len = UINT16_MAX + 1; err = t_cose_mini_sign1_sign(long_payload, @@ -128,17 +206,7 @@ int32_t mini_sign1_sign_test(void) { 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; - } + free_key_pair(key_pair); return 0; } -