Repository navigation
Fix two old CVE reports + various fixes from static analysis - #82
Merged
Merged
Conversation
pico_mdns_recv() ignored buflen and the DNS header has no length field, so the question/answer walkers trusted qdcount/ancount/ nscount and read past the packet end. Thread the buffer end through the walkers and bound every name walk and rdata length. Verification: make clean && make units UNITS=1 + ./test/units.sh green (24 binaries, 0 failures); new regression test fails on the unpatched code (truncated name accepted).
pico_dns_decompress_name() used a 0xC0 compression pointer as an unbounded offset (pico_dns_packet has no length field), allowing an OOB read on a crafted packet. Thread packet_len through the decompress helpers and bound every label walk and pointer target to the packet end.
Bound DNS name decompression callers and the seek_suffix walks to the packet end, guard fixed-size DHCP option reads and NUL-terminate the hostname/domain options, free and cap DHCP server negotiations, and fix the SNTP NULL-sock deref plus the IPv4 DNS sync no-op that leaked its cookie. Verification: make units UNITS=1 clean (no new warnings); ./test/units.sh all suites pass; uncrustify --check -c uncrustify.cfg clean; cstyle-check no new violations vs HEAD.
get_neigh_option() copied len*8 bytes (len is an attacker-controlled uint8_t, up to 2040) into a fixed-size stack struct with only a len<=0 guard, so a single unauthenticated ICMPv6 ND packet with an oversized option smashed the stack. Pass the destination capacity in and reject the option when len*8 exceeds either the destination buffer or the remaining option area, per RFC 4861 (silently drop malformed options).
defrag_new()/defrag_update() wrote the fragment payload at the attacker-controlled offset into a buffer allocated at exactly dgram_size with no off+len check, so a FRAG1/FRAGN with a small dgram_size and a full-size payload (or a large offset) overflowed the heap. Drop the fragment when off+len exceeds the reassembled datagram size (RFC 4944).
auth_rsp() sized the MD5 input buffer from the CHAP header length (ch->len) but copied *recvd_challenge_len bytes from the wire - an independent attacker field - allowing a content-controlled heap overflow of up to ~251 bytes. Cap the copy to the remaining buffer space so an oversized challenge is truncated instead of smashing the heap.
pico_ipv6_check_headers_sequence walked the extension header chain without validating that each header lies within the packet, so a crafted nexthdr chain could read out of bounds. Add an end offset (base header + payload length) and reject headers that run past it. pico_ipv6_process_hopbyhop advanced the option pointer by an attacker-controlled length with no bounds check (the destination options path already had it). Add the same optlen > len guard to the PADN, router-alert and unknown-option cases.
tcp_parse_options passed the SACK option's declared length to tcp_rcv_sack without bounding it to the bytes actually left in the options region, so a short packet with a large declared SACK length read out of bounds. Cap the read to the remaining options bytes; a truncated block is then rejected by the existing len%8 check.
pico_iphc_reassemble computed payload_len as f->len - handled_len and used it as a buf_move length. A malformed frame whose compressed length exceeds the actual frame made payload_len negative, which cast to a huge size_t and overflowed the reassembly buffer. Reject such frames before the copy loop.
pico_802154_process_in copied the source/destination addresses and advanced net_hdr by the header length without checking the frame held that many bytes, so a short frame read out of bounds and underflowed the link-layer length. Validate the frame length against the minimum header and the computed header length before parsing, and discard malformed frames. pico_6lowpan_ll_process_in used the frame after a discard and let the header length underflow f->len; return immediately once an extension discards the frame.
frag_fill copied 'copy' bytes from the datagram, but the copy length is derived from the link-layer capacity, which can exceed the datagram size and read out of bounds. Cap the copy to the bytes actually left in the datagram before writing the fragment header.
olsr_recv advanced its parse offset by the attacker-controlled msg->size field with no validation: a zero size spun the parse loop forever and a size larger than the remaining bytes made recv_hello/recv_mid/ reconsider_topology read out of bounds. Stop parsing when a message size is smaller than the fixed header or runs past the end of the packet.
defrag_new allocates up to dgram_size (<=32KiB) per new fragment tag with no bound on concurrent entries, so a flood of distinct tags exhausts memory. Count LPReassemblyTree entries and evict the oldest (by timestamp) once PICO_6LOWPAN_MAX_REASSEMBLY is reached, mirroring the DHCP server negotiation cap.
pico_icmp6_update_pmtu read the embedded IPv6 header (dst, nxthdr) and pico_icmp6_ping_recv_reply read echo id/seq without checking that the received frame actually carries them, so a short/attacker-crafted frame read out of bounds. Guard both on f->transport_len (set by the IPv6 layer in the real path). Update the two PTB unit tests, which drove process_in directly, to set transport_len like the real path does.
pico_igmp_is_checksum_valid derived the payload length from the attacker-controlled IPv4 total length, truncated it to uint8_t, and checksummed that many bytes from transport_hdr with no check against the actual frame size. A short or crafted frame read out of bounds and a payload over 255 bytes checksummed the wrong length. Use a 16-bit length and reject the frame when the claimed payload does not fit.
pico_ipv4_nat_generate_tuple returns NULL when no free port is found after 32 retries (or on OOM / tree insert failure), but the TCP and UDP outbound paths dereferenced tuple->nat_addr unconditionally, crashing on port exhaustion. Return -1 when no tuple is available.
radiotest_poll read a radio-manager frame into a 128-byte stack buffer using the peer-supplied length byte unchecked (stack overflow), and then handed (ret_len - 2) to pico_6lowpan_stack_recv which underflows to a huge uint32_t when the frame is 2 or 3 bytes (OOB read). Reject frames longer than the buffer and require at least a frame + CRC + address.
uncrustify --check wanted the wrapped argument one space shallower.
There was a problem hiding this comment.
🟡 Changes recommended
Two parsing hardening changes still allow out-of-bounds reads due to ordering/length-bounding issues (802.15.4 header access and IPv6 ext-header end computation), and DHCP teardown risks disrupting other devices’ negotiations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens picoTCP-NG’s packet parsing against multiple out-of-bounds read/write conditions (including CVE-2020-24338/24339/24340-related DNS/mDNS cases) and addresses static-analysis findings by adding defensive bounds checks and resource caps, along with targeted unit-test regressions.
Changes:
- Add systematic length/containment checks to multiple protocol parsers (mDNS/DNS, IPv6 ext headers, ICMPv6, IGMP, TCP options, PPP CHAP, 6LoWPAN, 802.15.4, OLSR, NAT).
- Introduce resource caps/evictions to bound memory growth under flooding (6LoWPAN reassembly, DHCP server negotiations).
- Update and extend unit tests to cover newly rejected malformed inputs and updated APIs.
File summaries
| File | Description |
|---|---|
| test/unit/modunit_pico_sntp_client.c | Adjust SNTP test cookie initialization for new stack pointer usage. |
| test/unit/modunit_pico_mdns.c | Update mDNS unit tests for new end-pointer API; add truncated-packet regression tests. |
| test/unit/modunit_pico_ipv6_pmtu.c | Ensure unit tests set transport_len to reflect payload bounds. |
| test/unit/modunit_pico_ipv6_nd.c | Update ND option test calls for new buffer-size parameter. |
| test/unit/modunit_pico_dns_common.c | Expand DNS name decompression tests and adapt to packet-length-bounded API. |
| modules/pico_tcp.c | Cap SACK parsing to remaining TCP options space. |
| modules/pico_sntp_client.c | Decouple cookie stack from socket pointer; tighten DNS-start flow and cleanup. |
| modules/pico_olsr.c | Validate per-message size before parsing to avoid OOB reads. |
| modules/pico_nat.c | Guard NAT outbound tuple generation against NULL. |
| modules/pico_mdns.c | Add end-bounded parsing for questions/answers/known-answer suppression; validate header presence early. |
| modules/pico_ipv6.c | Add additional bounds checks for hop-by-hop options and extension-header sequencing. |
| modules/pico_ipv6_nd.c | Prevent ND option copy/parse from exceeding option area or destination buffers. |
| modules/pico_igmp.c | Bound IGMP checksum calculation to bytes actually received. |
| modules/pico_icmp6.c | Bound ICMPv6 echo-reply and PTB header reads to received payload size. |
| modules/pico_dns_common.h | Extend DNS decompression APIs to take explicit packet length. |
| modules/pico_dns_common.c | Implement packet-length-bounded DNS decompression and propagate new API through helpers. |
| modules/pico_dns_client.c | Add bounds checks while walking DNS answers; update CNAME decompression call. |
| modules/pico_dhcp_server.c | Cap concurrent negotiations; add teardown cleanup of negotiation entries. |
| modules/pico_dhcp_client.c | Validate DHCP option lengths before reading; ensure hostname/domain buffers are NUL-terminated. |
| modules/pico_dev_radiotest.c | Bound radiotest receive size and reject too-short frames. |
| modules/pico_dev_ppp.c | Cap CHAP challenge copy to allocated buffer size. |
| modules/pico_802154.c | Reject frames shorter than minimum header / computed header length. |
| modules/pico_6lowpan.c | Cap reassembly entries; cap fragment copies to datagram size; reject overlong IPHC reassembly. |
| modules/pico_6lowpan_ll.c | Return error on discard to stop further processing after freeing frame. |
| docs/security.md | Move CVE-2020-24340 to “Fixed in master” section. |
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
danielinux
changed the title
- 802.15.4: check min frame length before dereferencing hdr->fcf - IPv6: bound ext-header end to actually-received bytes (not just the claimed payload length) so a truncated packet cannot drive OOB reads - DHCP server destroy: free only this device's negotiations, not all on the stack - DHCP server: fix eviction comment (evicts lowest xid, not oldest)
Closed
Move the three DNS/mDNS CVEs from 'Known issues' to the fixed list; no known issues remain for v2.1.
These five memory-safety fixes shipped in PR #81; list them in the 2.2 fixed section alongside the three DNS/mDNS CVEs.
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
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.
3b300af Bound radiotest receive buffer and frame length
9367584 Guard NAT outbound tuple against NULL
e807c0a Bound IGMP checksum length to the received frame
83ffe86 Bound ICMPv6 PTB and echo-reply header reads
1582ea6 Cap concurrent 6LoWPAN reassembly entries
ebf3766 Validate OLSR per-message size before parsing
6940e82 Cap 6LoWPAN fragment copy to the datagram size
fe27cf6 Reject 802.15.4 frames shorter than their own header
15841ab Reject IPHC reassembly when compressed length exceeds the frame
170713f Cap TCP SACK option read to the remaining options length
bb33f7a Bound IPv6 extension header walk to the datagram length
b4dd932 Cap PPP CHAP challenge copy to the allocated buffer
e2d9845 Bound 6LoWPAN reassembly fragment copy to datagram size
304667e Bound IPv6 ND option copy to destination size and option area
b48544e Fix Skoll scan findings in DNS, DHCP and SNTP modules
0008a7c Bound DNS name decompression to packet length (CVE-2020-24338/24339)
9b0db90 Fix OOB read in mDNS response handling (CVE-2020-24340)