Repository: smoltcp-rs/smoltcp (language: rust)

Issue:
DHCPv4 panics when lease duration < renew duration
When processing an ACK in the DHCP socket, we calculate the renew duration (T1) and rebind duration (T2). These are provided as an option. Ideally, the server SHOULD provide both of them, with the following constraint: T1 < T2 < lease duration. When only T1 is provided, [we calculate T2 as T1 + (lease duration - T1) * 0.75](https://github.com/smoltcp-rs/smoltcp/blob/3a30d0c891f79c9f46dfcda6f5532fd370bd7d74/src/socket/dhcpv4.rs#L518-L525). However, this panics when the lease duration is smaller than T1. This is because the implementation of the subtraction of two durations uses `checked_sub` and `expect()`.

I'm not sure what the correct approach is to fix this, as I'm not familiar with DHCPv4. I think we should use default values for T1 and T2 when the provided values of T1 and T2 do not follow T1 < T2 < lease duration.

From [RFC 2131](https://datatracker.ietf.org/doc/html/rfc2131#section-4.4.5): 
> T1 MUST be earlier than T2, which, in turn, MUST be earlier than the time at
> which the client's lease will expire.

Credits to the people from Radically Open Security for finding this panic.


Discussion / hints:
Proof of concept from Radically Open Security to trigger the panic:

1. Run the `dhcp_client` example.
2. Python script that triggers the panic using scapy:
```python
from scapy.all import *
from scapy.base_classes import Net
dhcp_server = DHCP_am(iface='tap0', domain='example.com',
    pool=Net('192.168.69.0/24'),
    network='192.168.69.0/24',
    gw='192.168.69.254',
    renewal_time=9, lease_time=8)
dhcp_server()
```

Fix patch:
diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs
index 72980a946..7d8c920ad 100644
--- a/src/socket/dhcpv4.rs
+++ b/src/socket/dhcpv4.rs
@@ -502,6 +502,8 @@ impl<'a> Socket<'a> {
         // Times T1 and T2 are configurable by the server through
         // options. T1 defaults to (0.5 * duration_of_lease). T2
         // defaults to (0.875 * duration_of_lease).
+        // When receiving T1 and T2, they must be in the order:
+        // T1 < T2 < lease_duration
         let (renew_duration, rebind_duration) = match (
             dhcp_repr
                 .renew_duration
@@ -510,8 +512,11 @@ impl<'a> Socket<'a> {
                 .rebind_duration
                 .map(|d| Duration::from_secs(d as u64)),
         ) {
-            (Some(renew_duration), Some(rebind_duration)) => (renew_duration, rebind_duration),
-            (None, None) => (lease_duration / 2, lease_duration * 7 / 8),
+            (Some(renew_duration), Some(rebind_duration))
+                if renew_duration < rebind_duration && rebind_duration < lease_duration =>
+            {
+                (renew_duration, rebind_duration)
+            }
             // RFC 2131 does not say what to do if only one value is
             // provided, so:
 
@@ -519,7 +524,7 @@ impl<'a> Socket<'a> {
             // between T1 and the duration of the lease. If T1 is set to
             // the default (0.5 * duration_of_lease), then T2 will also
             // be set to the default (0.875 * duration_of_lease).
-            (Some(renew_duration), None) => (
+            (Some(renew_duration), None) if renew_duration < lease_duration => (
                 renew_duration,
                 renew_duration + (lease_duration - renew_duration) * 3 / 4,
             ),
@@ -527,9 +532,16 @@ impl<'a> Socket<'a> {
             // If only T2 is provided, then T1 will be set to be
             // whichever is smaller of the default (0.5 *
             // duration_of_lease) or T2.
-            (None, Some(rebind_duration)) => {
+            (None, Some(rebind_duration)) if rebind_duration < lease_duration => {
                 ((lease_duration / 2).min(rebind_duration), rebind_duration)
             }
+
+            // Use the defaults if the following order is not met:
+            // T1 < T2 < lease_duration
+            (_, _) => {
+                net_debug!("using default T1 and T2 values since the provided values are invalid");
+                (lease_duration / 2, lease_duration * 7 / 8)
+            }
         };
         let renew_at = now + renew_duration;
         let rebind_at = now + rebind_duration;


Test patch: