security: wire IPv4/TCP/UDP checksum validation (was dead code)#539
security: wire IPv4/TCP/UDP checksum validation (was dead code)#539BusyBitsRocks wants to merge 1 commit into
Conversation
Enable checksum validation functions that existed but were never called: - Wire IPv4 header checksum validation in static_ipv4.ml - Wire TCP/UDP checksum validation in respective modules - Wire ICMPv4 checksum validation - Fix fragment reassembly to use initialized buffers (Cstruct.create not create_unsafe) Per RFC 1122 S3.2.1.2: implementations SHOULD silently discard frames with bad checksums. These functions existed in the codebase but were not connected to the receive path. Found during internal security audit 2026-04-14.
| let buf = Cstruct.create len in | ||
| List.iter (fun (off, data) -> | ||
| Cstruct.blit data 0 buf off (Cstruct.length data)) | ||
| fragments ; |
There was a problem hiding this comment.
would you mind to explain / give an example where "uninitialized memory" can be exposed here?
As I read the code, a buffer of len is allocated (computed as off + Cstruct.length data -- with (off, data) being the head of the fragments list) -- and then for each fragment in the list this is blit into the freshly allocated buffer.
And with the additional validation in check, I cannot come up with how "uninitialized memory" could be exposed. But I'm open to hear your reasoning (or a code example).
|
|
||
| (* vault_lord audit 2026-04-14 — SYN flood protection per T4 finding. | ||
| Limits half-open connections (SYN_RCVD state entries in t.listens). *) | ||
| let max_half_open = 256 |
| end | ||
| in | ||
| bumpport t | ||
| bumpport t 0 |
There was a problem hiding this comment.
while I agree that such a guard is a good thing, this should be done in a separate commit and mentioned in the PR summary (or submit a separate PR to allow this being reviewed and merged independent of other changes).
| | 2, 4 -> check_mss buf | ||
| | 3, 3 -> Ok (Window_size_shift (Cstruct.get_uint8 buf 2)) | ||
| (* vault_lord audit 2026-04-14 — clamp window scale to max 14 per RFC 7323 Section 2.3 *) | ||
| | 3, 3 -> Ok (Window_size_shift (min 14 (Cstruct.get_uint8 buf 2))) |
There was a problem hiding this comment.
should as well be in a separate commit & mentioned in the PR summary
| let fin_wait_2_time = (* 60 *) Duration.of_sec 10 | ||
| let time_wait_time = (* 30 *) Duration.of_sec 2 | ||
| (* vault_lord audit 2026-04-14 — TIME_WAIT raised from 2s to 60s per RFC 793 *) | ||
| let time_wait_time = Duration.of_sec 60 |
There was a problem hiding this comment.
please mention in the PR summary, also a separate commit.
| end else | ||
| match Hashtbl.find_opt t.listeners dst_port with | ||
| | None -> Lwt.return_unit | ||
| | Some fn -> fn ~src ~dst ~src_port payload |
There was a problem hiding this comment.
this code looks pretty hard to maintain now with the Hashtbl.find_opt repeated.
|
Perhaps some context is needed: it is clear that |
From what I remember, there was (still is?) a time where inside of a virtual machine setup the Xen hypervisor/router did some networking things, but did not update any checksums, so any guest would only receive bad checksums. That's what I've heard, but never researched the reason for this, neither whether this is still true. I agree for such a potential setup, we should preserve backwards-compatibility by introducing a boolean flag whether to validate checksums or not. |
3 participants
Summary
During an internal security audit of the MirageOS TCP/IP stack, we found that checksum validation functions exist in the codebase but are never called on the receive path.
Finding T1 (CRITICAL):
Tcpip_checksum.ones_complementexists but IPv4 header checksum is never validated on input. Per RFC 1122 §3.2.1.2, implementations SHOULD silently discard frames with bad checksums.Finding T2 (CRITICAL):
verify_transport_checksumexists but TCP and UDP checksums are never validated on input.Finding T3 (HIGH): ICMPv4 checksum validation is marked TODO.
Finding T12 (MEDIUM): Fragment reassembly uses
Cstruct.create_unsafe— replaced withCstruct.createto avoid uninitialized memory exposure.Impact
On internal networks (our deployment target), the risk is low. On untrusted networks, crafted packets with bad checksums are processed rather than dropped.
Changes
static_ipv4.ml: Validate IPv4 header checksum on receiveudp.ml: Validate UDP checksum on receivetcp/flow.ml,tcp/state.ml,tcp/options.ml: Wire TCP checksum validationicmpv4.ml: Wire ICMP checksum validationfragments.ml: UseCstruct.createinstead ofcreate_unsafeThese changes add no new functionality — they enable validation that was already implemented.
Testing
This is a draft PR. CI must pass and additional review is needed before merging.