Fix: sodium_misuse() callback runs under global lock and can deadlock

SUMMARY
`src/libsodium/sodium/core.c` invokes the process-global misuse callback from `sodium_misuse()` while still holding the library-wide critical section. If the application-installed callback re-enters any API path that acquires the same lock, including `sodium_set_misuse_handler()`, execution deadlocks before `abort()` is reached. This breaks the intended fail-stop behavior of misuse handling.

PROVENANCE
Verified from the provided finding, reproduced locally from the committed control flow, and documented for Swival Security Scanner (https://swival.dev).

PRECONDITIONS
- A caller installs a misuse handler via `sodium_set_misuse_handler()`.
- The handler re-enters an API path that takes the same global critical section, including `sodium_set_misuse_handler()`.

PROOF
1. `sodium_set_misuse_handler()` writes the global `_misuse_handler` under `sodium_crit_enter()` / `sodium_crit_leave()` in `src/libsodium/sodium/core.c:170` and `src/libsodium/sodium/core.c:174`.
2. `sodium_misuse()` acquires that same critical section, copies `_misuse_handler`, and invokes `handler()` before releasing the lock in `src/libsodium/sodium/core.c:155` and `src/libsodium/sodium/core.c:160`.
3. The pthread-backed critical section in this file is non-recursive (`src/libsodium/sodium/core.c:89`), so a callback that calls `sodium_set_misuse_handler()` blocks in `sodium_crit_enter()` waiting on the lock already held by `sodium_misuse()`.
4. Because `sodium_misuse()` is waiting for the callback to return, it never reaches `abort()`, converting a fail-stop misuse path into a hang.
5. Reproduction confirmed this with a minimal pthread harness: the handler printed `handler: before reentry` and then hung until terminated by `timeout`.

WHY THIS IS A REAL BUG
The callback target is application-controlled through an exported setter, and `sodium_misuse()` calls it on a misuse path without enforcing any non-reentrancy contract. On pthread targets, a handler that performs a supported API call can permanently block process termination. That is a reachable behavioral failure, not a theoretical lock-order concern.

FIX REQUIREMENT
Load `_misuse_handler` while holding the lock, release the critical section, and only then invoke the callback. This removes lock-dependent behavior from arbitrary user code while preserving synchronized access to the global handler pointer.
This commit is contained in:
Frank Denis
2026-04-16 23:00:13 +02:00
parent 83edcb43b1
commit df8802b013
+1 -1
View File
@@ -196,7 +196,7 @@ sodium_misuse(void)
(void) sodium_crit_leave();
if (sodium_crit_enter() == 0) {
handler = _misuse_handler;
if (handler != NULL) {
if (sodium_crit_leave() == 0 && handler != NULL) {
handler();
}
}