Add CCM cipher suite variants by boaks · Pull Request #105 · eclipse-tinydtls/tinydtls

boaks · 2021-10-18T20:57:41Z

In case of CCM gets recommended and CCM8 deprecated.

obgm · 2021-12-16T08:35:43Z

 #define A_DATA_LEN 13
    unsigned char nonce[DTLS_CCM_BLOCKSIZE];
    unsigned char A_DATA[A_DATA_LEN];
+    int macLen = get_cipher_suite_mac_len(security->cipher);


With previous suggested change option 2, this type now should probably be uint8_t.

And, as this is C code, please follow the usual conventions for variable naming, i.e., mac_len.

obgm · 2021-12-16T08:41:56Z

 #define A_DATA_LEN 13
    unsigned char nonce[DTLS_CCM_BLOCKSIZE];
    unsigned char A_DATA[A_DATA_LEN];
+    int macLen = get_cipher_suite_mac_len(security->cipher);


See previous comment.

obgm · 2021-12-16T08:44:08Z

-    const dtls_ccm_params_t params = { nonce, 8, 3 };
+    const dtls_ccm_params_t params = { nonce, macLen, 3 };

    if (clen < 16)		/* need at least IV and MAC */


I think that now, we might need to have to check for something like clen < (mac_len + MAX_IV_LENGTH), with MAX_IV_LENGTH currently being 8.
Otherwise, the newly added clen - macLen below might yield an underflow.

done.
(there is no symbolic IV-length, right?)

Correct—for some unknown reason, that has not been defined when crafting this check.

I would propose to go without for this PR.
And have a second PR introducing RECORD_IV_LENGTH.

WDYT?

Sure, a separate PR would be the cleanest solution.

boaks · 2021-12-16T20:25:17Z

The first part of the changes. I will provided the rest at the weekend.

boaks · 2021-12-19T08:12:07Z

I added a cipher_suite_param_t in a second commit. Is extended the original proposal by psk and ecdhe_ecdsa and used it for the other functions.

obgm · 2022-05-25T19:04:26Z

 };

+typedef enum {
+  NONE,


I wonder if we should prefix these with, e.g. DTLS_CIPHER_ or similar. Especially NONE might be prone to name clashes.

What's about KEY_EXCHANGE_...?

obgm · 2022-05-25T20:53:58Z

There is a lot of whitespace change which makes reading this PR a bit difficult.
Anyway, what I liked about the previous approach was that a lot of the conditionals were resolved at compile time. I do not know how good the optimizer is in unrolling the check loop.

boaks · 2022-05-26T06:12:28Z

There is a lot of whitespace change which makes reading this PR a bit difficult.

I'm used to fix the format of functions when I change them. Yes, that makes it hard to compare. One idea would then be, to spend some time in fix the format at once, and the rebase this PR. Though many files shows "Eclipse Public License v1.0" which is replaced by v2.0, such a cleanup seems to be required anyway.

Should I go for format and license fix next week?

what I liked about the previous approach was that a lot of the conditionals were resolved at compile time

static inline int is_tls_ecdhe_ecdsa_with_aes_128_ccm_8(dtls_cipher_t cipher)
{
#ifdef DTLS_ECC
  return cipher == TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8;
#else
  (void) cipher;
  return 0;
#endif /* DTLS_ECC */
}

The point seems to be, that using a reduced variety, makes it also possible to use very simple implementations.
Maybe, we add the key_exchange_type to the handshake data and process it only once?

boaks · 2022-05-27T20:25:38Z

Maybe, we add the key_exchange_type to the handshake data and process it only once?

After analyzing it again, maybe exchange the dtls_handshake_parameters_t.cipher with dtls_handshake_parameters_t.cipher_suite_params_index as index into cipher_suite_params makes it simple again.

static inline int is_tls_ecdhe_ecdsa_with_aes_128_ccm_8(int cipher_suite_params_index)
{
  return DTLS_KEY_EXCHANGE_ECDHE_ECDSA == cipher_suite_params[cipher_suite_params_index].key_exchange_algorithm;
}

Add cipher suites with full 16 byte MAC. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

Use cipher_suite_param_t for cipher-suite specific mac_len and key_exchange_algorithm. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

Simplify cipher suite parameter lookup. Cleanup old functions. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

boaks · 2022-05-30T11:21:09Z

I reverted many of the whitespace changes, I consider that is easier done separately.
I added also the "cipher_index" in order to obsolete the repetition of the loops.
I remove the is is_tls_???_with_aes_128_ccm_??? and replaced them by comparing the key exchange algorithm.
PR #113 will adapted, once it gets clear, which variant is wanted.

obgm

I like the new approach with the index into the cipher_suite_params table. I have only a few comments, primarily to fix the data types.

obgm · 2022-06-16T09:55:28Z

  dtls_compression_t compression;	/**< compression method */

-  dtls_cipher_t cipher;		/**< cipher type */
+  int cipher_index;	     /**< internal index for cipher_suite_params, DTLS_CIPHER_INDEX_NULL for TLS_NULL_WITH_NULL_NULL */


Being an index into an array, size_t would be the correct data type here.
(As the array usually should not have more than, say, 255 non-NULL elements, another unsigned type such as uint8_t might do as well. However, I do not expect cipher_index to be negative at any point in time.)

obgm · 2022-06-16T09:55:58Z


  dtls_compression_t compression;		/**< compression method */
-  dtls_cipher_t cipher;		/**< cipher type */
+  int cipher_index;		/**< internal index for cipher_suite_params, DTLS_CIPHER_INDEX_NULL for TLS_NULL_WITH_NULL_NULL */


obgm · 2022-06-16T10:05:04Z

 };

-static inline const cipher_suite_param_t* get_cipher_suite_param(dtls_cipher_t cipher)
+static const int last_cipher_suite_param = sizeof(cipher_suite_params) / sizeof(cipher_suite_param_t);


The datatype for last_cipher_suite_param must be size_t.

obgm · 2022-06-16T10:06:27Z

-static inline const cipher_suite_param_t* get_cipher_suite_param(dtls_cipher_t cipher)
+static const int last_cipher_suite_param = sizeof(cipher_suite_params) / sizeof(cipher_suite_param_t);
+
+static inline int get_cipher_index(dtls_cipher_t cipher)


The return type must be size_t.

obgm · 2022-06-16T10:08:10Z

+ * \return cipher suite.
 */
-static inline int get_cipher_suite_mac_len(dtls_cipher_t cipher)
+static inline dtls_cipher_t get_cipher_suite(int cipher_index)


cipher_index must be of type size_t.

obgm · 2022-06-16T10:12:31Z

+static inline int get_cipher_suite_mac_len(int cipher_index)
 {
-  return get_cipher_suite_param(cipher)->key_exchange_algorithm == DTLS_KEY_EXCHANGE_PSK;
+  assert(0 <= cipher_index && cipher_index < last_cipher_suite_param);


0 <= cipher_index not needed.

obgm · 2022-06-16T10:12:57Z

-known_cipher(dtls_context_t *ctx, dtls_cipher_t code, int is_client) {
-  int psk;
-  int ecdsa;
+known_cipher(dtls_context_t *ctx, int cipher_index, int is_client) {


cipher_index needs to be of type size_t

obgm · 2022-06-16T10:16:03Z

  default:
-    dtls_crit("calculate_key_block: unknown key exchange %d\n", key_exchange_algorithm);
-    return dtls_alert_fatal_create(DTLS_ALERT_INTERNAL_ERROR);
+    assert(!"calculate_key_block: not supported key exchange algorithm\n");


I think that this change warrants a PR of its own and should not be hidden somewhere deep down in a harmless looking "enhancement PR".

Let me try to explain my view:

The most critical "code points" here are the cipher-suite code-points in the Hellos. These code-points must be contained in the cipher_suite_params. The incoming/received "unknown" is therefore the cipher-suite and that fails ahead.
When this "internal/static" function "calculate_key_block" is called, only the cipher_suite_key_exchange_algorithm_t contained in that table are possible.
The "switch-default" itself (previous "unknown key exchange") is to suppress compiler warnings, it doesn't occur (unknown cipher suites are detected ahead and unknown key exchanges don't exist on their own).
Therefore I just moved it to the DTLS_KEY_EXCHANGE_NONE in order to save a couple of bytes.

Maybe a comment instead of separate commit will do it as well?

obgm · 2022-06-16T10:29:18Z

 } cipher_suite_param_t;

 static const struct cipher_suite_param_t cipher_suite_params[] = {
+  { TLS_NULL_WITH_NULL_NULL,             0, DTLS_KEY_EXCHANGE_NONE },


Please add a comment that this entry (with index DTLS_CIPHER_INDEX_NULL must always be the first array element.

boaks · 2022-06-23T15:33:03Z

I prepared PR #147 addressing the left issues here and request to merge it into main.
is_key_exchange_ecdhe_ecdsa and is_key_exchange_psk are replacing the old
and removed is_tls_ecdhe_ecdsa_with_aes_128_ccm_8 and is_tls_psk_with_aes_128_ccm_8.

boaks force-pushed the add_ccm branch 4 times, most recently from 98d5444 to 8dd726a Compare November 24, 2021 18:18

boaks force-pushed the add_ccm branch from 8dd726a to 979470e Compare December 8, 2021 15:49

obgm reviewed Dec 16, 2021

View reviewed changes

boaks force-pushed the add_ccm branch from 979470e to 7566bfe Compare December 16, 2021 20:24

boaks force-pushed the add_ccm branch 4 times, most recently from 37cd373 to d6511ef Compare December 23, 2021 19:08

boaks force-pushed the add_ccm branch from d6511ef to 22074f0 Compare February 7, 2022 18:47

boaks force-pushed the add_ccm branch from 22074f0 to 8c689a4 Compare February 25, 2022 15:38

boaks force-pushed the add_ccm branch 2 times, most recently from 6a3f1a4 to cb6d7f5 Compare May 23, 2022 11:41

obgm reviewed May 25, 2022

View reviewed changes

boaks force-pushed the add_ccm branch 2 times, most recently from 1dd7f3c to b795393 Compare May 26, 2022 07:04

boaks mentioned this pull request May 26, 2022

Issues for general cleanup? #143

Open

boaks added 2 commits May 30, 2022 08:49

dtls.c: add support for CCM cipher suites.

a290a3e

Add cipher suites with full 16 byte MAC. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

dtls.c: add cipher_suite_param_t

b8c4db1

Use cipher_suite_param_t for cipher-suite specific mac_len and key_exchange_algorithm. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

boaks force-pushed the add_ccm branch from b795393 to 14a7c0a Compare May 30, 2022 10:39

Introduce cipher_index.

456b7fb

Simplify cipher suite parameter lookup. Cleanup old functions. Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>

boaks force-pushed the add_ccm branch from 14a7c0a to 456b7fb Compare May 30, 2022 11:05

obgm reviewed Jun 16, 2022

View reviewed changes

boaks closed this Jun 23, 2022

Conversation

boaks commented Oct 18, 2021

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!

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!

boaks commented Dec 16, 2021

Uh oh!

boaks commented Dec 19, 2021

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!

obgm commented May 25, 2022

Uh oh!

boaks commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boaks commented May 27, 2022

Uh oh!

boaks commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obgm 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!

boaks Jun 23, 2022 • 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!

boaks commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

boaks commented May 26, 2022 •

edited

Loading

boaks commented May 30, 2022 •

edited

Loading

boaks Jun 23, 2022 •

edited

Loading