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

Support for micro-ecc (WIP) #228

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Support for micro-ecc (WIP) #228

wants to merge 17 commits into from

Conversation

obgm
Copy link
Contributor

@obgm obgm commented Feb 18, 2024

This is a proof of concept for supporting micro-ecc as a replacement for the ECC implementation in tinydtls.
micro-ecc is included as a submodule, therefore you either need to specify --recurse-submodules with git clone or do a git submodule update --init --recursive in an existing working copy.

For backwards compatibility, only curve secp256r1 is supported.

This change is based on PR #227 to avoid warnings during the build process.

boaks and others added 17 commits December 22, 2023 11:12
The previous version ignores the option for the local port. That may be
caused by issues using the same default local port for the server and
client.
This enables the use of an specific local port and changes the default
to an ephemeral free port, similar to quite a lot of other UDP clients.
The DEFAULT_PORT is therefore only used for the destination.

Signed-off-by: Achim Kraus <[email protected]>
Simple client side implementation indicates support and uses the cid of
the server, when negotiated by that.

Signed-off-by: Achim Kraus <[email protected]>
Do not calculate the cookie using the Extensions as these are
different between DTLS1.2 and DTLS1.3

https://datatracker.ietf.org/doc/html/rfc6347#section-4.2.1

When responding to a HelloVerifyRequest, the client MUST use the same
parameter values (version, random, session_id, cipher_suites,
compression_method) as it did in the original ClientHello.  The
server SHOULD use those values to generate its cookie and verify that
they are correct upon cookie receipt.

https://www.rfc-editor.org/rfc/rfc9147.html#section-5.3

The ClientHello up to, but not including the Extensions is the same for
DTLS1.2 and DTLS1.3

Signed-off-by: Jon Shallow <[email protected]>
To avoid doxygen warnings regarding outdated configuration options,
this change updates the Doxyfile template to the current version 1.9.8.

Change-Id: Ia60a20e4cb3da375cde48c9ae3ca234200bb8a10
The SUB_OBJS dependency rule invoked make -C on the target directory
without passing important flags such as CPPFLAGS. This change removes
this rule so that these objects are built with the all flags set in
this Makefile.

Change-Id: Ic673a881f18cbfa773860d722041a7a083f65d8e
Sets OPT_OBJS and CPPFLAGS to build uECC with curve secp256r1

Change-Id: I3c4860fbc568a492082eeb08e4ee3abd6fbc3c6e
When compiled with support for ECC, the pseudo-random number generator
must be set for micro-ecc. As the function signature required by
micro-ecc is different from dtls_prng(), a wrapper function is
required to map the different size types. As dtls_prng's size type is
larger, no other conversion is necessary.

Change-Id: I2f0da18983256be3bc27f18079cf9a774049de33
As micro-ecc comes with its own unit tests, the tests for the internal
ECC implementation are removed.

Change-Id: I36826df4a99bd916659898587a94ea6d76af4a33
Change-Id: I85de2c3c88063dae35e81b2b3999bf7d76d04ce7
Add submodule ext/micro-ecc from https://github.com/kmackay/micro-ecc.git

Change-Id: I17efa4c2fd9bd952b479bce32f0ebbb764301ca3
Change-Id: I5bd1f510c162c5ef4432f5c9cb93836312706a74
This change provides support for the curve secp256r1 from micro-ecc.

Change-Id: I2d272e2ddb498016a2d6e85af7af8247010768d8
assert(sizeof(sign) >= 2 * uECC_BYTES);
curve_size = uECC_curve_public_key_size(uecc_curve);

assert(key_size == (unsigned int)curve_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

That fires on my interop test. Adding

dtls_warn("%d != %d\n", (int) key_size, curve_size);

I get

Feb 26 16:46:45 WARN 32 != 64
tinydtls-server: crypto.c:638: dtls_ecdsa_verify_sig_hash2: Assertion `key_size == (unsigned int)curve_size' failed.

AFAIK, the key-size is passed in as sizeof(config->keyx.ecdsa.other_pub_x) in dtls.c, line 2589

Copy link
Contributor

Choose a reason for hiding this comment

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

Beside the assert, the first interop-tests with Californium are successful.
Great!

uint8_t pub_key[2 * DTLS_EC_KEY_SIZE];
memcpy(pub_key, pub_key_x, key_size);
memcpy(pub_key + key_size, pub_key_y, key_size);
return dtls_ecdsa_verify_sig_hash2(pub_key, key_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, the size of pub_key isn't that of the passed in key_size.


#include "global.h"
#include "dtls_debug.h"
#include "numeric.h"
#include "dtls.h"
#include "crypto.h"
#include "ccm.h"
#include "ecc/micro-ecc/uECC.h"
#ifdef DTLS_ECC
#include "ext/micro-ecc/uECC.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

"ext/micro-ecc" is part of the include path, maybe it's simpler to use "uECC.h"?

Copy link
Contributor

@mrdeep1 mrdeep1 Feb 26, 2024

Choose a reason for hiding this comment

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

It is my preference to always include the package information path in the #include statement, to prevent name clashes (unlikely with uECC.h, but with possible with names like global.h etc.).

So, I would recommend that the include path includes ext/, and use #include <micro-ecc/uECC.h>.

Copy link
Contributor

Choose a reason for hiding this comment

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

The pain is, that "micro-ecc/uECC.c" uses:

#include "uECC.h"
#include "uECC_vli.h"

and "micro-ecc/uECC_vli.h" uses:

#include "uECC.h"
#include "types.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

Anything that is getting built external to micro-ecc will need the equivalent of "micro-ecc/uECC.h" to get at the function definitions. For the actual micro-ecc code compilation, I assume that it is done in the micro-ecc/ directory, in which case just "uECCH.h" without a path is fine.

If you declare #include "" , the compiler first searches in current directory of source file and if not found,
continues to search in any included -I directories and then the default include directories.

If you declare #include <> , the compiler searches directly in any included -I directories and then the default
include directories.

$ cpp -v shows the include search directory order.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I've checked it and at least the cmake build doesn't need to extend the include path with "ext/micro-ecc".

Copy link
Contributor

Choose a reason for hiding this comment

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

"ext/micro-ecc" is part of the include path,

I mixed it up, it's only part of "FILES".

@boaks
Copy link
Contributor

boaks commented Feb 26, 2024

See PR #229 for the fix of the "assert key_size", and support for cmake and, github workflow.

@@ -337,6 +349,9 @@ dtls_init(void) {
memarray_init(&dtlscontext_storage, dtlscontext_storage_data,
sizeof(dtls_context_t), DTLS_CONTEXT_MAX);
#endif /* RIOT_VERSION */
#ifdef DTLS_ECC
uECC_set_rng(uecc_rng_function);
#endif /* DTLS_ECC */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that could be moved to "crypto.c". In my opinion, that would limit the scope of "ecc-related changes" to crypto.

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.

3 participants