`MASK_L` over-shifts for RFC 3610-legal `L ≥ 4`, collapsing the CCM counter (keystream reuse at `L = 4`)

[LukeW1999]

ccm.c:31:

#define MASK_L(_L) ((1 << 8 * _L) - 1)

1 has type int. RFC 3610 §2.1 permits L ∈ [2, 8], so 8 * _L reaches 64
and any L ≥ 4 shifts a 32-bit int by at least its width — undefined
behaviour. ccm.h documents M and L by referring the caller to RFC 3610 and
places no bound of its own, and dtls_ccm_decrypt_message has no documentation
block at all.

This is already happening in the test suite

tests/ccm-testdata.c has four vectors with L = 5, and the loops at
tests/unit-tests/test_ccm.c:33 and :56 run every vector through
dtls_ccm_encrypt_message / dtls_ccm_decrypt_message unfiltered. Running that
loop under UBSan:

ccm.c:221:3: runtime error: shift exponent 40 is too large for 32-bit type 'int'
ccm.c:147:3: runtime error: shift exponent 40 is too large for 32-bit type 'int'
ccm.c:291:3: runtime error: shift exponent 40 is too large for 32-bit type 'int'
35 vectors x 2 directions: 70 pass, 0 fail

The vectors pass while executing the undefined shift, which is presumably why it
has gone unnoticed: at L = 5 the counter is truncated to 8 bits and no vector
is long enough for that to change an output.

The consequence at L = 4 is keystream reuse, not just UB

SET_COUNTER only fills the counter field while C is non-zero:

memset((A) + DTLS_CCM_BLOCKSIZE - (L), 0, (L));
(C) = (cnt) & MASK_L(L);
for (i_ = DTLS_CCM_BLOCKSIZE - 1; (C) && (i_ > (L)); --i_, (C) >>= 8)
  (A)[i_] |= (C) & 0xFF;

On x86-64 the shift count is taken mod 32, so the macro evaluates to:

L	MASK_L(L)	intended
2	65535	65535
3	16777215	16777215
4	0	4294967295
5	255	1099511627775
6	65535	281474976710655
7	16777215	72057594037927935

At L = 4 the mask is 0, so C is always zero, the fill loop never executes,
and every block is encrypted under an identical counter block. L ∈ {5,6,7}
truncate the counter to 8/16/24 bits, which reuses the keystream once a message
exceeds 2^8 / 2^16 / 2^24 blocks.

Reproduction

Clean clone of current master (3482499), built with the project's own CMake
build, only adding -fsanitize=undefined:

git clone --depth 1 https://github.com/eclipse-tinydtls/tinydtls.git td && cd td
cmake -S . -B build -DCMAKE_C_FLAGS="-fsanitize=undefined" \
                    -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=undefined"
cmake --build build -j4

reuse.c — 48 bytes of identical plaintext through the exported API:

#include <stdio.h>
#include <string.h>
#include "ccm.h"

static void run(size_t L) {
    rijndael_ctx ctx;
    unsigned char key[16] = {0}, nonce[16] = {0}, buf[64];
    memset(buf, 0, sizeof buf);
    rijndael_set_key_enc_only(&ctx, key, 128);
    long e = dtls_ccm_encrypt_message(&ctx, 8, L, nonce, buf, 48, buf, 0);
    printf("L=%zu  ct[0:16]==ct[16:32]: %-3s  ct[16:32]==ct[32:48]: %-3s  encrypt->%ld",
           L, memcmp(buf, buf+16, 16) ? "no" : "YES",
              memcmp(buf+16, buf+32, 16) ? "no" : "YES", e);
    long d = dtls_ccm_decrypt_message(&ctx, 8, L, nonce, buf, e, buf, 0);
    printf("  decrypt->%ld (round trip %s)\n", d, d == 48 ? "OK" : "BROKEN");
}
int main(void) { run(3); run(4); return 0; }

gcc -fsanitize=undefined -I. -Ibuild -o reuse reuse.c build/libtinydtls.a && ./reuse

ccm.c:147:3: runtime error: shift exponent 32 is too large for 32-bit type 'int'
ccm.c:221:3: runtime error: shift exponent 32 is too large for 32-bit type 'int'
ccm.c:291:3: runtime error: shift exponent 32 is too large for 32-bit type 'int'
L=3  ct[0:16]==ct[16:32]: no   ct[16:32]==ct[32:48]: no   encrypt->56  decrypt->48 (round trip OK)
L=4  ct[0:16]==ct[16:32]: YES  ct[16:32]==ct[32:48]: YES  encrypt->56  decrypt->48 (round trip OK)

Note the round trip still succeeds at L = 4: encrypt and decrypt are broken
identically, so no encrypt/decrypt test vector can detect this.

Scope — deliberately not overstated

The DTLS layer always passes L = 3, so the protocol path is unaffected, and
I am not aware of any consumer that passes L ≥ 4. The L = 5 vectors in
this repository's own test data are the only occurrences I found. So the
demonstrated impact today is the undefined behaviour itself; the keystream reuse
is a latent consequence for a caller I cannot point to.

I am reporting it as an ordinary issue rather than through SECURITY.md for that
reason. If you would rather it had gone through the security process, say so and
I will not discuss it further in public.

contiki-ng/tinydtls carries the same macro at dtls-ccm.c:30; I have not filed
there.

Suggested fix, and what it was tested against

#define MASK_L(_L) \
    ((_L) >= sizeof(unsigned long) ? ~0UL : ((1UL << (8 * (_L))) - 1))

The guard has to be against sizeof(unsigned long) rather than a literal 4,
since C is unsigned long; on a 32-bit target it degrades to an all-ones mask,
which is the correct mask for a 32-bit counter.

Applied to ccm.c and rebuilt, over all 35 vectors in tests/ccm-testdata.c in
both directions:

	UBSan	vectors
unmodified master	3 shift errors	70 pass, 0 fail
with the fix	silent	70 pass, 0 fail

and the L = 4 keystream reuse above disappears (ct[0:16] != ct[16:32]).

Rejecting L > 8 at the API boundary would also be reasonable, but that changes
behaviour for the existing L = 5 vectors, so it seems better kept separate.

How it was found

Bounded model checking with ESBMC over a harness that calls
dtls_ccm_decrypt_message with every parameter unconstrained except
L ∈ [2,8] and M ∈ {4,…,16} — the range RFC 3610 permits and ccm.h refers
to. Everything above was then confirmed natively; the UBSan output and the
keystream comparison are real runs, not model output.

[boaks]

Thanks for reporting.

L     Number of octets in length field          3 bits  L-1

Not that sure, but in first check it seems, that only 3 is used.

  /* For backwards-compatibility, dtls_encrypt_params is called with
   * M=8 and L=3. */
  const dtls_ccm_params_t params = { nonce, 8, 3 }; 

And RFC 6347 limits also the record size to 2 bytes.

So not sure, when a larger length will be used.

[LukeW1999]

/* For backwards-compatibility, dtls_encrypt_params is called with

• M=8 and L=3. */
  const dtls_ccm_params_t params = { nonce, 8, 3 };

And RFC 6347 limits also the record size to 2 bytes.

So not sure, when a larger length will be used.

Agreed, the protocol path isn't affected.

One thing though: tests/ccm-testdata.c has four vectors with L=5, and test_ccm.c runs all of them. So a build with -fsanitize=undefined trips on the shift during make check. All 70 checks still pass, which is probably why it went unnoticed.

Not a security issue then, more of a cleanup. Up to you whether it's worth the one-liner.

[boaks]

Using 1UL will require "unsigned long" to be 64bit, which is not the case for all platforms.
It would require something as uint64_t and also for the "C" in the calling context (uint64_t counter_tmp).
So FMPOV, I don't consider that pays off.