Skip to content

options: Ensure that an overly long bitflag string does not crash #576

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.

Sign up for GitHub

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

Merged
rsmarples merged 1 commit into from
Dec 30, 2025
Merged

options: Ensure that an overly long bitflag string does not crash #576

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/if-options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,10 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
ndop->var = np;
if (bp) {
dl = strlen(bp);
if (dl > sizeof(ndop->bitflags)) {
logwarnx("bitflag string too long %s", bp);
dl = sizeof(ndop->bitflags);
}
Comment on lines +2133 to +2136
Copy link

@coderabbitai coderabbitai bot Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -50

Repository: NetworkConfiguration/dhcpcd

Length of output: 2419

🏁 Script executed:

# Search for the struct definition containing bitflags
rg "struct.*ndop|typedef.*ndop" -A 20 | head -100

Repository: 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 -100

Repository: NetworkConfiguration/dhcpcd

Length of output: 1568

🏁 Script executed:

# Alternative: search in header files specifically
rg "struct dhcp_opt|bitflags\[" | grep -E "\.(h|c):" | head -20

Repository: 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 -80

Repository: 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 -20

Repository: 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 2

Repository: 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.

Suggested change
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.

memcpy(ndop->bitflags, bp, dl);
memset(ndop->bitflags + dl, 0,
sizeof(ndop->bitflags) - dl);
Expand Down