sys/crypto: add ccms by fjmolinas · Pull Request #13149 · RIOT-OS/RIOT

fjmolinas · 2020-01-17T08:25:45Z

Contribution description

This PR extends CCM to add CCM* (CCMS) . It differs from CCM in that it can be used for messages that require only encryption.

The test cases have been taken from [1] as well.

[1] https://mentor.ieee.org/802.15/dcn/04/15-04-0537-02-004b-formal-specification-ccm-star-mode-operation.pdf

Testing procedure

run test
make -C tests/sys_crypto/ all test
take a look at [1] to check the implementation (or better said adaptation of the already existing CCM)

fjmolinas · 2020-02-05T10:30:05Z

Build failed only on insufficient memory and doxygen mistake:

running './dist/tools/headerguards/check.sh' x
Command output:

	--- sys/include/crypto/modes/ccm_common.h
	+++ sys/include/crypto/modes/ccm_common.h
	@@ -46,5 +46,5 @@
	 }
	 #endif
	 
	-#endif /* CRYPTO_MODES_CCM_H */
	+#endif /* CRYPTO_MODES_CCM_COMMON_H */
	 /** @} */

I'll add the boards after review:

atmega1284p
derfmega128
mega-xplained
microduino-corerf
msb-430
msb-430h

mtausig

TBH, I don't quite see the point of this cipher mode.
The code looks find,though, apart from minor things.

sys/crypto/modes/ccm.c

mtausig · 2020-02-08T20:52:35Z

tests/sys_crypto/tests-crypto-modes-ccms.c

+    /* MAC */
+    0x4F, 0xDE, 0x52, 0x90, 0x61, 0xF9, 0xC6, 0xF1
+};
+static const size_t TEST_IEEE_0537R2_3_EXPECTED_LEN = 38;


Could you maybe add some more test vectors with longer data. Since the CCM implementation had problems in the past in that area, it would be helpful.

Does it make sense to add those? They both share the same base code, and those extended length cases are already tested in tests-crypto-modes-ccms.c, I could still generate some if you feel its needed.

Why not just add one long sequence... Doesn't the above referenced document provide one?

Nope, I added all the vectors provided in the document. I added in the last fixup some longer test vectors generated with PyCryptodome.

Fix naming

Fix and and comment on valid mac_length

More code comments

Returning 0 on an empty message is actually also a fix for ccm, so should be its own commit.

PeterKietzmann

Code looks good to me, just left some minor comments for improvement. Provided test passes.

sys/crypto/modes/ccm.c

PeterKietzmann · 2020-04-08T08:31:49Z

sys/crypto/modes/ccm.c

+                        const uint8_t *input, size_t input_len,
+                        uint8_t *plain)
+{
+    if(_validate_mac_length_ccms(mac_length)){


Just to double check, this is the only difference to cipher_decrypt_ccm above?

Yes and no. CCM* specifies as well that:

The nonce N shall encode the potential values for M such that one can uniquely determine from N the actually used value of M. The exact format of the nonce N is outside the scope of this specification and shall be determined and fixed by the actual implementation environment of the CCM* mode.

This part I did not address, this PR intended only to allow messages requiring only encryption. How to generate that nonce to encode M, and how to decode M from the nonce, was not required for me currently. I would address it latter if needed. But I realize this is not clear in the documentation I will add a comment.

Did you check how this is implemented elsewhere? OpenWSN?

tests/sys_crypto/tests-crypto-modes-ccm.c

PeterKietzmann · 2020-04-08T08:41:55Z

tests/sys_crypto/tests-crypto-modes-ccms.c

+}
+
+#define do_test_decrypt_op(name) do { \
+        test_decrypt_op(TEST_ ## name ## _KEY, TEST_ ## name ## _KEY_LEN, \


I'm not familiar with the new tests/sys_crypto structure. Is it possible to share this between tests-crypto-modes-ccm.c and tests-crypto-modes-ccms.c ?

The macros could be in a shared header file yes ~~, and I could move the shared test_decrypt_op()/test_encrypt_op into a helper source file as well.~~

Ah, this hasn't been addressed yet.

tests/sys_crypto/tests-crypto-modes-ccms.c

PeterKietzmann · 2020-04-08T09:11:12Z

TBH, I don't quite see the point of this cipher mode.

@mtausig the OpenWSN re-integration in #13824 requires it. This mode is involved in link layer security in TSCH networks. Do you have objections?

mtausig · 2020-04-08T10:42:48Z

TBH, I don't quite see the point of this cipher mode.

@mtausig the OpenWSN re-integration in #13824 requires it. This mode is involved in link layer security in TSCH networks. Do you have objections?

No, if there is a valid reason for it I'm OK with it. Maybe add a disclaimer in the documentation to prevent it from beeing used for generic encryption.

Add long plain text and aad tests

…test Fix ccm to ccms

_valid_mac_length_% return 1 if valid, 0 otherwise

Skip MAC computation when M=0

… test change doc to make explicit that plaintext=0 is allowed

Explicit plaintext length in header

PeterKietzmann

@fjmolinas thanks for addressing my comments! The code looks good, an adequate amount of documentation is in place and tests still pass. ACK from my side. Please squash

Add implemenation disclaimer.

fjmolinas · 2020-04-09T11:06:42Z

No, if there is a valid reason for it I'm OK with it. Maybe add a disclaimer in the documentation to prevent it from beeing used for generic encryption.

Added a note in the documentation

Just to double check, this is the only difference to cipher_decrypt_ccm above?
Yes and no. CCM* specifies as well that:

The nonce N shall encode the potential values for M such that one can uniquely determine from N the actually used value of M. The exact format of the nonce N is outside the scope of this specification and shall be determined and fixed by the actual implementation environment of the CCM* mode.

This part I did not address, this PR intended only to allow messages requiring only encryption. How to generate that nonce to encode M, and how to decode M from the nonce, was not required for me currently. I would address it latter if needed. But I realize this is not clear in the documentation I will add a comment.

Add more context on this in the documentation. Note that If at some point the nonce is encoded, this would depend on the application. Since this was out of scope of the specification they just use the source address, frame counter and security level, OpenWSN does something similar using source address and the ASN (absolute slot number).

fjmolinas · 2020-04-09T11:11:23Z

Add more context on this in the documentation. Note that If at some point the nonce is encoded, this would depend on the application. Since this was out of scope of the specification they just use the source address, frame counter and security level, OpenWSN does something similar using source address and the ASN (absolute slot number).

Contiki's implementation also just receives the nonce. But let me take a second look elsewhere.

fjmolinas · 2020-04-09T13:55:19Z

The CCM* mode extends the definition of the original CCM mode, such as to provide for confidentiality-only services, in addition to the other security service options already offered. Moreover, the CCM* mode adapts the original CCM mode such that the resulting mode can be used securely with variable-length authentication tags, rather than fixed-length authentication tags only. Thus, the CCM* mode takes away the disadvantages of the CCM mode pointed out above. For the detailed specification of the CCM* mode of operation, see Section 2.

I looked again into ccm, ccms documentation and implementations. I now think I agree with @mtausig, there is no interest in adding CCMS as this PR is constructed. Using CCMS for encryption only is not recommended security wise, so its not to be expected that upper layers would have a use for it. Only the nonce part could be of interest, but its not part of the PR. OpenWSN on its side although it uses an implementation of ccms similar to this PR (so it allows encryption only), it never uses encryption only mode, for the same security issues.

Also in the case of OpenWSN oscore or iee802154e specifies how the nonce should be computed, so there is no use case for the second feature that a CCM extension would provide. Since my main use case was the OpenWSN PR, and since it is no longer needed, I'll would like to close this PR, and split the change that adds the test and handling of input_len=0 3b4aca6.

If I come around implementing a scheme for nonce encoding, I would re-open.

fjmolinas · 2020-04-10T09:08:55Z

Closed.

fjmolinas added 2 commits January 17, 2020 09:18

sys/crypto: add ccms

4cd624b

tests/sys_crypto: add ccms test

0032f6b

fjmolinas added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: crypto Area: Cryptographic libraries labels Jan 17, 2020

fjmolinas requested a review from mtausig February 5, 2020 10:27

mtausig suggested changes Feb 8, 2020

View reviewed changes

fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Mar 27, 2020

MERGECOMMIT: remove once RIOT-OS#13149 is in

777c020

fjmolinas added 2 commits March 31, 2020 10:13

fixup! tests/sys_crypto: add ccms test

acd0a7f

Fix naming

fixup! fixup! tests/sys_crypto: add ccms test

ec8fccd

Fix and and comment on valid mac_length

fjmolinas requested review from MichelRottleuthner, aabadie, leandrolanzieri and smlng as code owners March 31, 2020 08:25

fjmolinas added 3 commits March 31, 2020 10:35

fixup! fixup! fixup! tests/sys_crypto: add ccms test

e8e53a4

fixup! fixup! fixup! fixup! tests/sys_crypto: add ccms test

1aad0fb

More code comments

squash! fixup! fixup! fixup! fixup! tests/sys_crypto: add ccms test

3b4aca6

Returning 0 on an empty message is actually also a fix for ccm, so should be its own commit.

fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Apr 3, 2020

MERGECOMMIT: remove once RIOT-OS#13149 is in

3efd16f

fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Apr 6, 2020

MERGECOMMIT: remove once RIOT-OS#13149 is in

b767502

fjmolinas mentioned this pull request Apr 6, 2020

pkg/openwsn: re-integrate the network stack as a package #13824

Merged

11 tasks

PeterKietzmann reviewed Apr 8, 2020

View reviewed changes

fjmolinas added 4 commits April 9, 2020 10:53

fixup! fixup! fixup! fixup! fixup! tests/sys_crypto: add ccms test

fcf6095

Add long plain text and aad tests

fixup! fixup! fixup! fixup! fixup! fixup! tests/sys_crypto: add ccms …

258cf23

…test Fix ccm to ccms

fixup! fixup! fixup! tests/sys_crypto: add ccms test

b11172b

_valid_mac_length_% return 1 if valid, 0 otherwise

fixup! sys/crypto: add ccms

1f2698c

Skip MAC computation when M=0

fjmolinas added 2 commits April 9, 2020 12:10

fixup! squash! fixup! fixup! fixup! fixup! tests/sys_crypto: add ccms…

00c6a82

… test change doc to make explicit that plaintext=0 is allowed

fixup! fixup! sys/crypto: add ccms

3b2665e

Explicit plaintext length in header

PeterKietzmann approved these changes Apr 9, 2020

View reviewed changes

leandrolanzieri added this to the Release 2020.04 milestone Apr 9, 2020

fixup! fixup! fixup! sys/crypto: add ccms

7978ef5

Add implemenation disclaimer.

leandrolanzieri removed this from the Release 2020.04 milestone Apr 9, 2020

fjmolinas mentioned this pull request Apr 9, 2020

sys/crypto/modes/ccm: handle input_len = 0 #13848

Merged

fjmolinas closed this Apr 10, 2020

leandrolanzieri mentioned this pull request Apr 20, 2020

sys/crypto/modes/ccm: handle input_len = 0 [backport 2020.04] #13904

Merged

Conversation

fjmolinas commented Jan 17, 2020 • edited by PeterKietzmann Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

fjmolinas commented Feb 5, 2020

Uh oh!

mtausig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PeterKietzmann commented Apr 8, 2020

Uh oh!

mtausig commented Apr 8, 2020

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Apr 9, 2020

Uh oh!

fjmolinas commented Apr 9, 2020

Uh oh!

fjmolinas commented Apr 9, 2020

Uh oh!

fjmolinas commented Apr 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fjmolinas commented Jan 17, 2020 •

edited by PeterKietzmann

Loading

fjmolinas Apr 9, 2020 •

edited

Loading