`dtls_get_cookie`: dereference before the length check (currently masked by `dtls_create_cookie`)

[LukeW1999]

Not urgent — I believe this is unreachable in the current tree, and I say why
below. It is a hardening report: the check that makes the dereference safe lives
in a different function.

The site

dtls.c:439

  msglen -= DTLS_HS_LENGTH + DTLS_CH_LENGTH;
  msg    += DTLS_HS_LENGTH + DTLS_CH_LENGTH;

  /* skip session id */
  SKIP_VAR_FIELD(msg, msglen, uint8, DTLS_ALERT_HANDSHAKE_FAILURE,
                 "get_cookie, session_id");

  if (msglen < (*msg & 0xff) + sizeof(uint8))
    return dtls_alert_fatal_create(DTLS_ALERT_HANDSHAKE_FAILURE);

SKIP_VAR_FIELD is correctly bounded but can leave msglen == 0, with msg
pointing one past the end of the message. The next line then reads *msg.

This site reads a length-prefixed field by hand, and it is doing the second
half of what SKIP_VAR_FIELD does while omitting the first:

#define SKIP_VAR_FIELD(P, L, T, A, M) {                \
    size_t skip_length = sizeof(T);                    \
    if (L < skip_length) { ... return ...; }           \   /* <-- this one */
    skip_length += dtls_ ## T ## _to_int(P);           \
    if (L < skip_length) { ... return ...; }           \   /* <-- present */

Reproduction

dtls_get_cookie is static, so the driver includes the translation unit.
Clean clone of master (3482499), the project's own CMake build with
-fsanitize=address added and nothing else changed:

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

direct.c:

#include <stdlib.h>
#include <stdio.h>
#include "dtls.c"

int main(void) {
    size_t msglen = 60;
    uint8 *msg = malloc(msglen);      /* exactly msglen bytes */
    memset(msg, 0, msglen);
    msg[12] = 0xfe; msg[13] = 0xfd;   /* version, at msg + DTLS_HS_LENGTH */
    msg[46] = 13;                     /* session_id length; body ends at 60 */
    uint8 *cookie = NULL;
    printf("dtls_get_cookie -> %d\n", dtls_get_cookie(msg, msglen, &cookie));
    free(msg); return 0;
}

gcc -fsanitize=address -g -O0 -I. -Iasan -o direct direct.c asan/libtinydtls.a && ./direct

==251536==ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1 at 0x50600000005c
    #0 dtls_get_cookie dtls.c:455
0x50600000005c is located 0 bytes to the right of 60-byte region

The message is a 60-byte handshake message whose session id ends exactly at the
end of the buffer: 12 (handshake header) + 34 (dtls_client_hello_t) + 1
(session id length = 13) + 13 = 60.

Why I think it cannot happen today

The only caller is dtls_0_verify_peer (dtls.c:2249), which calls
dtls_create_cookie first with the same (data, data_length) and returns on
error. dtls_create_cookie performs the longer parse — after the session id it
also skips the cookie, the cipher suites and the compression list — and its
cookie skip requires at least one byte to remain. That is the byte
dtls_get_cookie dereferences unchecked, so no message accepted by
dtls_create_cookie can drive dtls_get_cookie out of bounds.

I checked this with a bounded model checker rather than only by reading:
composing the two calls the way dtls_0_verify_peer does, and running every
message up to 60 bytes with all bytes unconstrained, reports no violation — and a
reachability probe confirms messages do reach dtls_get_cookie, so the result is
not vacuous. Caveats: bounded at 60 bytes, and the HMAC digest was replaced by
its documented length (the cookie's contents cannot affect whether
dtls_get_cookie is reached).

Why it still seems worth a line

• The invariant is non-local and undocumented: nothing in dtls_get_cookie
  says a caller must have run dtls_create_cookie first. A change to the cookie
  path, or a second caller, makes it live.
• A unit-level fuzz target on dtls_get_cookie — a natural thing to add, since
  the repository currently has none — hits it immediately.

Fix

  if (msglen < sizeof(uint8) || msglen < (*msg & 0xff) + sizeof(uint8))

Behaviour-preserving for every input. When msglen == 0 the original
condition 0 < (*msg & 0xff) + sizeof(uint8) is true whatever the out-of-bounds
byte happens to be, since the right-hand side is at least 1; so both versions
take the same branch and return the same alert. When msglen >= 1 the added
disjunct is false and the two conditions are identical.

Confirmed: with the change, the reproduction above prints
dtls_get_cookie -> -552 — the same value as before — and ASan is silent.

[boaks]

Thanks for reporting.

[boaks]

See PR #280.