Hi,

This link is not valid anymore (mentioned in README.md):
https://git.trustedfirmware.org/trusted-firmware-m.git/tree/docs/coding_guide.rst

New location of ARM's coding guidleine:
https://git.trustedfirmware.org/trusted-firmware-m.git/tree/docs/about/coding_guide.rst

Not sure all crypto libraries will use the same context for mac_validate and mac_compute like PSA. It is probably not a safe assumption. Perhaps look at what OpenSSL does. Probably need to have two t_cose_crypto_hmac_update() functions, one for compute and one for validate because not all crypto may be the same as PSA

hi,

missing call to EVP_MD_CTX_free() here.

Building t_cose fails with this error below when using latest Mbed TLS and Mbed Crypto without MBEDTLS_ALLOW_PRIVATE_ACCESS option.

$ make -f Makefile.psa
...
test/t_cose_make_psa_test_key.c: In function ‘check_for_key_pair_leaks’:
test/t_cose_make_psa_test_key.c:222:24: error: ‘mbedtls_psa_stats_t’ {aka ‘struct mbedtls_psa_stats_s’} has no member named ‘volatile_slots’; did you mean ‘private_volatile_slots’?
  222 |     return (int)(stats.volatile_slots +
      |                        ^~~~~~~~~~~~~~
      |                        private_volatile_slots
test/t_cose_make_psa_test_key.c:223:18: error: ‘mbedtls_psa_stats_t’ {aka ‘struct mbedtls_psa_stats_s’} has no member named ‘persistent_slots’; did you mean ‘private_persistent_slots’?
  223 |            stats.persistent_slots +
      |                  ^~~~~~~~~~~~~~~~
      |                  private_persistent_slots
test/t_cose_make_psa_test_key.c:224:18: error: ‘mbedtls_psa_stats_t’ {aka ‘struct mbedtls_psa_stats_s’} has no member named ‘external_slots’; did you mean ‘private_external_slots’?
  224 |            stats.external_slots +
      |                  ^~~~~~~~~~~~~~
      |                  private_external_slots
test/t_cose_make_psa_test_key.c:225:18: error: ‘mbedtls_psa_stats_t’ {aka ‘struct mbedtls_psa_stats_s’} has no member named ‘half_filled_slots’; did you mean ‘private_half_filled_slots’?
  225 |            stats.half_filled_slots +
      |                  ^~~~~~~~~~~~~~~~~
      |                  private_half_filled_slots
test/t_cose_make_psa_test_key.c:226:17: error: ‘mbedtls_psa_stats_t’ {aka ‘struct mbedtls_psa_stats_s’} has no member named ‘cache_slots’
  226 |            stats.cache_slots);
      |                 ^
make: *** [<builtin>: test/t_cose_make_psa_test_key.o] Error 1

These are introduced by this commit in Mbed TLS making several variables private.
The macro MBEDTLS_PRIVATE() append prefix private_ to each wrapped variable name if MBEDTLS_ALLOW_PRIVATE_ACCESS is not defined.

There are some options to fix this issue, I think.

• Do not allow t_cose users to use latest Mbed TLS/Crypto without the MBEDTLS_ALLOW_PRIVATE_ACCESS option
• Add CRYPTO_CONFIG_OPTS like T_COSE_DISABLE_PSA_KEY_PAIR_LEAK_TEST in Makefile.psa to skip the test

diff --git a/test/t_cose_make_psa_test_key.c b/test/t_cose_make_psa_test_key.c
index 245d4b7..cfa9e81 100644
--- a/test/t_cose_make_psa_test_key.c
+++ b/test/t_cose_make_psa_test_key.c
@@ -210,8 +210,9 @@ void free_ecdsa_key_pair(struct t_cose_key key_pair)
  */
 int check_for_key_pair_leaks()
 {
-#if defined(T_COSE_USE_PSA_CRYPTO_FROM_MBED_CRYPTO11)
-    /* No way to check for leaks with MBED Crypto 1.1 */
+#if defined(T_COSE_USE_PSA_CRYPTO_FROM_MBED_CRYPTO11) || \
+    defined(T_COSE_DISABLE_PSA_KEY_PAIR_LEAK_TEST)
+    /* No way to check for leaks with Mbed Crypto 1.1 and above 3.1 */
     return 0;
 
 #else

Temporary work-around:

( cat doxygen/t_cose_doxyfile ; echo "OUTPUT_DIRECTORY=" ; echo "INPUT=inc/t_cose" ) | doxygen -

Is it void or not?

Search the mac_compute and mac_validate files for the words "sign" and "verify" and you'll find lots. Many are cut-paste left overs from cloning the signing code. Most should probably changed. This includes documentation, variable names and a little other.

For example "signing_key" should probably be "mac_key" and "verify_key" should be "validate_key".

One can assume (misunderstand) that actually the public key is used to sign the data and not the private key.

Suggestions:

• t_cose_crypto_priv_key_sign(). It can be a good pair with t_cose_crypto_pub_key_verify()
• t_cose_crypto_sign(). An might t_cose_crypto_verify().

In the encrypt and decrypt interface this is just the adding of a useful_buf_c for AAD that can be NULL_USEFUL_BUF_C if not used. The implementation will be a little more work.

This is the compiler output, in my case an error due to -Werror:

t_cose/inc/t_cose/t_cose_sign1_verify.h:423:28: error: compound literals are a C99-specific feature [-Werror,-Wc99-extensions]
    me->verification_key = T_COSE_NULL_KEY;
                           ^
t_cose/inc/t_cose/t_cose_common.h:139:6: note: expanded from macro 'T_COSE_NULL_KEY'
    ((struct t_cose_key){T_COSE_CRYPTO_LIB_UNIDENTIFIED, {0}})

Compiler is clang++ 10.

The TEEP protocol wants to allow using EdDSA (value -8 in the COSE Algorithms registry) with COSE_Sign1

How about changing the interface t_cose_sign_encode_start and t_cose_sign_encode_finish in dev branch?
This is close to #34 , but now we can ignore the backward compatibility on t_cose 2.0.
It will be much simpler, and easy to implement detached content Mac and Encrypt.

Before

enum t_cose_err_t
t_cose_sign_encode_start(struct t_cose_sign_sign_ctx *me,
                         bool                         payload_is_detached,
                         QCBOREncodeContext          *cbor_encode_ctx)
{
    ...
    if(!payload_is_detached) {
        QCBOREncode_BstrWrap(cbor_encode_ctx);
    }
}

enum t_cose_err_t
t_cose_sign_encode_finish(struct t_cose_sign_sign_ctx *me,
                          struct q_useful_buf_c        aad,
                          struct q_useful_buf_c        detached_payload,
                          QCBOREncodeContext          *cbor_encode_ctx)
{
    if(q_useful_buf_c_is_null(detached_payload)) {
        /* Payload is inline, not detached */
        QCBOREncode_CloseBstrWrap2(cbor_encode_ctx, false, &signed_payload);
    } else {
        signed_payload = detached_payload;
    }
    ...
}

enum t_cose_err_t
t_cose_sign_one_short()
{
    return_value = t_cose_sign_encode_start(me, payload_is_detached, &encode_context);
    if(payload_is_detached) {
        QCBOREncode_AddNULL(&encode_context);
    } else {
        QCBOREncode_AddEncoded(&encode_context, payload);
    }
    if(!payload_is_detached) {
        payload = NULL_Q_USEFUL_BUF_C;
    }
    return_value = t_cose_sign_encode_finish(me, aad, payload, &encode_context);
}

After

enum t_cose_err_t
t_cose_sign_one_short()
{
    /*
    COSE_Sign = [
        Headers,
        payload : bstr / nil,
        signatures : [+ COSE_Signature]
    ]
    */

    /* encode `[` and `Headers` */
    return_value = t_cose_sign_encode_start(me, &encode_context);

    /* encode payload */
    if(payload_is_detached) {
        QCBOREncode_AddNULL(&encode_context);
        //XXX: no need to execute       payload = NULL_Q_USEFUL_BUF_C;
    } else {
        QCBOREncode_BstrWrap(cbor_encode_ctx);
        QCBOREncode_AddEncoded(&encode_context, payload);
        QCBOREncode_CloseBstrWrap2(cbor_encode_ctx, false, &signed_payload);
    }

    /* encode (`signatures` or `signature`) and `]` */
    return_value = t_cose_sign_encode_finish(me, aad, payload, &encode_context);
}

We can leave this issue around for a while to see if these get used, but if they don't when 2.0 gets close to release they should be removed.

It seems like a lot of effort was put into code documentation. It would be good to publish the doxygen generated website to GitHub Pages to make it easier to explore the APIs without building the docs locally first.

Right now the HPKE code calls PSA directly. This should instead go through new functions in the crypto adapter for DH and HKDF. That should also result in it working with OpenSSL.

9052 is published!

hi, can t_cose support PS256 signature verification algorithm(COSE_Sign1)? or can I get some hints to implement it.

Remove struct t_cose_parameters from t_cose_common.h. I don’t think it is needed anymore. Also remove all the stuff added to t_cose_common.h because it was added. Put it back in t_cose_sign1_verify.h where it exists for backwards compatibility purposes.

The COSE_Recipient API for both encryption and decryption should be a fully public object the way COSE_Signature is. There should be two objects, one for HPKE and one for KW.

Some of the function names are aligned with the PSA API naming, not with the t_cose naming for Mac0 functions.

The Makefile 'install' targets ('install' and 'install_so') depend on the 'all' target, which includes tests.
I think this is not necessary and not everyone wants that. If you just want to compile and install the library (headers, *.a, and *.so), like myself, then no tests are needed.

The Mac0 implementation only has tests against itself -- that it can validate what it computes. This is a convenient and necessary test, but there should be others against the output from other implementations of collections of know good examples (COSE_Sign1 is tested like this).

Some are available here: https://github.com/cose-wg/Examples/tree/master/mac0-tests

It might reduce object code to implement create_tbm() like create_tbs_hash() where constants are used for some of the CBOR.

It may also reduce object code to have the hmac_update() crypto layer call not return anything like the hash_update() does.

t_cose_sign_encode_start() in t_cose_sign_sign.c lists error codes in

It would be better to define the error codes in t_cose_common.h.

Useful if you only want EdDSA or RSA sign/verify.

New contributors should have a single place to go to for understanding how the project works and how contributions should be made. For code contributions, this could include things like which target branch to use, what the code style is, etc.

All functions, even the _private need documentation. The detached documentation can be short, just a reference to the non-detached.

The order of functions in compute should be init, set*, ..., _compute with the _private functions last.

Be able to add arbitrary headers when encoding and be able to call out to custom decoder when encountering an unknown header.

When encryption was first created the general purpose parameter decoder/encoder wasn't ready. Now it is. Use of it will simplify the encryption code a lot.

This requires the ability to set up a memory pool allocator for the internal QCBOR encoder

t_cose_crypto_is_algorithm_supported() is used in test code to determine at run time which tests to run. It might be good to also use it to determine which Mac0 tests to run.

t_cose should be checked against popular released versions of mbedtls and openssl to be sure it is working with most, hopefully all, of them.

Being able to access protected header, payload, and signature bytes is useful for implementing things like countersignature validation in external libraries.

For inspiration, this is how go-cose exposes headers both decoded and as bytes: https://pkg.go.dev/github.com/veraison/go-cose#Headers
And here the overall structure: https://pkg.go.dev/github.com/veraison/go-cose#Sign1Message

This is the new way to list parameters to encode. See t_cose_sign_sign. this will get rid of the content type methods.

Verification limits the protected headers size to 24 bytes. This is quite large for protected headers and thus not a huge problem.

However the same technique of incremental hashing used with the payload can be used with the protected headers and the limit completely removed.

Would be great if someone added a crypto adaptation layer for PQ!

Support for detached payloads is only partially complete. Test cases are needed for it too. Both on the compute and validate sides. It's actually pretty close as is, so not too much work.

Current t_cose seems not to support "detached content", defined in in RFC 8152 COSE section 4.1.
It sets nil (0xF6) at the payload instead of the bstr-wrapped target payload.

The latest SUIT manifest uses this functionality, and there are examples here.

I've been trying to switch to 1.0 but for some reason signature validation is failing. I'm using the OpenSSL adapter. I can't see anything obvious I'm doing wrong, but I'm sure it is something simple.

The following test program demonstrates the issue (NOTE: this is not production code, doesn't clean up memory, and doesn't do certificate validation):

#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>

#include <openssl/ec.h>
#include <openssl/engine.h>
#include <openssl/err.h>
#include <openssl/evp.h>
#include <t_cose/t_cose_sign1_verify.h>

int main(int argc, char *argv[])
{
  if (argc != 3)
  {
    std::cout << "Usage: " << argv[0] << " <certificate> <cose_sign1>" << std::endl;
    return 1;
  }
  std::string cert_path(argv[1]);
  std::string cose_sign1_path(argv[2]);
  
  std::ifstream cert_file(cert_path, std::ios::binary | std::ios::ate);
  if (!cert_file.good())
  {
    std::cout << "Failed to open certificate file" << std::endl;
    return 1;
  }
  std::streamsize cert_size = cert_file.tellg();
  cert_file.seekg(0, std::ios::beg);
  std::vector<char> cert_buf(cert_size);
  if (!cert_file.read(cert_buf.data(), cert_size))
  {
    std::cout << "Error reading certificate file" << std::endl;
    return 1;
  }
  
  std::ifstream cose_sign1_file(cose_sign1_path, std::ios::binary | std::ios::ate);
  if (!cose_sign1_file.good())
  {
    std::cout << "Failed to open COSE_Sign1 file" << std::endl;
    return 1;
  }
  std::streamsize cose_sign1_size = cose_sign1_file.tellg();
  cose_sign1_file.seekg(0, std::ios::beg);
  std::vector<char> cose_sign1_buf(cose_sign1_size);
  if (!cose_sign1_file.read(cose_sign1_buf.data(), cose_sign1_size))
  {
    std::cout << "Error reading COSE_Sign1 file" << std::endl;
    return 1;
  }
  
  auto buf = BIO_new_mem_buf(cert_buf.data(), cert_buf.size());
  auto cert = PEM_read_bio_X509(buf, NULL, NULL, NULL);
  if (!cert)
  {
    std::cout << "Error reading certificate" << std::endl;
    return 1;
  }
  auto pkey = X509_get_pubkey(cert);
  if (!pkey)
  {
    std::cout << "Error reading public key" << std::endl;
    return 1;
  }

  q_useful_buf_c cose_sign1;
  cose_sign1.ptr = cose_sign1_buf.data();
  cose_sign1.len = cose_sign1_buf.size();

  t_cose_key cose_key;
  cose_key.crypto_lib = T_COSE_CRYPTO_LIB_OPENSSL;
  cose_key.k.key_ptr = pkey;

  t_cose_sign1_verify_ctx verify_ctx;
  t_cose_sign1_verify_init(&verify_ctx, T_COSE_OPT_TAG_REQUIRED);
  t_cose_sign1_set_verification_key(&verify_ctx, cose_key);

  q_useful_buf_c returned_payload;
  t_cose_err_t error = t_cose_sign1_verify(&verify_ctx, cose_sign1, &returned_payload, nullptr);
  if (error)
  {
    std::cout << "Error verifying COSE_Sign1: " << error << std::endl;
    return 1;
  }

  std::cout << "Verified COSE_Sign1" << std::endl;
  return 0;
}

This fails with code 13 (T_COSE_ERR_SIG_VERIFY). In the old version (with EC_KEY) it worked. I also checked the test data against a Python library which can also validate it.

Here is the test data containing two files (COSE_Sign1 message and PEM-encoded certificate): test-data.zip

I did some minimal debugging since T_COSE_ERR_SIG_VERIFY is returned in two code paths. It is the second path after calling EVP_DigestVerify(). I also checked that the tbs hash is the same in 1.0 and before (by dumping it out), and it is. So it must be something else.

Any help is much appreciated.

The convention used for signing and MAC is to have one API for use without a detached payload and separate API for one with a detached payload. The two APIs are similar, but not the same. The same should be done for encryption and decryption.

This was started in the Mac0 commit, but didn't go through and disable all COSE signing code.

Except for the TBS bytes, COSE should allow all arrays and maps to be indefinite length.

After #53 and a few other checks, I want to make an official 1.0 release.

The 2.0 release will contain some major changes such as
Mac0 support
Encryption support
Custom headers support

Not sure which ones yet, will have to see what gets done.

move qcbor_decode_error_to_t_cose_error to the t_cose_util.c file. We want one copy of it for small code size, not multiple for execution speed.