options: Ensure that an overly long bitflag string does not crash #576
Conversation
WalkthroughThis PR fixes a heap buffer overflow vulnerability in the DHCP option parsing code. A bounds check is added before copying the bitflag string to the heap-allocated buffer. If the input exceeds the buffer size, the length is clamped and a warning is logged, preventing the out-of-bounds write. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/if-options.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: freebsd
- GitHub Check: openbsd
| if (dl > sizeof(ndop->bitflags)) { | ||
| logwarnx("bitflag string too long %s", bp); | ||
| dl = sizeof(ndop->bitflags); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Examine the code context around lines 2133-2136
cat -n src/if-options.c | sed -n '2125,2145p'Repository: NetworkConfiguration/dhcpcd
Length of output: 802
🏁 Script executed:
# Find the structure definition for ndop to understand bitflags buffer
rg "bitflags" -A 2 -B 2 | head -50Repository: NetworkConfiguration/dhcpcd
Length of output: 2419
🏁 Script executed:
# Search for the struct definition containing bitflags
rg "struct.*ndop|typedef.*ndop" -A 20 | head -100Repository: NetworkConfiguration/dhcpcd
Length of output: 808
🏁 Script executed:
# Find struct dhcp_opt definition in header files
fd -e h -e c | xargs rg "struct dhcp_opt\s*\{" -A 30 | head -100Repository: NetworkConfiguration/dhcpcd
Length of output: 1568
🏁 Script executed:
# Alternative: search in header files specifically
rg "struct dhcp_opt|bitflags\[" | grep -E "\.(h|c):" | head -20Repository: NetworkConfiguration/dhcpcd
Length of output: 1247
🏁 Script executed:
# Find the header that defines struct dhcp_opt
fd -name "*.h" | xargs rg "struct dhcp_opt" -B 2 -A 15 | head -80Repository: NetworkConfiguration/dhcpcd
Length of output: 2930
🏁 Script executed:
# Check the ASan report mentioned in the original review to understand the vulnerability
rg "issue.*573|buffer.*overflow" | head -20Repository: NetworkConfiguration/dhcpcd
Length of output: 284
🏁 Script executed:
# Verify there are no other memcpy operations on bitflags that might be affected
rg "bitflags.*memcpy|memcpy.*bitflags" -B 2 -A 2Repository: NetworkConfiguration/dhcpcd
Length of output: 299
Security fix correctly prevents the heap buffer overflow.
The bounds check at line 2133 properly prevents out-of-bounds writes by clamping the input length to the 8-byte bitflags buffer before the memcpy at line 2137. The subsequent memset at lines 2138-2139 correctly zeros the remaining bytes.
Consider limiting the displayed length of bp in the warning message.
The logwarnx at line 2134 includes the full bp string without length limits. If an attacker provides an extremely long bitflag string, logging it could cause secondary issues like log spam or performance degradation. Consider truncating the displayed portion:
if (dl > sizeof(ndop->bitflags)) {
- logwarnx("bitflag string too long %s", bp);
+ logwarnx("bitflag string too long (length %zu, max %zu): %.50s%s",
+ dl, sizeof(ndop->bitflags), bp, dl > 50 ? "..." : "");
dl = sizeof(ndop->bitflags);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (dl > sizeof(ndop->bitflags)) { | |
| logwarnx("bitflag string too long %s", bp); | |
| dl = sizeof(ndop->bitflags); | |
| } | |
| if (dl > sizeof(ndop->bitflags)) { | |
| logwarnx("bitflag string too long (length %zu, max %zu): %.50s%s", | |
| dl, sizeof(ndop->bitflags), bp, dl > 50 ? "..." : ""); | |
| dl = sizeof(ndop->bitflags); | |
| } |
🤖 Prompt for AI Agents
In src/if-options.c around lines 2133 to 2136, the current logwarnx prints the
entire bp string which could be extremely long; change the warning to log only a
limited prefix of bp (e.g. first 64 bytes) and append "..." when truncated.
Implement this by computing a display length (min(strlen(bp), 64)), copying that
prefix into a small stack buffer or using a bounded-print helper,
null-terminating it, and passing that truncated string to logwarnx so the
message remains informative without risking log spam or heavy memory usage.
Fixes #573.