diff --git a/README.md b/README.md index 33bce1e3..022f4fcb 100644 --- a/README.md +++ b/README.md @@ -85,11 +85,10 @@ meson test -C builddir hk_sniffer_tests # one test ``` `hk_sniffer_tests` is a regression for a `size_t` underflow in `hk_param_sniffer()`: -a short packet on source port 13 (e.g. a DIPP `ring_size`/`observation_meta` RDP -reply, which shares sport 13 with HK params) underflowed the param payload length -and walked the mpack reader off the buffer, segfaulting the param sniffer under -`prometheus start`. The test feeds the real function such packets and asserts they -are skipped. +any packet on source port 13 shorter than the param header overhead (e.g. a short +RDP control/ack frame) underflowed the payload length and walked the mpack reader +off the buffer, segfaulting the param sniffer under `prometheus start`. The test +feeds the real function such packets and asserts they are skipped. Note: meson build directories are not relocatable. If the tree was first configured under a different absolute path (a moved or renamed checkout), `meson setup diff --git a/src/hk_param_sniffer.c b/src/hk_param_sniffer.c index be318e13..09b944b8 100644 --- a/src/hk_param_sniffer.c +++ b/src/hk_param_sniffer.c @@ -213,10 +213,10 @@ bool hk_param_sniffer(csp_packet_t * packet) { /* Protocol has a header size of 5, and RDP adds 5 bytes to the end of the packet if activated */ size_t header_size = 5; size_t overhead = header_size + ((packet->id.flags & CSP_FRDP) ? 5 : 0); - /* Guard the unsigned subtraction. Other traffic also uses sport 13 (e.g. the DIPP - * ring_size / observation_meta RDP replies, which are short). Without this, a packet - * shorter than the overhead underflows data_len to ~SIZE_MAX and the mpack reader - * below walks off the buffer -> segfault. Too short to carry a param payload => skip. */ + /* Guard the unsigned subtraction. Any packet on sport 13 shorter than this overhead + * (e.g. a short RDP control/ack frame, or another service that happens to share the + * port) would underflow data_len to ~SIZE_MAX and make the mpack reader below walk + * off the buffer -> segfault. Too short to carry a param payload => skip. */ if (packet->length < overhead) { return false; } diff --git a/tests/hk_sniffer_host.c b/tests/hk_sniffer_host.c index 8677a214..e78c9916 100644 --- a/tests/hk_sniffer_host.c +++ b/tests/hk_sniffer_host.c @@ -1,9 +1,9 @@ /* * hk_sniffer_host.c - reproduce (and verify the fix for) the hk_param_sniffer() crash. * - * Bug: hk_param_sniffer() accepts any packet whose CSP *source* port is 13. The DIPP - * ring_size / observation_meta RPC replies also use sport 13, and they are short RDP - * frames. The length math + * Bug: hk_param_sniffer() accepts any packet whose CSP *source* port is 13 and treats + * it as an HK param stream. Short packets on sport 13 (e.g. RDP control/ack frames, or + * any other service that shares the port) carry no param payload. The length math * size_t data_len = packet->length - 5 - ((flags & CSP_FRDP) ? 5 : 0); * is UNSIGNED, so a short packet underflows data_len to ~SIZE_MAX. The mpack reader is * then pointed at buffer..buffer+SIZE_MAX and walks off the packet -> SIGSEGV. @@ -68,7 +68,7 @@ static csp_packet_t *make_pkt(uint16_t sport, uint8_t flags, uint16_t length) { csp_packet_t *p = calloc(1, sizeof(*p)); /* data[CSP_BUFFER_SIZE] is inline + zeroed */ p->id.sport = sport; p->id.dport = 14; /* sniffer keys on sport, not dport */ - p->id.src = 5423; /* a DIPP node, as in the live capture */ + p->id.src = 42; /* arbitrary non-zero source node */ p->id.flags = flags; p->length = length; return p; @@ -78,8 +78,8 @@ int main(void) { int bug = 0; printf("hk_sniffer_host: feeding short sport-13 packets to the real hk_param_sniffer()\n\n"); - /* length 8, RDP: 8 - 5 - 5 underflows. This is the DIPP ring_size reply shape. */ - bug |= run_child("short RDP packet, sport 13 (len=8, the DIPP reply):", make_pkt(13, CSP_FRDP, 8)); + /* length 8, RDP: 8 - 5 - 5 underflows. Typical short RDP control/ack frame shape. */ + bug |= run_child("short RDP packet, sport 13 (len=8):", make_pkt(13, CSP_FRDP, 8)); /* length 3, non-RDP: 3 - 5 underflows. */ bug |= run_child("short non-RDP packet, sport 13 (len=3):", make_pkt(13, 0, 3)); /* control: not sport 13 -> must be skipped immediately, never crashes. */