feat(kernel): validate DHCP messages arriving on the client port - #185
Merged
Conversation
Audit of `dhcp.rs` ahead of wiring it into the UDP receive path, plus the wiring, in one PR per the "one protocol per PR" convention. Audit result: no overflow/underflow/out-of-bounds/unbounded-loop defect. The options TLV walker and the overload (`file`/`sname`) regions are all saturating-arithmetic and length-bounded. Two validation-policy gaps remained, and the client was not reachable from the receive path at all. Fixes: - `DhcpClient::process_response` now validates the BOOTP `htype`/`hlen` fields. Those fields declare the link-layer address model that determines how `chaddr` is read; RFC 2131 §2 fixes them at 1/6 for Ethernet, and any other value means the hardware address is not the 6-byte MAC this client binds the lease to. Previously they were accepted unchecked. - New `dhcp::validate_message`, a framing-only admission gate: fixed-header length plus magic cookie, a real op (either direction), the Ethernet htype/hlen invariant, the cookie, and an options field that is both bounded and terminated by `END` (RFC 2131 §4.1 requires END as the last option). The shared walker now reports whether it saw `END`; the lenient public `parse_dhcp_options` keeps tolerating a missing END, while the strict receive-path gate requires it. Wiring: `handle_udp_packet` keeps the UDP header and, for server-to-client traffic (`src_port == 67 && dst_port == 68`), validates the payload after the checksum. DHCP is directional, so the exact port pair — not `||` as with the shared DNS port — is the correct client-inbound predicate. Malformed frames drop silently via the existing `Ok(0)` convention. No `DhcpClient` state, lease, transaction-ID correlation, or query transmission is introduced; that stateful integration needs a CSPRNG (which `random.rs` is explicitly not) and an interface/route model, so it is a separate PR. Tests: RED-first — `rejects_non_ethernet_htype` and `rejects_wrong_hlen` failed before the fix and pass after; `accepts_valid_ethernet_offer` guards the legitimate path. `validate_message` accepts a real discover/offer and rejects truncated header, bad cookie, non-Ethernet htype, and a missing END. Net-layer: a valid OFFER on 67→68 is consumed, a bad-cookie datagram is dropped, and DHCP-shaped bytes on non-DHCP ports bypass the gate entirely (guards the `&&` predicate). The pre-existing `test_process_response_nak` used an incomplete fixture (htype/hlen unset); the fixture is corrected to a well-formed frame, not deleted. Suite: 4619 passed, 0 skipped. QEMU boot passes on x86_64, aarch64, riscv64.
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.
Audit of
crates/kernel/src/dhcp.rsplus wiring into the UDP receive path, per thecrates/kernel/AGENTS.md"audit then wire one protocol per PR" rule.Audit result
No overflow/underflow/out-of-bounds/unbounded-loop defect. The options TLV walker and the RFC 2132 §9.3 overload (
file/sname) regions are all saturating-arithmetic and length-bounded, withEND/PADhandled and non-recursive overload. Two validation-policy gaps remained, and the client was unreachable from the receive path.Fix 1 —
htype/hlenvalidation inprocess_responseThe BOOTP
htype/hlenfields declare the link-layer address model that determines howchaddris read. RFC 2131 §2 fixes them at 1/6 for Ethernet; any other value means the hardware address is not the 6-byte MAC this client binds the lease to. They were accepted unchecked. TDD:rejects_non_ethernet_htype/rejects_wrong_hlenfailed before, pass after;accepts_valid_ethernet_offerguards the legit path.Fix 2 —
dhcp::validate_messageframing gateNew framing-only validator: fixed-header length + magic cookie, real op (either direction), Ethernet htype/hlen, cookie, and an options field that is bounded and terminated by
END(RFC 2131 §4.1). The shared walker now reports whether it sawEND; the lenient publicparse_dhcp_optionskeeps tolerating a missing END while the strict gate requires it.Wiring
handle_udp_packetvalidates the payload after checksum forsrc_port == 67 && dst_port == 68. DHCP is directional, so the exact pair (not||as with shared DNS port 53) is the correct client-inbound predicate. Malformed frames drop via the existingOk(0)convention. NoDhcpClientstate / lease / xid-correlation / query TX — that needs a CSPRNG (random.rsis explicitly not one) and an interface/route model, so it is a separate PR. Note: broadcast offers are still blocked byhandle_ipv4'sdst == local_ip; this PR is parser wiring, not a live exchange.Tests
validate_message: discover/offer accepted; truncated header, bad cookie, non-Ethernet htype, missing END rejected.&&).test_process_response_nakhad an incomplete fixture (htype/hlen unset); corrected, not deleted.Verification
cargo fmt --all -- --check·cargo clippy --workspace -- -D warnings·cargo build --workspacecargo build -p oncrix-kernel --bin oncrix-kernel --target aarch64-unknown-none/riscv64gc-unknown-none-elfbash scripts/run-tests.sh— 4619 passed, 0 skipped (kernel 367 → 385)bash scripts/run-qemu-test.sh— boots clean on all three architectures