Skip to content

rpcapd: fix uint16_t truncation of packet payload length#1685

Open
TristanInSec wants to merge 1 commit into
the-tcpdump-group:masterfrom
TristanInSec:fix/rpcapd-plen-truncation
Open

rpcapd: fix uint16_t truncation of packet payload length#1685
TristanInSec wants to merge 1 commit into
the-tcpdump-group:masterfrom
TristanInSec:fix/rpcapd-plen-truncation

Conversation

@TristanInSec
Copy link
Copy Markdown

The length passed to rpcap_createhdr() in daemon_thrdatamain() was
cast to uint16_t, truncating the plen field when caplen exceeds 65516.
This corrupts the rpcap protocol stream for any capture with large
packets (jumbo frames, TSO/GRO) and snaplen > 65516.

Fix: cast to uint32_t, matching the rpcap_createhdr parameter type.

The length argument to rpcap_createhdr() in daemon_thrdatamain() was
cast to uint16_t before being passed, even though the parameter type is
uint32_t.  This causes the plen field in the wire header to wrap when
caplen exceeds 65516 bytes (65536 minus sizeof(struct rpcap_pkthdr)).

The client then reads fewer bytes than the server sent, leaving the
remainder in the TCP receive buffer.  The next rpcap_recv() call
interprets stranded packet data as a new rpcap_header, permanently
desynchronizing the protocol stream.

This affects any capture session where snaplen > 65516 and the monitored
interface delivers packets that large (jumbo frames, TSO/GRO segments).
MAXIMUM_SNAPLEN is 262144, so the default snaplen of many applications
triggers this with large packets.

Fix by casting to uint32_t instead of uint16_t.
@guyharris
Copy link
Copy Markdown
Member

Yeah, that cast was completely wrong, given that the field is 32-bit in the protocol and that the argument is 32-bit, and it dates all the way back to at least WinPcap 4.1.3. Perhaps the original authors thought there was a 16-bit limit on packet lengths... which there never was.

The code also needs a check to make sure that the addition of the message header doesn't overflow 32 bits. Either that, or the daemon should refuse to allow setting the snapshot length large enough to allow a packet that would cause an overflow to pass through unsliced, and libpcap should ensure that the snapshot length always ensures slicing.

@TristanInSec
Copy link
Copy Markdown
Author

Good point about the 32-bit overflow. sizeof(rpcap_pkthdr) is 20, and MAXIMUM_SNAPLEN is 262144 (pcap-int.h:156), so the maximum sum is 262164 -- well within uint32_t range. But if MAXIMUM_SNAPLEN ever increased, or if a custom snaplen were allowed, it could wrap.

I can add an overflow guard in the same PR if you want. Something like:

uint32_t plen = (uint32_t)sizeof(struct rpcap_pkthdr) + pkt_header->caplen;
if (plen < pkt_header->caplen) {
    /* overflow */
    goto error;
}

Or if you prefer the snaplen enforcement approach I'm happy to go that route instead. Let me know which you'd rather see and I'll update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants