Improve test coverage and a few fixes

laurencelundblade · Jun 25, 2023 · fa052f7 · fa052f7
1 parent 4d2fbff
commit fa052f7
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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