fix: avoid heap-buffer-overflow in make_sorted_dirlist#98
Open
kugland wants to merge 2 commits into
Open
Conversation
Demonstrates sprintf overflow when readdir returns names > MAXNAMLEN. Uses malloc wrapping with canary detection. Fails on current HEAD, will pass after fix is applied.
The "%s%s" sprintf into a fixed strlen(path) + MAXNAMLEN + 1 buffer relied on a debug-only assert to bound the directory entry name. In release builds, and on filesystems whose runtime _PC_NAME_MAX exceeds the compile-time NAME_MAX, sprintf could overflow the heap allocation. Use xasprintf so the bound is unconditional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make_sorted_dirlist()sizes its filename buffer asxmalloc(strlen(path) + MAXNAMLEN + 1)and thensprintf("%s%s", path, ent->d_name)into it, guarded only byassert(strlen(ent->d_name) <= MAXNAMLEN). Release builds defineNDEBUG(darkhttpd.c near the top), so the assert is stripped. On filesystems whose runtime_PC_NAME_MAXexceeds the compile-timeMAXNAMLEN(e.g. some FUSE filesystems, orMAXNAMLENbeing smaller than expected on some platforms),readdir()can return a longer name and thesprintfoverruns the heap allocation.Fix: drop the fixed-size buffer and use
xasprintf("%s%s", path, ent->d_name). The bound is then unconditional regardless ofNDEBUGor_PC_NAME_MAX.Commits
master).This makes the issue easy to reproduce on the test commit alone.
Test plan
The test (
devel/test_dirlist_overflow.c) stubsopendir/readdirto feedmake_sorted_dirlist()an entry whosed_nameis wider thanMAXNAMLEN, and uses-Wl,--wrap=malloc,--wrap=realloc,--wrap=freeto attach a canary past each allocation. The overflow stamps the canary; the test reportsFAILand exits 1. With the fix, no canary is touched.FAIL: heap-buffer-overflow in make_sorted_dirlist (sprintf past xmalloc bound), exit 1.PASS: no heap-buffer-overflow detected, exit 0.devel/run-testsso it runs with the rest of the suite.