Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid Heap Memory Access in DTLS Server/Client #245

Closed
5angjun opened this issue Jan 15, 2025 · 8 comments
Closed

Invalid Heap Memory Access in DTLS Server/Client #245

5angjun opened this issue Jan 15, 2025 · 8 comments
Labels
bug Something isn't working please retest Please retest the related PR or commit, if that works for you

Comments

@5angjun
Copy link

5angjun commented Jan 15, 2025

Issue Summary

  • Running the DTLS server under Valgrind and AddressSanitizer detected multiple memory access violations, including double-free and heap-use-after-free errors.
  • These errors occur when both the SIGINT handler and server context cleanup routines attempt to free the same memory. While this issue may not be directly exploitable, it can cause undefined behavior, potentially destabilizing the server/client.

How to reproduce

  • I fixed a random/cookie generation routine for easily understanding and writing Proof-of-Concept.
  • I add a raise(2) routine for easily triggering race condition
  • I hope you understand.
cd tests
valgrind --leak-check=full ./dtls-server
python3 reproduce.py

Double-Free-Bugs

Function: SIGINT handler and server/client cleanup routines.
Description: Memory for a context is freed twice, once during signal handling and once during normal cleanup.

=================================================================
==55548==ERROR: AddressSanitizer: attempting double-free on 0x615000002d80 in thread T0:
    #0 0x4948fd  (/home/sangjun/tinydtls/tests/dtls-server+0x4948fd)
    #1 0x4efbd4  (/home/sangjun/tinydtls/tests/dtls-server+0x4efbd4)
    #2 0x4fe244  (/home/sangjun/tinydtls/tests/dtls-server+0x4fe244)
    #3 0x4d7245  (/home/sangjun/tinydtls/tests/dtls-server+0x4d7245)
    #4 0x4c645c  (/home/sangjun/tinydtls/tests/dtls-server+0x4c645c)

Heap-Use-After-Free

Function: dtls_handle_message and handle_alert.
Description: Freed memory is accessed during message handling, leading to undefined behavior.

==85957==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000003b60 at pc 0x0000004c8f9f bp 0x7fffffffcbe0 sp 0x7fffffffcbd8
READ of size 8 at 0x615000003b60 thread T0
    #0 0x4c8f9e  (/home/sangjun/tinydtls/tests/dtls-server+0x4c8f9e)
    #1 0x4d50bf  (/home/sangjun/tinydtls/tests/dtls-server+0x4d50bf)
    #2 0x4d7245  (/home/sangjun/tinydtls/tests/dtls-server+0x4d7245)
    #3 0x4c645c  (/home/sangjun/tinydtls/tests/dtls-server+0x4c645c)

Attachments:

Root Cause Analysis

The SIGINT handler and the server/client's main execution flow both attempt to free the same context.

In the case of an interrupt (e.g., SIGINT), the cleanup routine may execute concurrently with other server logic, resulting in double-free or use-after-free errors.

Impact Assessment

While this issue is unlikely to be directly exploitable (due to ASLR and modern memory protections), it can:

  • Cause server/Client crashes.

  • Result in undefined behavior, impacting server reliability.

  • Make debugging and further development more complex.

Conclusion

  • This memory management issue highlights a race condition between signal handling and normal server/client cleanup. Implementing synchronization mechanisms and refactoring signal handling can prevent these errors, ensuring server/client stability and reliability.
@boaks
Copy link
Contributor

boaks commented Jan 15, 2025

That happens only on shutdown an only with the simple examples, right?

@5angjun
Copy link
Author

5angjun commented Jan 15, 2025

The issue occurs exclusively during shutdown when a SIGINT signal is triggered.

However, it is not confined to simple examples. As evidenced in the actual ASAN logs, the problem is reproducible even in scenarios where SIGINT is programmatically triggered without relying on a direct raise call.
This behavior closely resembles a race condition, adding significant complexity to debugging and further development efforts.

@5angjun
Copy link
Author

5angjun commented Jan 15, 2025

Shutdown bugs might appear trivial, but they often reveal underlying issues in memory management or resource cleanup. Such issues can compromise system stability and security, while neglecting them may lead to more complex debugging and maintenance challenges. Proactively addressing these bugs enhances system reliability and mitigates potential risks. For further details, please refer to CWE-364: Signal Handler Race Condition.

@boaks
Copy link
Contributor

boaks commented Jan 15, 2025

My point is more, that this affects the shutdown only of the simple test examples.

I'm not sure what the current grants about the usage with multiple threads are.
My experience (usage) is execution from a single thread only and that works very well.

@mrdeep1
Copy link
Contributor

mrdeep1 commented Jan 15, 2025

In general interrupt handlers should not be free'ing off information that will get freed off elsewhere. Set a flag in the interrupt handler and then process the flag in the main general loop as per:

diff --git a/tests/dtls-server.c b/tests/dtls-server.c
index fa684d4..6c7cc08 100644
--- a/tests/dtls-server.c
+++ b/tests/dtls-server.c
@@ -50,6 +50,8 @@ static const dtls_cipher_t* ciphers = NULL;
 static unsigned int force_extended_master_secret = 0;
 static unsigned int force_renegotiation_info = 0;

+static int quit = 0; /* Set to 1 if the application is to exit */
+
 #ifdef DTLS_ECC
 static const unsigned char ecdsa_priv_key[] = {
             0xD9, 0xE2, 0x70, 0x7A, 0x72, 0xDA, 0x6A, 0x05,
@@ -252,9 +254,8 @@ dtls_handle_read(struct dtls_context_t *ctx) {

 static void dtls_handle_signal(int sig)
 {
-  dtls_free_context(the_context);
-  signal(sig, SIG_DFL);
-  kill(getpid(), sig);
+  (void)sig;
+  quit = 1;
 }

 static int
@@ -455,7 +456,7 @@ main(int argc, char **argv) {

   dtls_set_handler(the_context, &cb);

-  while (1) {
+  while (!quit) {
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);

@5angjun
Copy link
Author

5angjun commented Jan 15, 2025

Thank you for your clarification.

You are correct that this issue primarily affects the shutdown phase in the provided test examples. And it can only affect on example. However, these test examples are indeed one problem as Issue#98 mentioned

Regarding your point about single-threaded usage, the current implementation indeed assumes single-threaded execution works well in normal operations. However, the interaction between the SIGINT handler and the main execution flow introduces a race condition even in single-threaded contexts, primarily because SIGINT signals and their handlers are processed asynchronously.

Issues like race conditions or something like this issue, caused by shared code or resources, can arise when multiple execution paths—whether in multithreaded or single-threaded contexts—access shared data or resources without proper synchronization.

The key point is that it can also occur in single-threaded environments.
For instance:

  • Signal handlers, by nature, can interrupt any part of the execution flow. If the handler attempts to free resources already being managed/freed by the main thread, issues like double-free or use-after-free arise.

To ensure robust operation, this can resolve the problem of the test client/sever.
For example

  • Use a flag to indicate whether the cleanup process has already been initiated.
  • Postpone resource deallocation until the main loop confirms it’s safe to proceed.

This approach would eliminate the race condition and enhance reliability.

Best Regards

@boaks boaks added bug Something isn't working please retest Please retest the related PR or commit, if that works for you labels Jan 18, 2025
@boaks
Copy link
Contributor

boaks commented Jan 18, 2025

Please retest with the merged PR.
If the issues is fixed, please close this issue.

@5angjun
Copy link
Author

5angjun commented Jan 18, 2025

It appears that this bug is fixed in #246.
We can close this issue.

Thank you.

@5angjun 5angjun closed this as completed Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working please retest Please retest the related PR or commit, if that works for you
Projects
None yet
Development

No branches or pull requests

3 participants