Potential buffer overflow in dtls-sock

[nmeum]

The RIOT module dtls-sock is potentially vulnerable to a stack-based buffer overflow. The root cause of this issue is that the current tinyDTLS version, used by RIOT, does not perform any sanity checks on the DTLS fragment length.

This has been fixed on the tinydtls develop branch [1], the tinyDTLS master branch doesn't include this fix yet. Neither does the tinyDTLS version used by RIOT.

Maybe the aforementioned fix should be cherry-picked to the tinyDTLS master branch (CC: @obgm)?

Steps to reproduce

On native:

$ make -C examples/dtls-sock/ all-valgrind
$ make -C examples/dtls-sock/ term-valgrind
> dtlss start

In a different terminal:

echo 'Fv7///wAAABAEPoA8QH8ABD6ABD87PIKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoK/Ozy/BMA/A==' | \
	base64 -d | nc -u '<DTLS Server IP Address>' 20220

This package should crash the RIOT process with a SIGSEGV as it contains a large (invalid) DTLS fragment size which causes an invalid memory access.

Impact

I did not write a POC but I consider this to be an exploitable stack-based buffer overflow. The invalid fragment size is passed to sha256_update. This function contains various memcpy invocations with the attacker controlled len parameter. For example:

RIOT/sys/hashes/sha256.c

Line 248 in d6390b3

memcpy(ctx->buf, src, len);

The buffer this function copies data to is the buf member from the
sha256_context_t:

RIOT/sys/include/hashes/sha256.h

Line 77 in e296f69

unsigned char buf[64];

The sha256_context_t is embedded in the tinyDTLS type
dtls_hmac_context_t as the data member:

https://github.com/eclipse/tinydtls/blob/865ca387cd9d05e52943e5641ad0eefafef218a3/hmac.h#L104

dtls_hash_ctx is a typedef for sha256_context_t:

https://github.com/eclipse/tinydtls/blob/865ca387cd9d05e52943e5641ad0eefafef218a3/hmac.h#L30

The dtls_hmac_context_t type is located on the stack of the
dtls_create_cookie tinyDTLS function:

https://github.com/eclipse/tinydtls/blob/865ca387cd9d05e52943e5641ad0eefafef218a3/dtls.c#L364

In the worst case this potentially allows an attacker to overwrite the return address of the dtls_create_cookie function or some other stack frame using the memcpy invocation from sha256_update. But as I said before I didn't write a POC.

[miri64]

Maybe the aforementioned fix should be cherry-picked to the tinyDTLS master branch (CC: @obgm)?

Or, if not happening on tinydtls master, those commits can also be provided as patches in the RIOT package.

[obgm]

Why don't you just point pkg/tinydtls to the head of the develop branch which contains the needed bugfix?

[miri64]

Why don't you just point pkg/tinydtls to the head of the develop branch which contains the needed bugfix?

If you as one of the developer of tinydtls think that is a good idea, we can certainly do so. In my experience however the development branch is far more volatile and (especially security-related) software packages should rather point to the (assumed to be stable) master branch (not the case with RIOT btw, where master is the development branch).

[obgm]

I agree with your general assessment but for tinydtls, there has not yet been any release while under the roof of the Eclipse organization. Therefore, the current tinydtls master is deemed not more stable than the development branch.
Anyway, I just noted PR #12912 which is the show stopper here. I will take a look at that.

[aabadie]

Can someone confirm that this issue is fixed by #12912 ?

[pokgak]

Confirmed fixed with #12912 using the steps described in OP.

With the PR (9cfdadc), the application does not crash anymore and the shell is still responsive - ifconfig returns successfully.

Using the commit before the PR (26a1348), the application crash:

> dtlss start

[...] # incoming message from another terminal
> ==16912== Invalid read of size 1
==16912==    at 0x135C92: be32enc_vect (sys/hashes/sha256.c:80)
==16912==    by 0x135D51: sha256_transform (sys/hashes/sha256.c:135)
==16912==    by 0x13637D: sha256_update (sys/hashes/sha256.c:242)
==16912==    by 0x13132B: dtls_hash_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.h:61)
==16912==    by 0x1314A4: dtls_hmac_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.c:73)
==16912==    by 0x12A868: dtls_create_cookie (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:385)
==16912==    by 0x12CF0A: dtls_verify_peer (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:1697)
==16912==    by 0x12F162: handle_handshake_msg (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3390)
==16912==    by 0x12F687: handle_handshake (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3549)
==16912==    by 0x13060B: dtls_handle_message (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3936)
==16912==    by 0x11A053: sock_dtls_recv (pkg/tinydtls/contrib/sock_dtls.c:422)
==16912==    by 0x109F51: dtls_server_wrapper (examples/dtls-sock/dtls-server.c:115)
==16912==  Location 0x158b5d is 0 bytes inside main_stack[31293],
==16912==  a global variable declared at kernel_init.c:68
==16912== 
==16912== Invalid read of size 1
==16912==    at 0x135CAD: be32enc_vect (sys/hashes/sha256.c:81)
==16912==    by 0x135D51: sha256_transform (sys/hashes/sha256.c:135)
==16912==    by 0x13637D: sha256_update (sys/hashes/sha256.c:242)
==16912==    by 0x13132B: dtls_hash_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.h:61)
==16912==    by 0x1314A4: dtls_hmac_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.c:73)
==16912==    by 0x12A868: dtls_create_cookie (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:385)
==16912==    by 0x12CF0A: dtls_verify_peer (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:1697)
==16912==    by 0x12F162: handle_handshake_msg (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3390)
==16912==    by 0x12F687: handle_handshake (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3549)
==16912==    by 0x13060B: dtls_handle_message (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3936)
==16912==    by 0x11A053: sock_dtls_recv (pkg/tinydtls/contrib/sock_dtls.c:422)
==16912==    by 0x109F51: dtls_server_wrapper (examples/dtls-sock/dtls-server.c:115)
==16912==  Location 0x158b5c is 0 bytes inside main_stack[31292],
==16912==  a global variable declared at kernel_init.c:68
==16912== 
==16912== Invalid read of size 1
==16912==    at 0x135CC8: be32enc_vect (sys/hashes/sha256.c:82)
==16912==    by 0x135D51: sha256_transform (sys/hashes/sha256.c:135)
==16912==    by 0x13637D: sha256_update (sys/hashes/sha256.c:242)
==16912==    by 0x13132B: dtls_hash_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.h:61)
==16912==    by 0x1314A4: dtls_hmac_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.c:73)
==16912==    by 0x12A868: dtls_create_cookie (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:385)
==16912==    by 0x12CF0A: dtls_verify_peer (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:1697)
==16912==    by 0x12F162: handle_handshake_msg (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3390)
==16912==    by 0x12F687: handle_handshake (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3549)
==16912==    by 0x13060B: dtls_handle_message (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3936)
==16912==    by 0x11A053: sock_dtls_recv (pkg/tinydtls/contrib/sock_dtls.c:422)
==16912==    by 0x109F51: dtls_server_wrapper (examples/dtls-sock/dtls-server.c:115)
==16912==  Location 0x158b5f is 0 bytes inside main_stack[31295],
==16912==  a global variable declared at kernel_init.c:68
==16912== 
==16912== Invalid read of size 1
==16912==    at 0x135CE0: be32enc_vect (sys/hashes/sha256.c:83)
==16912==    by 0x135D51: sha256_transform (sys/hashes/sha256.c:135)
==16912==    by 0x13637D: sha256_update (sys/hashes/sha256.c:242)
==16912==    by 0x13132B: dtls_hash_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.h:61)
==16912==    by 0x1314A4: dtls_hmac_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.c:73)
==16912==    by 0x12A868: dtls_create_cookie (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:385)
==16912==    by 0x12CF0A: dtls_verify_peer (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:1697)
==16912==    by 0x12F162: handle_handshake_msg (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3390)
==16912==    by 0x12F687: handle_handshake (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3549)
==16912==    by 0x13060B: dtls_handle_message (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3936)
==16912==    by 0x11A053: sock_dtls_recv (pkg/tinydtls/contrib/sock_dtls.c:422)
==16912==    by 0x109F51: dtls_server_wrapper (examples/dtls-sock/dtls-server.c:115)
==16912==  Location 0x158b5e is 0 bytes inside main_stack[31294],
==16912==  a global variable declared at kernel_init.c:68
==16912== 
==16912== 
==16912== Process terminating with default action of signal 11 (SIGSEGV)
==16912==  Access not within mapped region at address 0x16B001
==16912==    at 0x135C92: be32enc_vect (sys/hashes/sha256.c:80)
==16912==    by 0x135D51: sha256_transform (sys/hashes/sha256.c:135)
==16912==    by 0x13637D: sha256_update (sys/hashes/sha256.c:242)
==16912==    by 0x13132B: dtls_hash_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.h:61)
==16912==    by 0x1314A4: dtls_hmac_update (examples/dtls-sock/bin/pkg/native/tinydtls/hmac.c:73)
==16912==    by 0x12A868: dtls_create_cookie (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:385)
==16912==    by 0x12CF0A: dtls_verify_peer (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:1697)
==16912==    by 0x12F162: handle_handshake_msg (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3390)
==16912==    by 0x12F687: handle_handshake (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3549)
==16912==    by 0x13060B: dtls_handle_message (examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3936)
==16912==    by 0x11A053: sock_dtls_recv (pkg/tinydtls/contrib/sock_dtls.c:422)
==16912==    by 0x109F51: dtls_server_wrapper (examples/dtls-sock/dtls-server.c:115)
==16912==  If you believe this happened as a result of a stack
==16912==  overflow in your program's main thread (unlikely but
==16912==  possible), you can try to increase the size of the
==16912==  main thread stack using the --main-stacksize= flag.
==16912==  The main thread stack size used in this run was 8388608.
==16912== 
==16912== HEAP SUMMARY:
==16912==     in use at exit: 0 bytes in 0 blocks
==16912==   total heap usage: 1 allocs, 1 frees, 1,024 bytes allocated
==16912== 
==16912== All heap blocks were freed -- no leaks are possible
==16912== 
==16912== For lists of detected and suppressed errors, rerun with: -s
==16912== ERROR SUMMARY: 6521 errors from 8 contexts (suppressed: 0 from 0)
make: *** [/home/pokgak/git/RIOT/boards/native/Makefile.include:145: term-valgrind] Segmentation fault (core dumped)

[nmeum]

Confirmed fixed with #12912 using the steps described in OP.

Has it also been fixed on the latest release branch yet?

[pokgak]

Has it also been fixed on the latest release branch yet?

Also confirmed fixed on current RIOT master (9198fbd).

[pokgak]

@fjmolinas will this be pulled into the 2020.1 release branch?

[aabadie]

If it's already in master, the fix will be part of the next release.

[nmeum]

What about the current release? Shouldn't this be backported?

[fjmolinas]

If it's already in master, the fix will be part of the next release.

We are still not in feature freeze so there is no release branch yet.

[fjmolinas]

Confirmed fixed with #12912 using the steps described in OP.

Has it also been fixed on the latest release branch yet?

@kb2ma this is your release what do you think?. IMO we should just cherry-pick both commits if it fixes the issue without need for more refactoring then backport, if it does not then I would just wait for 2020.01.

[kb2ma]

IMO we should just cherry-pick both commits if it fixes the issue without need for more refactoring then backport, if it does not then I would just wait for 2020.01.

Sounds like a good plan to me. I'll try the cherry-pick in the next day or so.

[kb2ma]

I can't recreate the issue in release 2019.10. dtls-sock was not included with that release; it was added with #11943 on 2019-12-05. I tried to recreate the issue with the dtls-echo example from around that time, but it does not crash when sending the large packet in this issue's description.

Also FWIW, the tinydtls pkg at the time of 2019.10 used an older commit from tinydtls (dcac93f from 2019-03-27).

If @nmeum can provide a failing test case from the head of 2019.10-branch, I can revisit.

[fjmolinas]

The issue has been confirmed as fixed.

[nmeum]

Also FWIW, the tinydtls pkg at the time of 2019.10 used an older commit from tinydtls (dcac93f from 2019-03-27).

@kb2ma Just FYI: tinydtls from commit dcac93f1b38e74f0a57b5df47647943f3df005c2 also contains the vulnerable code path: https://github.com/eclipse/tinydtls/blob/dcac93f1b38e74f0a57b5df47647943f3df005c2/dtls.c#L385-L387 it actually appears that he vulnerable code is present in TinyDTLS since at least February 2016: eclipse/tinydtls@e1388b3#diff-087c8af1d9f50a7df2ea8b32698689a3R342

As such all RIOT versions which ship some variant of TinyDTLS should be vulnerable to this.

I can't recreate the issue in release 2019.10. dtls-sock was not included with that release;

Are you sure? Because the commit adding sock_dtls is also part of the 2019.10-branch. Though the example application examples/dtls-sock isn't.

$ git log -1 fed72571ec2718de6cbf7692b62b5bc6d015165c
commit fed72571ec2718de6cbf7692b62b5bc6d015165c
Author: Aiman Ismail <muhammadaimanbin.ismail@haw-hamburg.de>
Date:   Wed Jun 5 10:54:25 2019 +0200

    sys/net/sock: add sock_dtls API
$ git branch -a --contains fed72571ec2718de6cbf7692b62b5bc6d015165c | grep 2019.10
remotes/origin/2019.10-branch

I can't recreate the issue in release 2019.10. dtls-sock was not included with that release; it was added with #11943 on 2019-12-05. I tried to recreate the issue with the dtls-echo example from around that time, but it does not crash when sending the large packet in this issue's description.

You might not be able to reproduce the issue with the exact same packet, but as I said: The underlying issue in the TinyDTLS code is also present in RIOT release 2019.10. Please correct me if my observations are wrong.

[kb2ma]

#11909 added the API (header) for dtls_sock in 2019.10. #11943 added the first implementation for dtls_sock, which will appear in 2020.01.

After some experimentation I found a way to alter the dtls-echo example, which uses only the tinydtls package, to recreate the issue by using the packet in this issue's description. I plan to follow-up with a couple of PRs in the next day or so.

[kb2ma]

Closing this again without the update to 2019.10 because I actually cannot recreate the problem there. TLDR; based on my original exchange with @fjmolinas, the solution is to wait for the 2020.01 release which should be days away.

What I reported yesterday was the ability to recreate the problem in dtls-echo at commit 6e53e28, which was when the dtls-sock implementation was merged in PR #11943.

The key to recreation of the problem in the dtls-echo example was the use of a 512 byte receive buffer in dtls_handle_read() in dtls-server.c. This is the size of the receive buffer in the server for the dtls-sock example.

However, when I tried to recreate the problem today in 2019.10-branch, I was unable. The likely reason is tinydtls commit 68a1cda, which also changed dtls_cookie_create(), and was added after the commit we used in 2019.10. See the table below for the chronology, where time advances from bottom to top.

tinydtls commit	RIOT status	Notes
7a0420b	2020.01 (with fix in PR #12912)	Includes more changes to dtls_cookie_create(); cannot recreate in dtls-sock or dtls-echo
865ca38	2020.01-develop; dtls-sock appears (commit 6e53e28)	Can recreate in dtls-sock and dtls-echo
68a1cda		Changes to dtls_cookie_create()
dcac93f	2019.10; no dtls-sock	Cannot recreate in dtls-echo

So does the issue @nmeum describes here exist in 2019.10? The same block of code was changed in 68a1cda and 7a0420b. So without a failing test case, I can't say. However, if anyone really wants an update to 2019.10 with the commits in PR #12912, see my personal 2019.10-branch. My next and final step in this follow-up is to create a PR that changes the size of the receive buffer in the dtls-sock example to use DTLS_MAX_BUF.

[nmeum]

So does the issue @nmeum describes here exist in 2019.10? The same block of code was changed in 68a1cda and 7a0420b. So without a failing test case, I can't say.

I would personally propose creating a git-format-patch(1) for eclipse/tinydtls@68a1cda and adding this patches to pkg/tinydtls/patches on the 2019.10 branch. If I understand the pkg/ buildsystem correctly, it should automatically apply this patch, other pkgs (e.g. microcoap) also have such a subdirectory. This should be the easiest and most cost-efficient way to make sure that 2019.10 is not affected by this issue.