eclipse / tinydtls

Eclipse tinydtls
https://projects.eclipse.org/projects/iot.tinydtls
Other
105 stars 58 forks source link

dtls.c: fix off-by-one error in dtls_asn1_len() #152

Closed obgm closed 2 years ago

obgm commented 2 years ago

For ASN.1 integers greater than 128 (i.e., with the most significant bit set) the encoded length might exceed data_len. The length check in the while loop must be done after decrementing data_len.

This PR addresses a bug reported by Shisong Qin, cf. #133

The function can be tested like this:

int main() {
  uint8_t input[] = { 0x87, 0xab, 0xcd };
  uint8_t *data = input;
  size_t n = sizeof(input);
  dtls_asn1_len(&data, &n);
}

Compile with

clang -O0 -fsanitize=address  check_asn1_len.c -o check_asn1_len

The erroneous function would cause the sanitizer to report a stack-buffer-overflow (see below) while staying silent for the fixed version.

=21072==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd6d54a863 at pc 0x0000004ce2d7 bp 0x7ffd6d54a740 sp 0x7ffd6d54a738
READ of size 1 at 0x7ffd6d54a863 thread T0
...
boaks commented 2 years ago

I would prefer to have PRs against "main". Only if the review needs to be postponed, I would cherry-pick them on develop and mark that with a label in the PR.

boaks commented 2 years ago

What's about using "main" instead of "develop"?

if the last changes are put in a PR against main, the LGTM and we can merge them fast into main.

obgm commented 2 years ago

Sure, will do.