Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 11 additions & 5 deletions src/dhcp.c
Original file line number Diff line number Diff line change
Expand Up @@ -1173,7 +1173,7 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
*lp = (uint8_t)(*lp + vivco->len + 1);
}
}

if (ifo->vsio_len &&
!has_option_mask(ifo->nomask, DHO_VIVSO))
{
Expand Down Expand Up @@ -1864,15 +1864,21 @@ send_message(struct interface *ifp, uint8_t type,
state->xid);
RT = 0; /* bogus gcc warning */
} else {
unsigned int jitter = ifo->backoff_jitter;

if (state->interval == 0)
state->interval = 4;
state->interval = ifo->initial_interval;
else {
unsigned int cutoff = ifo->backoff_cutoff;

state->interval *= 2;
if (state->interval > 64)
state->interval = 64;
if (state->interval > cutoff)
state->interval = cutoff;
}

RT = (state->interval * MSEC_PER_SEC) +
(arc4random_uniform(MSEC_PER_SEC * 2) - MSEC_PER_SEC);
(arc4random_uniform(jitter * 2) - jitter);
Comment on lines +1867 to +1880
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cd src && sed -n '1860,1890p' dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 969


🏁 Script executed:

# Search for RT definition and type in dhcp.c
rg -n 'unsigned int.*RT\s*=' src/dhcp.c | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Check MSEC_PER_SEC definition
rg -n 'define MSEC_PER_SEC' --type c

Repository: NetworkConfiguration/dhcpcd

Length of output: 112


🏁 Script executed:

# Look for struct state definition to understand state->interval type
rg -n 'struct.*state\s*{' -A 20 src/dhcp.c | head -50

Repository: NetworkConfiguration/dhcpcd

Length of output: 168


🏁 Script executed:

# Check struct if_options to see types of backoff_* fields
rg -n 'struct if_options\s*{' -A 50 --type c | grep -E '(backoff|interval|jitter)' | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 176


🏁 Script executed:

# Search for RT declaration in send_message function
rg -n 'unsigned int.*RT' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 93


🏁 Script executed:

# Find the function containing this code - likely send_message
rg -n 'send_message' src/dhcp.c | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 415


🏁 Script executed:

# Read more context around line 1867 to see RT declaration
sed -n '1800,1880p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 2107


🏁 Script executed:

# Search for state->interval type - check struct definition
rg -n 'struct.*state' src/dhcp.c | grep -v '//' | head -10

Repository: NetworkConfiguration/dhcpcd

Length of output: 529


🏁 Script executed:

# Find dhcp_state struct definition
rg -n 'struct dhcp_state' src/dhcp.c | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 282


🏁 Script executed:

# Look for struct dhcp_state { in header files
fd -e h -e c src | xargs rg 'struct dhcp_state\s*\{' -A 30 | head -80

Repository: NetworkConfiguration/dhcpcd

Length of output: 991


🏁 Script executed:

# Find if_options struct - likely in a header file
fd -e h src | xargs rg 'struct if_options' -A 50 | head -100

Repository: NetworkConfiguration/dhcpcd

Length of output: 3882


🏁 Script executed:

# Search for struct if_options definition more directly
fd -e h src | xargs grep -l 'if_options'

Repository: NetworkConfiguration/dhcpcd

Length of output: 55


🏁 Script executed:

# Look at if.h which likely has if_options
cat src/if.h | head -200

Repository: NetworkConfiguration/dhcpcd

Length of output: 6139


🏁 Script executed:

# Search for if_options struct definition across all header files
fd -e h src | xargs grep -n 'struct if_options' 2>/dev/null | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Look at dhcpcd.h or similar for if_options
rg -n 'struct if_options' -A 100 src/ | head -150

Repository: NetworkConfiguration/dhcpcd

Length of output: 5685


🏁 Script executed:

# Search for if_options definition in dhcpcd.h or similar main headers
rg -n 'struct if_options\s*\{' -A 150 src/dhcpcd.h 2>/dev/null | head -200

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Try looking in if-options.h
fd if-options.h src

Repository: NetworkConfiguration/dhcpcd

Length of output: 87


🏁 Script executed:

# Search more broadly for struct if_options with open brace
rg 'struct if_options\s*\{' -l src/

Repository: NetworkConfiguration/dhcpcd

Length of output: 87


🏁 Script executed:

cat src/if-options.h | head -300

Repository: NetworkConfiguration/dhcpcd

Length of output: 9442


🏁 Script executed:

# Get the types in struct dhcp_state
sed -n '1,50p' src/dhcp.h | grep -A 20 'struct dhcp_state'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Verify how RT is used after calculation to understand the impact
sed -n '1880,1920p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 1520


🏁 Script executed:

# Check where RT is used after the calculation - look for the timeout scheduling
sed -n '1920,1960p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 1186


🏁 Script executed:

# Look for eloop_timeout_add or similar functions that might use RT
rg -n 'eloop_timeout_add|eloop_add_timeout' src/dhcp.c | head -10

Repository: NetworkConfiguration/dhcpcd

Length of output: 582


🏁 Script executed:

# Search for how RT is used in the send_message function and after
rg -n 'RT' src/dhcp.c | grep -A 2 -B 2 '188[0-9]:'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Check line 1974 - how RT is passed to eloop_timeout_add_msec
sed -n '1970,1980p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 337


🏁 Script executed:

# Check the function signature for eloop_timeout_add_msec to understand what it expects
rg -n 'eloop_timeout_add_msec' -A 3 src/eloop.c src/eloop.h | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 356


🏁 Script executed:

# Now let's trace through the arithmetic more carefully
# Check if there are any bounds on backoff_jitter or initial_interval
rg -n 'backoff_jitter\|initial_interval\|backoff_cutoff' src/if-options.c | head -30

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Find eloop_q_timeout_add_msec definition
rg -n 'eloop_q_timeout_add_msec' src/eloop.h -A 5

Repository: NetworkConfiguration/dhcpcd

Length of output: 736


🏁 Script executed:

# Check the actual implementation to see what type is expected
rg -n 'eloop_q_timeout_add_msec' src/eloop.c -A 10 | head -30

Repository: NetworkConfiguration/dhcpcd

Length of output: 363


🏁 Script executed:

# Look at how backoff_jitter is parsed/set - are there bounds?
sed -n '1,100p' src/if-options.c | grep -i 'backoff'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Search for where backoff_jitter gets assigned to understand its bounds
rg -n 'backoff_jitter\s*=' src/if-options.c -B 3 -A 3 | head -50

Repository: NetworkConfiguration/dhcpcd

Length of output: 581


🏁 Script executed:

# Check if there's any validation of RT value before passing to eloop_timeout_add_msec
sed -n '1868,1975p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 3172


🏁 Script executed:

# Verify the default config makes this vulnerable
rg -n 'DEFAULT_INITIAL_INTERVAL|DEFAULT_BACKOFF_JITTER' src/if-options.h

Repository: NetworkConfiguration/dhcpcd

Length of output: 199


Fix unsigned integer arithmetic wrapping in the DHCP retransmission timeout calculation.

The expression arc4random_uniform(jitter * 2) - jitter wraps when the random value is less than jitter, since both operands are unsigned int. With default config (initial_interval 4 and backoff_jitter 1000), a random value under 500 causes RT to wrap to near UINT_MAX, scheduling an incorrect timeout. The scenario with initial_interval 1 and backoff_jitter 2000 reliably triggers wrap on any random value below 2000.

Perform the jitter calculation in signed or wider integer space and clamp the result before assigning to RT.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dhcp.c` around lines 1867 - 1880, The jitter subtraction currently
happens in unsigned space causing wrap; change the jitter computation to use a
signed/wider integer: compute a signed jitter_offset =
(int64_t)arc4random_uniform((uint32_t)ifo->backoff_jitter * 2) -
(int64_t)ifo->backoff_jitter, clamp jitter_offset to a reasonable range if
needed, then compute RT using signed/wider arithmetic as RT =
(int64_t)state->interval * MSEC_PER_SEC + jitter_offset and assign back ensuring
types match (use explicit casts) to avoid unsigned wrap; update the code around
variables jitter, state->interval, RT and the arc4random_uniform call.


/* No carrier? Don't bother sending the packet.
* However, we do need to advance the timeout. */
if (!if_is_link_up(ifp))
Expand Down
36 changes: 36 additions & 0 deletions src/dhcpcd.conf.5.in
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,42 @@ You can use this option to stop this from happening.
.It Ic fallback Ar profile
Fall back to using this profile if DHCP fails.
This allows you to configure a static profile instead of using ZeroConf.
.It Ic initial_interval Ar seconds
Set the initial DHCPv4 retransmission interval to
.Ar seconds .
The minimum value is 1.
The default is 4 seconds as per RFC 2131.
This option only affects DHCPv4;
DHCPv6 retransmission is governed by RFC 8415.
See also
.Ic backoff_cutoff
and
.Ic backoff_jitter .
.It Ic backoff_cutoff Ar seconds
Cap the DHCPv4 exponential backoff interval at
.Ar seconds .
The minimum value is 1.
The default is 64 seconds as per RFC 2131.
Setting this to 1 effectively disables exponential growth, so
retransmissions use only the initial interval plus jitter.
This option only affects DHCPv4;
DHCPv6 retransmission is governed by RFC 8415.
See also
.Ic initial_interval
and
.Ic backoff_jitter .
.It Ic backoff_jitter Ar milliseconds
Set the random jitter applied to each DHCPv4 retransmission interval.
The jitter is applied as \(+-
.Ar milliseconds .
The default is 1000 (\(+-1 second) as per RFC 2131.
A value of 0 disables jitter, producing deterministic retransmission timing.
This option only affects DHCPv4;
DHCPv6 retransmission is governed by RFC 8415.
See also
.Ic backoff_cutoff
and
.Ic initial_interval .
.It Ic fallback_time Ar seconds
Start fallback after
.Ar seconds .
Expand Down
33 changes: 33 additions & 0 deletions src/if-options.c
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ const struct option cf_options[] = {
{"fallback_time", required_argument, NULL, O_FALLBACK_TIME},
{"ipv4ll_time", required_argument, NULL, O_IPV4LL_TIME},
{"nosyslog", no_argument, NULL, O_NOSYSLOG},
{"initial_interval", required_argument, NULL, O_INITIAL_INTERVAL},
{"backoff_cutoff", required_argument, NULL, O_BACKOFF_CUTOFF},
{"backoff_jitter", required_argument, NULL, O_BACKOFF_JITTER},
{NULL, 0, NULL, '\0'}
};

Expand Down Expand Up @@ -2580,6 +2583,33 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
logsetopts(logopts);
}
break;
case O_INITIAL_INTERVAL:
ARG_REQUIRED;
ifo->initial_interval =
(uint32_t)strtou(arg, NULL, 0, 1, UINT32_MAX, &e);
if (e) {
logerrx("invalid initial interval: %s", arg);
return -1;
}
break;
case O_BACKOFF_CUTOFF:
ARG_REQUIRED;
ifo->backoff_cutoff =
(uint32_t)strtou(arg, NULL, 0, 1, UINT32_MAX, &e);
if (e) {
logerrx("invalid backoff cutoff: %s", arg);
return -1;
}
break;
Comment on lines +2586 to +2603
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Validate the new knobs together, not just individually.

Right now backoff_cutoff can be set below initial_interval. That makes the sequence shrink (4s -> 1s -> 1s) instead of cap, which contradicts the new manpage text and the expected meaning of a cutoff. Please clamp or reject that combination in finish_config() once both values are known.

🛠️ Possible clamp in finish_config()
 static void
 finish_config(struct if_options *ifo)
 {
+	if (ifo->backoff_cutoff < ifo->initial_interval) {
+		logerrx("backoff_cutoff must be >= initial_interval");
+		ifo->backoff_cutoff = ifo->initial_interval;
+	}
+
 	/* Terminate the encapsulated options */
 	if (ifo->vendor[0] && !(ifo->options & DHCPCD_VENDORRAW)) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/if-options.c` around lines 2586 - 2603, The new options backoff_cutoff
and initial_interval must be validated together in finish_config(): after
parsing, check if ifo->backoff_cutoff is less than ifo->initial_interval and
either clamp backoff_cutoff to initial_interval or return an error; update
finish_config() to perform this comparison and adjust or reject the
configuration (modify the function that finalizes options, e.g.,
finish_config()) and provide a clear logerrx or warning message indicating the
clamp or failure including both values.

case O_BACKOFF_JITTER:
ARG_REQUIRED;
ifo->backoff_jitter =
(uint32_t)strtou(arg, NULL, 0, 0, UINT32_MAX, &e);
if (e) {
logerrx("invalid backoff jitter: %s", arg);
return -1;
}
break;
default:
return 0;
}
Expand Down Expand Up @@ -2667,6 +2697,9 @@ default_config(struct dhcpcd_ctx *ctx)
#ifdef INET
ifo->fallback_time = DEFAULT_FALLBACK;
ifo->ipv4ll_time = DEFAULT_IPV4LL;
ifo->initial_interval = DEFAULT_INITIAL_INTERVAL;
ifo->backoff_cutoff = DEFAULT_BACKOFF_CUTOFF;
ifo->backoff_jitter = DEFAULT_BACKOFF_JITTER;
#endif
ifo->metric = -1;
ifo->auth.options |= DHCPCD_AUTH_REQUIRE;
Expand Down
9 changes: 9 additions & 0 deletions src/if-options.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
#define DEFAULT_REQUEST 180 /* secs to request, mirror DHCP6 */
#define DEFAULT_FALLBACK 5 /* secs until fallback */
#define DEFAULT_IPV4LL 5 /* secs until ipv4ll */
#define DEFAULT_INITIAL_INTERVAL 4 /* DHCP_BASE per RFC 2131 */
#define DEFAULT_BACKOFF_CUTOFF 64 /* DHCP_MAX per RFC 2131 */
#define DEFAULT_BACKOFF_JITTER 1000 /* +/- milliseconds */

#ifndef HOSTNAME_MAX_LEN
#define HOSTNAME_MAX_LEN 250 /* 255 - 3 (FQDN) - 2 (DNS enc) */
Expand Down Expand Up @@ -190,6 +193,9 @@
#define O_VSIO O_BASE + 57
#define O_VSIO6 O_BASE + 58
#define O_NOSYSLOG O_BASE + 59
#define O_INITIAL_INTERVAL O_BASE + 60
#define O_BACKOFF_CUTOFF O_BASE + 61
#define O_BACKOFF_JITTER O_BASE + 62

extern const struct option cf_options[];

Expand Down Expand Up @@ -257,6 +263,9 @@ struct if_options {
uint32_t request_time;
uint32_t fallback_time;
uint32_t ipv4ll_time;
uint32_t initial_interval;
uint32_t backoff_cutoff;
uint32_t backoff_jitter;
unsigned long long options;
bool randomise_hwaddr;

Expand Down
4 changes: 2 additions & 2 deletions tests/Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
SUBDIRS= crypt eloop-bench
SUBDIRS= crypt eloop-bench backoff

all:
all:
for x in ${SUBDIRS}; do cd $$x; ${MAKE} $@ || exit $$?; cd ..; done

install:
Expand Down
18 changes: 18 additions & 0 deletions tests/backoff/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
PROG= test_backoff
SRCS= test_backoff.c

CFLAGS+= -Wall -Wextra -std=c99 -D_DEFAULT_SOURCE
LDFLAGS+= -lm

all: ${PROG}

${PROG}: ${SRCS}
${CC} ${CFLAGS} -o ${PROG} ${SRCS} ${LDFLAGS}

test: ${PROG}
@echo "Run as root: sudo ./${PROG} [-b path-to-dhcpcd]"

clean:
rm -f ${PROG}

distclean: clean
Loading