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-CCM from tinydtls #91

Closed
wants to merge 13 commits into from

Conversation

chrysn
Copy link
Contributor

@chrysn chrysn commented Jan 14, 2020

This attempts to address #89 by adding tinydtls as a supported backend. Only its AES-CCM (which is AES-CCM-16-64-128 and as far as I can tell not configurable) is included right now; other primitives are not considered for this PR.

The main friction point is that tinyDTLS is not designed as a crypto primitive provider, and thus when configured without PSK and without ECC support, it raises build errors like empty unions and such -- which in a practical setup with RIOT-OS will not be too much of an issue, because a) tinyDTLS will be configured properly by its RIOT pkg, and b) the backend is likely to be used in scenarios when there is already another reason to use DTLS (although that's not the case for me).

tinyDTLS' AES-CCM can not do any form of AAD streaming (see discussion in #89) -- while that's a setback, it's not like the other backends are particularly helpful there. We could probably do the same tricks (grabbing deep into the crypto code or asking the library author to make that public) on tinydtls as with any other library (in tinydtls, that'd be ccm.c's static void add_auth_data that could be split up and 16-byte-chunk-wise fed with data).


The PR is marked as WIP as it is not complete yet. The code present is sufficient to have an external build system use AES-CCM to encrypt and decrypt messages, but was not tested against another implementation yet, and not even for RIOT are there build system integrations, let alone a Makefile.tinydtls.

Given that AES-CCM comes in various shapes and sizes, the function may
still need renaming (not sure whether it makes sense to have it
parametrized like AES-GCM), but the constants should be good.
@chrysn
Copy link
Contributor Author

chrysn commented Jan 14, 2020

The topic of parameterization of CCM is probably best addressed at a later time when a version of tinyDTLS that includes eclipse/tinydtls@cbe1810 has been released.

... fixing failure to build with strict RIOT configurations.

Checking for <0 before casting to unsigned ensures that all cases are
covered; the compiler is invited to do any further optimization.
chrysn added a commit to coap-security/liboscore that referenced this pull request Jan 14, 2020
This incurs switching to a vendor branch until [91] is resolved, and
additional text in the Makefiles that might still move in with libcose.

[91]: bergzand/libcose#91
In scenarios where no signature algorithms are present (like when using
only a backend that can do AEAD), this prevents "unused parameter"
compiler errors.
@chrysn
Copy link
Contributor Author

chrysn commented Jan 14, 2020

Tests against other implementations were successful, and the code Just Works when advertised in the RIOT pkg config, and

diff --git a/pkg/libcose/Makefile.dep b/pkg/libcose/Makefile.dep
index f62a5eb7f..aa2732ad0 100644
--- a/pkg/libcose/Makefile.dep
+++ b/pkg/libcose/Makefile.dep
@@ -13,3 +13,6 @@ endif
 ifneq (,$(filter libcose_crypt_c25519,$(USEMODULE)))
   USEPKG += c25519
 endif
+ifneq (,$(filter libcose_crypt_tinydtls,$(USEMODULE)))
+  USEPKG += tinydtls
+endif
diff --git a/pkg/libcose/Makefile.include b/pkg/libcose/Makefile.include
index d2b027bfe..83ac8a322 100644
--- a/pkg/libcose/Makefile.include
+++ b/pkg/libcose/Makefile.include
@@ -10,6 +10,9 @@ endif
 ifneq (,$(filter libcose_crypt_c25519,$(USEMODULE)))
   CFLAGS += -DCRYPTO_C25519
 endif
+ifneq (,$(filter libcose_crypt_tinydtls,$(USEMODULE)))
+  CFLAGS += -DCRYPTO_TINYDTLS
+endif

or similar is applied to RIOT (although a workaround for RIOT-OS/RIOT#13121 is currently needed as well).

Following the precendent in suit.c that already uses that condition.
Along with its Makefile, tinydtls also needs a minimal configuration
header as it might be output by a configure script (but its complexity
is low enough that a static file does just as well for our purposes, and
is much less of a hassle).

For AES-CCM, a single encrypt-decrypt test is run, based on the Packet
Vector 1 of <https://www.rfc-editor.org/rfc/rfc3610.html#section-8>.
Since the silkeh/clang images use Debian buster now, libgcc-6-dev is not
available any more; replacing with the latest that is.
Clang is a bit stricter when it comes to static arrays with the size of
a static const, replacing with defined expressions.

The removed allowed compiler warnings were only relevant to the older
released versions of tinydtls ("master") branch, which was never used in
a committed version of this port -- but the new ones are required for
Clang and do not disturb GCC operation.
@bergzand
Copy link
Owner

The Monocypher build failures look unrelated to me 😞 I have to fix those in a separate PR

@chrysn
Copy link
Contributor Author

chrysn commented Jan 14, 2020

CI is now largely fixed, with an unrelated update to the installed libraries (f9da347) and a set of CFLAGS that make almost all compilers happy: GCC-7 is complaining about the exceptions I have to make for the newer ones, and generally insists on ISO rules the newer versions ignore. (I'll play around a bit more, but would consider not being that strict with older GCCs -- if a newer accepts code w/o warnings and the older compiles it, it should be fine.)

The remaining errors are monocypher-related (didn't track down) and some image-related issue with clang-tidy (which I think I'll still need to insert some NOLINT(readability-non-const-parameter) into but I'm trying to do better).

@chrysn
Copy link
Contributor Author

chrysn commented Jan 14, 2020

I'd rather not do anything about the readability-non-const-parameter lints I get on cose_crypto_sign: while for sign and signlen it does write in the backends, msg should not be written to, so fixing the lint would suppress the sign of a possibly missing const.

@chrysn chrysn changed the title [WIP] Add AES-CCM from tinydtls Add AES-CCM from tinydtls Jan 15, 2020
@chrysn
Copy link
Contributor Author

chrysn commented Jan 15, 2020

I've removed the WIP marker as it's now testable and ready for review as far as I am concerned.

There's a fixup commit in there and some others I'd probably squash (all the "tool X complained" into their respective original commits), as soon as this has passed the review stage.

@chrysn
Copy link
Contributor Author

chrysn commented Feb 20, 2020

Ping: Is there anything I can do here to add getting this through? I'd like to give #90 another try, and that'd be easier if I could build on this without juggling branches.

Copy link
Owner

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

I'm fully aware that I don't have a style guide, but would you mind changing the C++ style comments to C-style comments? Code looks good otherwise. I'll try to test this tomorrow and have this merged soon

makefiles/tinydtls_config/dtls_config.h Outdated Show resolved Hide resolved
makefiles/tinydtls_config/dtls_config.h Outdated Show resolved Hide resolved
makefiles/tinydtls_config/dtls_config.h Outdated Show resolved Hide resolved
src/crypt/tinydtls.c Outdated Show resolved Hide resolved
src/crypt/tinydtls.c Outdated Show resolved Hide resolved
tests/crypto.c Outdated Show resolved Hide resolved
tests/crypto.c Outdated Show resolved Hide resolved
@benpicco
Copy link

Ping 😉

@chrysn
Copy link
Contributor Author

chrysn commented Aug 17, 2020

AFAIR this is waiting for a "Concerns addressed, please rebase" or similar response.

The compatibility API's behavior changed in
eclipse/tinydtls#34; this picks the parameters
that work for AES-CCM-16-64-128.
@chrysn
Copy link
Contributor Author

chrysn commented Nov 8, 2020

After erroneously opening as an issue #92: The behavior of tinydtls changed in eclipse/tinydtls#34 in which AES-CCM version its compatibility API provides. The latest change uses the new 0.9 API instead that allows passing in the M and L parameters.

While this makes it easy to add different AES-CCM version, I don't have need or test vectors for them; they can be added at a later point, though.

@chrysn chrysn requested a review from bergzand November 8, 2020 04:43
@chrysn
Copy link
Contributor Author

chrysn commented Nov 8, 2020

I can't quite tell what's going wrong with CI again, but looks like it's more a generic thing.

On the topic of 0.9 API: It seems tinydtls didn't have releases so far (nothing is tagged), but there's a v0.9-RC1 tag here, and things work with the current develop branch, that tag, and the commit currently packaged for RIOT. For testing I'd stick with the develop branch (that's what's set in the CI), but testing through RIOT will probably stabilize on the releases once they're really out.

@fjmolinas
Copy link
Collaborator

@bergzand you mentioned the ci was updated, maybe this PR needs the ci to be re-trigguered?

@chrysn
Copy link
Contributor Author

chrysn commented May 12, 2021

I'm a bit conflicted on how to continue with this, now that tinycrypt already provides AES-CCM support.

Is it generally desirable to have diverse backends for algorithms (if only to easily benchmark them against each other)?

If not, I'd need to reevaluate whether there is another embedded-friendly backend so that #94 can continue and #95 could be replaced by something that does not involve dragging in another backend.

If it is desirable, just let me know and I'll rebase this. (The conflicts probably are just "we've created the same function with different switch branches that need merging", and then we'll need a priority decision on which backend to run AES-CCM through if both are built).

@fjmolinas
Copy link
Collaborator

I'm a bit conflicted on how to continue with this, now that tinycrypt already provides AES-CCM support.

Is it generally desirable to have diverse backends for algorithms (if only to easily benchmark them against each other)?

If not, I'd need to reevaluate whether there is another embedded-friendly backend so that #94 can continue and #95 could be replaced by something that does not involve dragging in another backend.

If it is desirable, just let me know and I'll rebase this. (The conflicts probably are just "we've created the same function with different switch branches that need merging", and then we'll need a priority decision on which backend to run AES-CCM through if both are built).

@chrysn from an Oscore side what is still missing in libcose? From what I can see myself, libcose is currently assuming that the nonce itself is sent as the unprotected field, while in OSCORE the partial_iv is sent, so some API changes are needed there. Maybe simply adding a nonce, nonce_len field to the cose_encrypt_decrypt function (that could be NULL and 0)? Thoughts @bergzand?

@chrysn
Copy link
Contributor Author

chrysn commented Dec 31, 2022

I think that with the additions that are now in already (AES-CCM through tinycrypt in #103, HKDF from #94), and given how much headache tinyDTLS has caused me (I don't have concrete points, only vague memories of their master branch being old and develop moving erratically, plus lots of workarounds in libCOSE), it's best to close this one unfinished. The cryptographic operations I need are in, and I see no need to add an extra backend just because.

@chrysn chrysn closed this Dec 31, 2022
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.

4 participants