search

the-tcpdump-group / libpcap

the LIBpcap interface to various kernel packet capture mechanism
https://www.tcpdump.org/
Other
2.72k stars 855 forks source link

Fix out-of-bound pointer subtraction in BPF buffers #1339

Closed nathaniel-bennett closed 3 months ago

nathaniel-bennett commented 4 months ago

The current implementation for BPF uses the following to determine if a buffer has been fully read:

    p->cc = (int)(ep - bp);

(where bp and ep are char* pointers.)

As stated in the source code, ep is set based on read(), which may not return a value that's a multiple of the alignment value for BPF_WORDALIGN(). This means that when bp is incremented by caplen + hdrlen padded to word alignment, it may point beyond the end of the buffer, such that bp > ep. Since subtraction is only well-defined for pointers that are both within a contiguous memory region (up to 1 past the end of a buffer), this results in undefined behavior.

This pull request resolves these issues by limiting how much bp can be incremented by so that it never exceeds ep. This ensures the pointer subtraction is well-defined and preserves the behavior of p->cc being assigned to 0 in such cases.

guyharris commented 4 months ago

ep is set based on read(), which may not return a value that's a multiple of the alignment value for BPF_WORDALIGN()

If the return value of read() isn't a multiple of the alignment value, presumably that's because the last packet doesn't need to be padded to align the address of the next packet, because there is no next packet. And, if we're processing the last packet, there's no need to increment bp to point to the non-existent next packet; we should just say "OK, that's it, no more packets to process in this buffer.

That might be a bit clearer.

guyharris commented 4 months ago

The code has only one loop variable, bp; it doesn't use cc as a loop variable as well. Perhaps the goal, at the time, was to micro-optimize the loop, but what might have been a useful optimization some time between 1988 and 1992 might not be one now.

Having cc be a loop variable might be clearer, and avoid this issue.

guyharris commented 4 months ago

And pcap-bpf.c isn't the only module that has this issue. See #1343 for a pull request that 1) uses cc as a loop variable and 2) fixes the other modules where it's an issue.

guyharris commented 4 months ago

Having cc be a loop variable might be clearer, and avoid this issue.

It might be clearer, but it's also a bigger change, so I changed it to an approach more like this change, clamping the amount by which we increment bp (in all modules where it's a buffer pointer) so that it never goes past ep.

guyharris commented 3 months ago

Included in now-merged #1343.