Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AES-CTR and AES-CBC #265

Merged

Conversation

kentakayama
Copy link
Contributor

This PR adds COSE AES-CTR and AES-CBC defined in RFC9459.

Though they are non AEAD cipher and registered as "Deprecated", they are leveraged in SUIT Encrypted Payloads.

@kentakayama
Copy link
Contributor Author

kentakayama commented Oct 23, 2023

The test cases are generated by python-cwt with this script.

#!/usr/bin/env python3

import subprocess
import sys
from cwt import COSE, COSEKey, Recipient

plaintext = b"This is the payload"
a128kw_key = COSEKey.from_symmetric_key("128-bit key xxxx", alg="A128KW")
#print(f"AES-KW key = {{'kty': {a128kw_key.kty}, 'k': h'{a128kw_key.key.hex().upper()}'}}")

print("# AES-CBC Examples")
a128cbc_key = COSEKey.from_symmetric_key(bytes.fromhex("50A2F79CD8028E9256DE13EA7AB27D31"), alg="A128CBC")
#a128cbc_key = COSEKey.from_symmetric_key(alg="A128CBC")
print(f"AES-CBC key = {{'kty': {a128cbc_key.kty}, 'k': h'{a128cbc_key.key.hex().upper()}'}}")
a128cbc_iv = bytes.fromhex("8262A8D72040A5E09A314586395D5750")
#a128cbc_iv = a128cbc_key.generate_nonce()

r = Recipient.new(unprotected={"alg": "A128KW"}, sender_key=a128kw_key)
sender = COSE.new()
encoded = sender.encode_and_encrypt(
    plaintext,
    a128cbc_key,
    protected={"alg": "A128CBC"},
    unprotected={
        "iv": a128cbc_iv,
    },
    recipients=[r],
)

# The recipient side:
recipient = COSE.new()
decrypted_payload = recipient.decode(encoded, keys = [a128kw_key])
assert plaintext == decrypted_payload

output = subprocess.check_output(f"echo {encoded.hex().upper()} | pretty2diag.rb -e", shell=True).decode(sys.stdout.encoding).rstrip("\n")
print(f"COSE_Encrypt: {output}")
print(f"in hex: {encoded.hex().upper()}")
print(f"Decrypted payload: {decrypted_payload}")
print()


print("# AES-CTR Examples")
a128ctr_key = COSEKey.from_symmetric_key(bytes.fromhex("29D535405CB0E83994524F7F30D34DAC"), alg="A128CTR")
#a128ctr_key = COSEKey.from_symmetric_key(alg="A128CTR")
print(f"AES-CTR key = {{'kty': {a128ctr_key.kty}, 'k': h'{a128ctr_key.key.hex().upper()}'}}")
a128ctr_iv = bytes.fromhex("77D35242A191E9F9FD26104AB308564E")
#a128ctr_iv = a128ctr_key.geerate_nonce()

r = Recipient.new(unprotected={"alg": "A128KW"}, sender_key=a128kw_key)
sender = COSE.new()
encoded = sender.encode_and_encrypt(
    plaintext,
    a128ctr_key,
    protected={"alg": "A128CTR"},
    unprotected={
        "iv": a128ctr_iv,
    },
    recipients=[r],
)

# The recipient side:
recipient = COSE.new()
decrypted_payload = recipient.decode(encoded, keys = [a128kw_key])
assert plaintext == decrypted_payload

output = subprocess.check_output(f"echo {encoded.hex().upper()} | pretty2diag.rb -e", shell=True).decode(sys.stdout.encoding).rstrip("\n")
print(f"COSE_Encrypt: {output}")
print(f"in hex: {encoded.hex().upper()}")
print(f"Decrypted payload: {decrypted_payload}")
print()

and its output is

# AES-CBC Examples
AES-CBC key = {'kty': 4, 'k': h'50A2F79CD8028E9256DE13EA7AB27D31'}
COSE_Encrypt: 96([<< {1: -65531} >>, {5: h'8262A8D72040A5E09A314586395D5750'}, h'B60EC3C9A166D8EEFDAEA598FAB1CEBB7F0D5DDF2237F13A03A91364594983FE', [[h'', {1: -3}, h'D21A60F7F67BA866F15BE27C983881AA47DF15AE6925A0D1']]])
in hex: D8608445A10139FFFAA105508262A8D72040A5E09A314586395D57505820B60EC3C9A166D8EEFDAEA598FAB1CEBB7F0D5DDF2237F13A03A91364594983FE818340A101225818D21A60F7F67BA866F15BE27C983881AA47DF15AE6925A0D1
Decrypted payload: b'This is the payload'

# AES-CTR Examples
AES-CTR key = {'kty': 4, 'k': h'29D535405CB0E83994524F7F30D34DAC'}
COSE_Encrypt: 96([<< {1: -65534} >>, {5: h'77D35242A191E9F9FD26104AB308564E'}, h'793A616A5617482BE76F17077858B148241591', [[h'', {1: -3}, h'316ADA13003FE7C61AD4EA503500B285CACADE5107A8E280']]])
in hex: D8608445A10139FFFDA1055077D35242A191E9F9FD26104AB308564E53793A616A5617482BE76F17077858B148241591818340A101225818316ADA13003FE7C61AD4EA503500B285CACADE5107A8E280
Decrypted payload: b'This is the payload'

@laurencelundblade
Copy link
Owner

Hi. Good idea! Thank you. Will look at it to merge soon.

@laurencelundblade
Copy link
Owner

There's a MUST here that I think needs implementing: https://www.rfc-editor.org/rfc/rfc9459.html#name-implementation-consideratio

Also, this can work with ESDH, right?

There's a few uses of key handle that need a cast to suppress warnings. You can probably see them with "make -f Makefile.psa warn".

Still checking but generally looks really good. :-)

@kentakayama
Copy link
Contributor Author

@laurencelundblade
They work well also with ESDH.
May I update esdh_enc_dec_test like this?

diff --git a/test/t_cose_encrypt_decrypt_test.c b/test/t_cose_encrypt_decrypt_test.c
index 28d1a46..cc6f209 100644
--- a/test/t_cose_encrypt_decrypt_test.c
+++ b/test/t_cose_encrypt_decrypt_test.c
@@ -458,7 +458,7 @@ int32_t decrypt_known_good_aeskw_non_aead_test(void)
 
 
 static int32_t
-esdh_enc_dec(int32_t curve)
+esdh_enc_dec(int32_t curve, int32_t payload_cose_algorithm_id)
 {
     enum t_cose_err_t                result;
     struct t_cose_key                privatekey;
@@ -497,7 +497,7 @@ esdh_enc_dec(int32_t curve)
      */
     t_cose_encrypt_enc_init(&enc_ctx,
                              T_COSE_OPT_MESSAGE_TYPE_ENCRYPT,
-                             T_COSE_ALGORITHM_A128GCM);
+                             payload_cose_algorithm_id);
 
     /* Create the recipient object telling it the algorithm and the public key
      * for the COSE_Recipient it's going to make.
@@ -565,11 +565,32 @@ esdh_enc_dec_test(void)
         return INT32_MIN;
     }
 
-    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_256);
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_256, T_COSE_ALGORITHM_A128GCM);
+    if(result) {
+        return result;
+    }
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_256, T_COSE_ALGORITHM_A128CTR);
+    if(result) {
+        return result;
+    }
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_256, T_COSE_ALGORITHM_A128CBC);
+    if(result) {
+        return result;
+    }
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_521, T_COSE_ALGORITHM_A256GCM);
     if(result) {
         return result;
     }
-    return esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_521);
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_521, T_COSE_ALGORITHM_A256CTR);
+    if(result) {
+        return result;
+    }
+    result = esdh_enc_dec(T_COSE_ELLIPTIC_CURVE_P_521, T_COSE_ALGORITHM_A256CBC);
+    if(result) {
+        return result;
+    }
+
+    return 0;
 }
 
 

@kentakayama
Copy link
Contributor Author

kentakayama commented Oct 30, 2023

One concern is that t_cose encodes the algorithm id into protected header, but it is not actually protected if we use AES-CTR or AES-CBC because they cannot handle Enc_structure as AAD.
Should t_cose move it into unprotected header for non AEAD cipher? And which function is appropriate?

NOTE: Example COSE_Encrypt binary generated by t_cose.

96([
  / protected: / << {
    / alg / 1: -65534 / A128CTR /
  } >>,
  / unprotected: / {
    / IV / 5: h'EE666B8979D37AA864E516C9B93FAF16'
  },
  / ciphertext: / h'FA8822B633D73C1A1D3C64829DE5E6C40D04CE',
  / recipients: / [
    [
      / protected: / << {
        / alg / 1: -29 / ES-ECDH+A128KW /
      } >>,
      / unprotected: / {
        / ephemeral key / -1: {
          / kty / 1: 2 / EC2 /,
          / crv / -1: 1 / P-256 /,
          / x / -2: h'5C706A67B66D1D7A2D23F61C63BF9A324187B1A489B988D6CFBE984A05FC40A5',
          / y / -3: h'00C217E8D88BCD3C35A0BD76134780FB64084D4D3C7012F5D055D22CC89E8009'
        },
        / kid / 4: 'fixed_test_key_id'
      },
      / ciphertext: / h'1B2CF1F257660D6C36458C42B081E43EFF651D9A452CBE34'
    ]
  ]
])

in hex

D8608445A10139FFFDA10550EE666B8979D37AA864E516C9B93FAF1653FA8822B633D73C1A1D3C64829DE5E6C40D04CE818344A101381CA220A4010220012158205C706A67B66D1D7A2D23F61C63BF9A324187B1A489B988D6CFBE984A05FC40A522582000C217E8D88BCD3C35A0BD76134780FB64084D4D3C7012F5D055D22CC89E8009045166697865645F746573745F6B65795F696458181B2CF1F257660D6C36458C42B081E43EFF651D9A452CBE34

@laurencelundblade
Copy link
Owner

RFC 9459 says "The 'protected' header MUST be a zero-length byte string.", so we're going to have to move the alg ID to unprotected and check that protected is empty. That will be a bit of work... It doesn't actually say anything about where the alg ID parameter goes, but it can't go in the protected header.

Copy link
Owner

@laurencelundblade laurencelundblade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the thorough job, especially the OSSL stuff. Mostly these are minor. The big one is the protected headers.

See you at the hackathon? I hope. :-)

crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_openssl_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
crypto_adapters/t_cose_psa_crypto.c Outdated Show resolved Hide resolved
src/t_cose_crypto.h Outdated Show resolved Hide resolved
}

buffer_bytes_used = 0;
/* This assumes a stream cipher and no need for handling blocks.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AES-CBC is not a stream cipher, so the comment is partly wrong.

I would also feel much more comfortable with this if there was a test for for to verify this code works correctly for various sizes that are not on block boundaries. I'm more worried about OpenSSL than PSA.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did more checking and this seems OK. The esdh_enc_dec() test successfully encrypts with a payload of 19 bytes for both PSA and OpenSSL.

PSA clearly uses the PKCS7 padding mode of AES CBC which clearly is for this purpose. I couldn't find documentation for OSSL for this, but it tests correctly in esdh_enc_dec().

So just up the comment to say that it works with streaming and with block modes that have a padding scheme like PKCS7 padding.

src/t_cose_encrypt_enc.c Outdated Show resolved Hide resolved
@laurencelundblade
Copy link
Owner

Yes, update esdh_enc_dec_test() to take the symmetric cipher too.

@kentakayama kentakayama force-pushed the add-aes-ctr-and-aes-cbc branch from cb46b8a to 01b9086 Compare November 4, 2023 13:41
@kentakayama
Copy link
Contributor Author

@laurencelundblade
Now t_cose produces like this,

96([
  << {} >> / XXX: encoded as [0x41 0xA0] NOT [0x40] = a zero-length byte string /,
  {1: -65534, 5: h'7CA66EE24E777BE2BCEC9896B7E26695'},
  h'F74CF551239A05449DE450706A8E0BF707A4A1',
  [
    [
      << {1: -29} >>,
      {-1:
        {
          1: 2,
          -1: 1,
          -2: h'C0BD9B891CD4CEE309144ED6957A574960478769FDDCA8458A8FD9D6C370C6E6',
          -3: h'6FA334C159C14A8B4EA9FC418B8A4D4EE76A99C6CCCE6645E4EAE675C4D6A2BB'
        },
        4: h'66697865645F746573745F6B65795F6964'
      },
      h'08680232AE9C8D2BBEB2E636CE990D011A9E12B3CFA513AA'
    ]
  ]
])

It seems to be updated encoding protected header.

@laurencelundblade
Copy link
Owner

Yes, working on fix for empty prot header encoding now.

@kentakayama kentakayama marked this pull request as draft November 4, 2023 14:08
@laurencelundblade laurencelundblade marked this pull request as ready for review November 4, 2023 15:03
Copy link
Owner

@laurencelundblade laurencelundblade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really close...

ce_alg.cose_alg_id = me->payload_cose_algorithm_id;
ce_alg.bits_in_key = bits_in_crypto_alg(ce_alg.cose_alg_id);
ce_alg.bits_iv = bits_iv_alg(ce_alg.cose_alg_id);
if(ce_alg.bits_in_key == UINT32_MAX) {
return T_COSE_ERR_UNSUPPORTED_CIPHER_ALG;
}
params[0] = t_cose_param_make_alg_id(ce_alg.cose_alg_id);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use t_cose_param_make_unprot_alg_id(). Then you can get rid of the loop below.

&nonce);
params[1] = t_cose_param_make_iv(nonce);

params[0].next = &params[1];
params[1].next = me->added_body_parameters;
/* At this point all the header parameters to be encoded are in a
* linked list the head of which is params[0]. */
if(is_none_aead_cipher) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed. See above.

}

buffer_bytes_used = 0;
/* This assumes a stream cipher and no need for handling blocks.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did more checking and this seems OK. The esdh_enc_dec() test successfully encrypts with a payload of 19 bytes for both PSA and OpenSSL.

PSA clearly uses the PKCS7 padding mode of AES CBC which clearly is for this purpose. I couldn't find documentation for OSSL for this, but it tests correctly in esdh_enc_dec().

So just up the comment to say that it works with streaming and with block modes that have a padding scheme like PKCS7 padding.

@@ -193,6 +193,16 @@ t_cose_encrypt_dec_detached(struct t_cose_encrypt_dec_ctx* me,

nonce_cbor = t_cose_param_find_iv(body_params_list);
ce_alg.cose_alg_id = t_cose_param_find_alg_id_prot(body_params_list);
if(ce_alg.cose_alg_id == T_COSE_ALGORITHM_NONE) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use t_cose_param_find_alg_id() here instead. It will return the protection and you can check it.

The problem is there is still a check needed that there are no other protected headers. I guess you just iterate over the linked list. Was hoping to use t_cose_headers_decode(,,true,,,) for that, but it doesn't work.

@laurencelundblade laurencelundblade merged commit 76cdae4 into laurencelundblade:dev Nov 5, 2023
14 checks passed
@kentakayama
Copy link
Contributor Author

I was completely dead to the world...
Much appreciate to fix them in #269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants