sys/crypto: Enable support for AES-192, AES-256 by Ollrogge · Pull Request #16183 · RIOT-OS/RIOT

Ollrogge · 2021-03-11T10:54:12Z

Contribution description

This PR adds support for AES-192 and AES-256. The code in /crypto/aes.c did already support AES-192 and AES-256 but the functionality was not made available. This PR adds the additional interfaces to enable AES-192 and AES-256.

Testing procedure

I have added tests for 192 bit and 256 bit keys to the already existing tests for CBC, CTR and ECB mode. The tests have the same origin as the original ones.

I was unable to find official test vectors for 192 bit and 256 bit for CCM and OCB mode. If someone can point me towards test vectors for these modes I can add them as well.

Issues/PRs references

Fixes #16178, ~~waiting for #16253~~

PeterKietzmann · 2021-03-11T12:32:11Z

Fixes #16178

PeterKietzmann · 2021-03-11T15:02:57Z

@Ollrogge thanks for your contribution, it is very useful!

I have added tests for 192 bit and 256 bit keys to the already existing tests for CBC, CTR and ECB mode. The tests have the same origin as the original ones.

Very nice! Please point to the origin. Did you check NIST SP 800-38A ?
Why do you replace aes_interface with three interfaces for 128/192/256 bit keys? Their structures only differ in key size, which is configured during aes_init anyway, right?
You have extended cipher_context_t, which is fair, to use aes_ functions directly. However, I'm wondering if this obsoletes max_key_size in ciphers.h. Do you have opinions on that? I know, the ciphers module is a beast...

PeterKietzmann · 2021-03-11T15:11:29Z

BTW: All tests pass on native and nrf51dk and the PR currently adds about 500 Bytes ROM.

Ollrogge · 2021-03-11T21:31:59Z

@Ollrogge thanks for your contribution, it is very useful!

I have added tests for 192 bit and 256 bit keys to the already existing tests for CBC, CTR and ECB mode. The tests have the same origin as the original ones.
* Very nice! Please point to the origin. Did you check [NIST SP 800-38A ](https://csrc.nist.gov/publications/detail/sp/800-38a/final)?

Yes I did. Sadly it does not contain test vectors for CCM and OCB mode.

* Why do you replace `aes_interface` with three interfaces for 128/192/256 bit keys? Their structures only differ in key size, which is configured during `aes_init` anyway, right?

True. By removing max_key_size they are the same. I did not create a general interface with a name like CIPHER_AES since this would break all code using the CIPHER_AES_128 interface.

* You have extended `cipher_context_t`, which is fair, to use _aes__ functions directly. However, I'm wondering if this obsoletes [`max_key_size`](https://github.com/RIOT-OS/RIOT/blob/master/sys/include/crypto/ciphers.h#L80) in ciphers.h. Do you have opinions on that? I know, the ciphers module is a beast...

Yes I think it does. I removed max_key_size. The check in ciphers.c is not important I think, since the specific modules check the key size anyways.

PeterKietzmann · 2021-03-12T14:22:44Z

I did not create a general interface with a name like CIPHER_AES since this would break all code using the CIPHER_AES_128 interface

How? Can you explain in a bit more detail? If aes_interface was allocated with something like AES_KEY_SIZE_UNINITIALIZED and in aes_init (which is always called before computing a cipher) you set the keysize for the context (like you did here), what breaks?

PeterKietzmann · 2021-03-12T14:29:37Z

Sadly it does not contain test vectors for CCM and OCB mode.

How about the Cryptographic Algorithm Validation Program CAVP?

The tests have the same origin as the original ones.

What is the origin?

Ollrogge · 2021-03-14T22:18:24Z

How? Can you explain in a bit more detail? If aes_interface was allocated with something like AES_KEY_SIZE_UNINITIALIZED and in aes_init (which is always called before computing a cipher) you set the keysize for the context (like you did here), what breaks?

Sorry, I mean the cipher_id_t which would break other code in my opinion. Currently it is named CIPHER_AES_128 . If I change it to e.g. CIPHER_AES code like this would break.

I could however remove the interfaces and leave just 1 interface called aes_interface and have all ids point to the same interface. I think that is better ?

Ollrogge · 2021-03-14T22:22:57Z

How about the Cryptographic Algorithm Validation Program CAVP?

Nice ! Thank you, these have cases for 192 and 256 bit keys. I will add these.

What is the origin?

this is where I took the additional tests for 192 and 256 bit keys for CBC, CTR and ECB mode from.

PeterKietzmann · 2021-03-15T08:34:32Z

I could however remove the interfaces and leave just 1 interface called aes_interface and have all ids point to the same interface. I think that is better ?

Sounds reasonable!

Ollrogge · 2021-03-16T15:28:14Z

Hey @PeterKietzmann,

I have added the additional AES-CCM test cases for 192 and 256 bit keys.
Additionally I changed the AES interfaces the way we talked about.

PeterKietzmann

@Ollrogge great improvement on the tests! Still some opens comments, though. Also, there is a ~500 Byte increase in the .text segment that I could not explain at first sight. Can you inspect that? Cosy (see #16083) might be of help.

PeterKietzmann · 2021-03-16T17:00:28Z

 */
 static const cipher_interface_t aes_interface = {
    AES_BLOCK_SIZE,
-    AES_KEY_SIZE,


This must be considered as an API change which I would like to avoid here. My comment was only meant for clarification, not to propose such a change.

PeterKietzmann · 2021-03-16T17:02:20Z


    res = aes_set_encrypt_key((unsigned char *)context->context,
-                              AES_KEY_SIZE * 8, &aeskey);
+                              context->key_size * 8, &aeskey);


What if context->key_size is not initialized? This happens easily when a user does not utilize the ciphers module (see Fortuna RNG). Wouldn't be necessary with the old cipher_interface_t struct.

PeterKietzmann · 2021-03-16T17:06:25Z

-
    cipher->interface = cipher_id;
-    return cipher->interface->init(&cipher->context, key, key_size);
+    cipher->context.key_size = key_size;


I would rather deal with that in a follow-up PR and focus on aes_* here (see comment about API change above)

PeterKietzmann · 2021-03-16T17:07:11Z

 int cipher_init(cipher_t *cipher, cipher_id_t cipher_id, const uint8_t *key,
                uint8_t key_size)
 {
-    if (key_size > cipher_id->max_key_size) {


Why remove that change?

PeterKietzmann · 2021-03-16T17:07:48Z

+    return cipher->interface->init(&cipher->context, key, key_size);
 }

-


Maybe don't commit any of the changes in ciphers.c

PeterKietzmann · 2021-03-16T17:09:38Z

-#if defined(MODULE_CRYPTO_3DES)
-    #define CIPHER_MAX_CONTEXT_SIZE 24
-#elif defined(MODULE_CRYPTO_AES)
+#if defined(MODULE_CRYPTO_3DES) || defined(MODULE_CRYPTO_AES)


Before, CIPHER_MAX_CONTEXT_SIZE was 24 for MODULE_CRYPTO_3DES, now is 32.

-> What was the intention behind this?

PeterKietzmann · 2021-03-16T17:10:48Z


    res = aes_set_decrypt_key((unsigned char *)context->context,
-                              AES_KEY_SIZE * 8, &aeskey);
+                              context->key_size * 8, &aeskey);


Same as above.

PeterKietzmann · 2021-03-16T17:11:36Z

 * @brief   the context for cipher-operations
 */
 typedef struct {
+    uint8_t key_size;                          /**< key size used */


See comment above.

PeterKietzmann · 2021-03-16T17:12:05Z

    uint8_t block_size;

-    /** Maximum key size for this cipher */
-    uint8_t max_key_size;


Maybe it is redundant but probably not a task of this PR (see above)

PeterKietzmann · 2021-03-16T17:13:25Z

    uint8_t data[AES_BLOCK_SIZE];

-    err = aes_init(&ctx, TEST_0_KEY, sizeof(TEST_0_KEY));
+    err = cipher_init(&cipher, CIPHER_AES_128, TEST_0_KEY, sizeof(TEST_0_KEY));


We should not replace the plain tests of the AES API with tests of the ciphers API.

leandrolanzieri · 2021-03-22T13:03:21Z

@Ollrogge great improvement on the tests! Still some opens comments, though. Also, there is a ~500 Byte increase in the .text segment that I could not explain at first sight. Can you inspect that? Cosy (see #16083) might be of help.

I checked the .elf files with elf_diff. Most of the difference comes from aes_set_encrypt_key. The implementation gets bigger. Looking at the code my guess is that, as now it is being called with a non-constant size (here), many of the conditionals can't be optimized on compile-time.

An option that might work is to conditionally enable support for the different sizes using pesudomodules. After that, one option could be a macro AES_SIZE(context), which would change between constant numbers or context->key_size depending on the enable modules. Another one would be to add IS_USED() checks on the ifs withing the function and do the optimization manually.

Ollrogge · 2021-03-22T14:06:08Z

Another option @PeterKietzmann and I talked about was to leave the implementation as is and make AES_KEY_SIZE configurable via KConfig. This would be sufficient for my use case.

PeterKietzmann · 2021-03-23T09:45:10Z

@leandrolanzieri +1

Could you show a short piece of pseudo code to assist implementing it?

leandrolanzieri · 2021-03-25T15:52:01Z

I was thinking on defining the pseudomodules on the Makefiles, and then have a macro like:

#if IS_USED(MODULE_AES_128) && !IS_USED(MODULE_AES_192) && !IS_USED(MODULE_AES_256)
#  define KEY_SIZE(ctx) AES_KEY_SIZE_128
#elif !IS_USED(MODULE_AES_128) && IS_USED(MODULE_AES_192) && !IS_USED(MODULE_AES_256)
#  define KEY_SIZE(ctx) AES_KEY_SIZE_192
#elif !IS_USED(MODULE_AES_128) && !IS_USED(MODULE_AES_192) && IS_USED(MODULE_AES_256)
#  define KEY_SIZE(ctx) AES_KEY_SIZE_256
#else
#  define KEY_SIZE(ctx) ctx->keySize
#endif

Ollrogge · 2021-03-25T20:41:36Z

Hey @leandrolanzieri,
I implemented the handling of the AES_KEY_SIZE as you suggested. Using 128 bit keys now only adds 24 Bytes to the .text section.

Additionally I removed all the changes in ciphers.c as you suggested @PeterKietzmann.

I hope its okay that I already squashed the commits. My fixups got really messy in the end.

PeterKietzmann · 2021-04-06T09:20:26Z

#16253 was merged. Please update this PR as discussed offline.

Ollrogge · 2021-04-06T12:20:21Z

I updated the PR based on the changes from #16253 . Using 128 bit keys now adds +8 bytes to the .text section compared to master.

While changing the CIPHERS_MAX_KEY_SIZE to be adjusted based on the key sizes used I noticed that it was 20 bytes before. I don't really understand why because 16 bytes should have been sufficient for 128 bit keys ? When using 16 bytes instead of 20 bytes for 128 bit keys the specific tests still pass.

leandrolanzieri · 2021-04-26T12:02:01Z

@Ollrogge thanks for updating the makefiles! It looks like just a couple of boards are overflowing after splitting the crypto tests. For those you can simply add them to the BOARD_INSUFFICIENT_MEMORY list, like so: https://github.com/RIOT-OS/RIOT/blob/master/tests/pkg_libcoap/Makefile.ci Also, you will need to rebase as there is a conflict with sys/Makefile.dep.

I think the test application names are not really descriptive, could you try to use a more precise name for them? Also, it would be nice to add some README.md in the apps to describe shortly what is tested on both applications, and perhaps a reference to where the test vectors come from.

Ollrogge · 2021-04-27T09:56:23Z

@Ollrogge thanks for updating the makefiles! It looks like just a couple of boards are overflowing after splitting the crypto tests. For those you can simply add them to the BOARD_INSUFFICIENT_MEMORY list, like so: https://github.com/RIOT-OS/RIOT/blob/master/tests/pkg_libcoap/Makefile.ci Also, you will need to rebase as there is a conflict with sys/Makefile.dep.

I think the test application names are not really descriptive, could you try to use a more precise name for them? Also, it would be nice to add some README.md in the apps to describe shortly what is tested on both applications, and perhaps a reference to where the test vectors come from.

Rebased my changes onto upstream/master and added the overflowing boards to BOARD_INSUFFICIENT_MEMORY. I also added the README's to the test applications. I renamed sys_crypto test application to sys_crypto_aes_ccm because this test application is only testing AES_CCM. I don't really know how to rename the sys_crypto test application because it literally tests all parts of the crypto module and therefore the general name seems appropriate to me.

Since my latest fixup commit the Murdock test is failing completely. I don't really understand why ? Can you tell me what I did wrong ?

leandrolanzieri

I added one more comment regarding the dependency resolution. Otherwise it looks good. Feel free to squash after applying the fix @Ollrogge

PeterKietzmann · 2021-05-04T08:15:41Z

My ACK holds, but I haven't followed the latest changes that deal with the size of the tests. @leandrolanzieri wanna merge?

leandrolanzieri · 2021-05-04T15:53:53Z

I'm running tests of the affected modules, just to be on the safe side.

fjmolinas · 2021-05-04T16:25:53Z

I'm running tests of the affected modules, just to be on the safe side.

Tested OpenWSN, nodes join fine (this is the part where crypto is used):

18:25:00 SUCCESS 5de6 [IEEE802154E] synchronized at slotOffset 0
18:25:00 SUCCESS 67eb [IEEE802154E] synchronized at slotOffset 0
18:25:07 INFO 5de6 [C6T] sending CJOIN request
18:25:08 VERBOSE New node: ae8d:fee1:60c7:cb8a. Creating new OSCORE context in /builds/tmp/RIOT/bin/oscore_context_ae8dfee160c7cb8a.json.
18:25:08 VERBOSE received JRC join request
18:25:08 VERBOSE 5de6 [PACKETFUNCTIONS] copy packet content to big packet (pkt len 1300 > max len 125)
18:25:08 VERBOSE 5de6 [PACKETFUNCTIONS] copy packet content to small packet (pkt len 33 < max len 125)
18:25:08 SUCCESS 5de6 [C6T] node joined
18:25:09 INFO 67eb [C6T] sending CJOIN request
18:25:10 VERBOSE New node: ae8d:fee1:e02c:e22d. Creating new OSCORE context in /builds/tmp/RIOT/bin/oscore_context_ae8dfee1e02ce22d.json.
18:25:10 VERBOSE received JRC join request
18:25:10 VERBOSE 67eb [PACKETFUNCTIONS] copy packet content to big packet (pkt len 1300 > max len 125)
18:25:10 VERBOSE 67eb [PACKETFUNCTIONS] copy packet content to small packet (pkt len 33 < max len 125)
18:25:10 SUCCESS 67eb [C6T] node joined
18:25:22 INFO 67eb [SIXTOP] sending a 6top request
18:25:22 INFO 5de6 [SIXTOP] sending a 6top request
18:25:23 SUCCESS 67eb [SIXTOP] sixtop return code RC_SUCCESS at sixtop state WAIT_ADDRESPONSE
18:25:24 SUCCESS 5de6 [SIXTOP] sixtop return code RC_SUCCESS at sixtop state WAIT_ADDRESPONSE
18:25:26 INFO received RPL DAO from bbbb:0:0:0:ae8d:fee1:60c7:cb8a
        - parents:
           bbbb:0:0:0:ae8d:fee1:208c:870d
        - children:
18:25:35 INFO received RPL DAO from bbbb:0:0:0:ae8d:fee1:60c7:cb8a
        - parents:
           bbbb:0:0:0:ae8d:fee1:208c:870d
        - children:
18:25:46 INFO received RPL DAO from bbbb:0:0:0:ae8d:fee1:60c7:cb8a
        - parents:
           bbbb:0:0:0:ae8d:fee1:208c:870d
        - children:

leandrolanzieri · 2021-05-05T06:33:51Z

I have tested the following, which work as expected:

semtech-loramac (using test app)
gnrc_lorawan (using example)
ieee802154_security (by adding it to gnrc_networking, as suggested Initial implementation of IEEE 802.15.4 security #15150)

leandrolanzieri

Changes look good, the feature works as expected while keeping the default behaviour and code-size because of the optimizations in place. Tests have been greatly improved.

ACK! Thanks @Ollrogge for the contribution!

leandrolanzieri · 2021-05-05T07:19:05Z

And GO!

benpicco · 2023-02-20T17:13:33Z

-};
-
-typedef struct aes_key_st AES_KEY;
+} AES_KEY;


Why not?

Suggested change

} AES_KEY;

} aes_key_t;

Ollrogge requested review from MichelRottleuthner, aabadie, fjmolinas, leandrolanzieri and smlng as code owners March 11, 2021 10:54

leandrolanzieri requested a review from PeterKietzmann March 11, 2021 10:56

leandrolanzieri added Area: crypto Area: Cryptographic libraries Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 11, 2021

PeterKietzmann requested changes Mar 16, 2021

View reviewed changes

Ollrogge force-pushed the aes_pr branch from 3ea4dc8 to c40577a Compare March 25, 2021 20:21

PeterKietzmann mentioned this pull request Mar 31, 2021

crypto/ciphers: remove unneeded max_key_size in cipher_interface_st #16253

Merged

Ollrogge force-pushed the aes_pr branch from c40577a to 527c36e Compare April 6, 2021 12:10

leandrolanzieri reviewed Apr 6, 2021

View reviewed changes

Comment thread sys/include/crypto/aes.h Outdated

Comment thread sys/include/crypto/ciphers.h Outdated

Comment thread makefiles/pseudomodules.inc.mk

Ollrogge force-pushed the aes_pr branch from 88648f4 to 034f4cb Compare April 23, 2021 08:15

Ollrogge force-pushed the aes_pr branch from 034f4cb to 9259741 Compare April 27, 2021 09:30

leandrolanzieri reviewed Apr 27, 2021

View reviewed changes

Comment thread sys/Makefile.dep Outdated

Ollrogge force-pushed the aes_pr branch 4 times, most recently from 953fb69 to 245fd87 Compare April 27, 2021 16:42

Ollrogge requested a review from haukepetersen as a code owner April 27, 2021 16:42

leandrolanzieri reviewed Apr 28, 2021

View reviewed changes

Comment thread sys/crypto/aes.c

Ollrogge force-pushed the aes_pr branch from 245fd87 to 50bb600 Compare April 28, 2021 22:10

leandrolanzieri reviewed May 3, 2021

View reviewed changes

Comment thread sys/Makefile.dep Outdated

Ollrogge added 5 commits May 3, 2021 17:34

sys/crypto: Enable support for AES-192, AES-256

427d2bd

sys/crypto: Remove leftover code related to deprecated 3DES block cipher

b1a2d47

sys/crypto: Add AES-CCM NIST SP 800-38C testvectors

3f52990

sys/crypto, sys/random: Fix Kconfig files in response to AES changes

f5ca90b

tests/sys/crypto: Split test application to reduce size

46a0ef0

Ollrogge force-pushed the aes_pr branch from 50bb600 to 46a0ef0 Compare May 3, 2021 15:41

leandrolanzieri added this to the Release 2021.07 milestone May 5, 2021

leandrolanzieri approved these changes May 5, 2021

View reviewed changes

leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 5, 2021

leandrolanzieri merged commit d36628d into RIOT-OS:master May 5, 2021

benpicco reviewed Feb 20, 2023

View reviewed changes

		return cipher->interface->init(&cipher->context, key, key_size);
		}

Uh oh!

Conversation

Ollrogge commented Mar 11, 2021 • edited by leandrolanzieri Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

PeterKietzmann commented Mar 11, 2021

Uh oh!

PeterKietzmann commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Mar 11, 2021

Uh oh!

Ollrogge commented Mar 11, 2021

Uh oh!

PeterKietzmann commented Mar 12, 2021

Uh oh!

PeterKietzmann commented Mar 12, 2021

Uh oh!

Ollrogge commented Mar 14, 2021

Uh oh!

Ollrogge commented Mar 14, 2021

Uh oh!

PeterKietzmann commented Mar 15, 2021

Uh oh!

Ollrogge commented Mar 16, 2021

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Mar 22, 2021

Uh oh!

Ollrogge commented Mar 22, 2021

Uh oh!

PeterKietzmann commented Mar 23, 2021

Uh oh!

leandrolanzieri commented Mar 25, 2021

Uh oh!

Ollrogge commented Mar 25, 2021

Uh oh!

PeterKietzmann commented Apr 6, 2021

Uh oh!

Ollrogge commented Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri commented Apr 26, 2021

Uh oh!

Ollrogge commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PeterKietzmann commented May 4, 2021

Ollrogge commented Mar 11, 2021 •

edited by leandrolanzieri

Loading

PeterKietzmann commented Mar 11, 2021 •

edited

Loading