From 0dbffd51de0f301bb40b59772337f8be99c6670b Mon Sep 17 00:00:00 2001 From: mahmood Date: Thu, 4 Jun 2026 15:05:57 +0100 Subject: [PATCH] fix(hk): guard data_len underflow in hk_param_sniffer on short sport-13 packets hk_param_sniffer() treats any packet whose CSP source port is 13 as an HK param stream and computes the payload length as: size_t data_len = packet->length - 5 - ((flags & CSP_FRDP) ? 5 : 0); data_len is unsigned, so any packet on sport 13 shorter than that 5/10-byte overhead -- e.g. a short RDP control/ack frame, or any other service that shares the port -- underflows data_len to ~SIZE_MAX. param_queue_init() and the mpack reader are then pointed at buffer..buffer+SIZE_MAX and walk off the end of the packet: an out-of-bounds read that segfaults the param sniffer under `prometheus start`. Guard the subtraction: a packet too short to carry a param payload is skipped. Adds tests/hk_sniffer_host.c (fork-isolated) which feeds the real hk_param_sniffer() short sport-13 packets and asserts they are skipped; fails without the guard, passes with it. Wired as hk_sniffer_tests in tests/meson.build. README gains a Testing section. --- README.md | 21 +++++++++ src/hk_param_sniffer.c | 10 ++++- tests/hk_sniffer_host.c | 95 +++++++++++++++++++++++++++++++++++++++++ tests/meson.build | 10 +++++ 4 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/hk_sniffer_host.c diff --git a/README.md b/README.md index bf7413cf..022f4fcb 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,27 @@ wsl --install After a reboot, you can then start the application WSL to get a virtual Ubuntu environment and follow the guidelines for installing CSH in Linux as above. Windows by default does not forward USB devices to WSL entities. To enable USB forwarding, follow the guide in https://learn.microsoft.com/en-us/windows/wsl/connect-usb. +## Testing + +Host tests live in `tests/` and run through meson: + +``` +meson test -C builddir # whole suite +meson test -C builddir hk_sniffer_tests # one test +``` + +`hk_sniffer_tests` is a regression for a `size_t` underflow in `hk_param_sniffer()`: +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 +--reconfigure` fails with `FileNotFoundError` on a stale `meson-private` path. +Recreate the build dir instead of reconfiguring: `rm -rf builddir && ./configure`. + + ## Extension support src/slash_apm.c defines a command, apm load, for loading a shared library as an APM, and a command, ap, info, for listing loaded APMs. diff --git a/src/hk_param_sniffer.c b/src/hk_param_sniffer.c index 846fc3e8..09b944b8 100644 --- a/src/hk_param_sniffer.c +++ b/src/hk_param_sniffer.c @@ -212,7 +212,15 @@ 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 data_len = packet->length - header_size - ((packet->id.flags & CSP_FRDP) ? 5 : 0); + size_t overhead = header_size + ((packet->id.flags & CSP_FRDP) ? 5 : 0); + /* 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; + } + size_t data_len = packet->length - overhead; param_queue_t queue; param_queue_init(&queue, &packet->data[header_size], data_len, data_len, PARAM_QUEUE_TYPE_SET, 2); queue.last_node = packet->id.src; diff --git a/tests/hk_sniffer_host.c b/tests/hk_sniffer_host.c new file mode 100644 index 00000000..e78c9916 --- /dev/null +++ b/tests/hk_sniffer_host.c @@ -0,0 +1,95 @@ +/* + * 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 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. + * + * This calls the REAL hk_param_sniffer() (compiled from ../src/hk_param_sniffer.c) on + * crafted short sport-13 packets, inside a forked child so a crash is *detected* rather + * than killing the test. Child dies by signal (SIGSEGV / SIGALRM hang) => bug present; + * child returns cleanly => the length guard is in place and working. + */ +#include +#include +#include +#include +#include +#include + +#include +#include "param_sniffer.h" /* real prototypes for the two stubs below */ +#include "hk_param_sniffer.h" + +/* Stubs for the two param_sniffer.c symbols hk_param_sniffer references. Neither is on + * the crash path. The real param_sniffer_crc is a no-op unless CSP_FCRC32 is set (our + * packets don't set it), so returning 0 is faithful to production behaviour. */ +int param_sniffer_crc(csp_packet_t *packet) { (void)packet; return 0; } +int param_sniffer_log(void *ctx, param_queue_t *queue, param_t *param, int offset, + void *reader, csp_timestamp_t *timestamp) { + (void)ctx; (void)queue; (void)param; (void)offset; (void)reader; (void)timestamp; + return 0; +} + +/* Run hk_param_sniffer(packet) in a child. A short non-param packet must be SKIPPED + * (return false). The buggy code instead enters the underflowed read loop and either + * crashes/hangs or, if it survives the walk, returns true having "processed" garbage. + * So: child crash/hang OR return==true => bug; return==false => correctly skipped. + * Returns 1 if buggy, 0 if correctly skipped. */ +static int run_child(const char *label, csp_packet_t *packet) { + printf("%-58s", label); + fflush(stdout); + pid_t pid = fork(); + if (pid == 0) { + freopen("/dev/null", "w", stdout); /* silence the per-iteration HK warnings */ + alarm(5); /* backstop an infinite read loop */ + _exit(hk_param_sniffer(packet) ? 2 : 0); /* 2 = processed (bug), 0 = skipped */ + } + int status = 0; + waitpid(pid, &status, 0); + if (WIFSIGNALED(status)) { + int s = WTERMSIG(status); + printf("BUG: crash (signal %d, %s)\n", s, + s == SIGSEGV ? "SIGSEGV" : s == SIGALRM ? "hang/SIGALRM" : "other"); + return 1; + } + if (WEXITSTATUS(status) != 0) { + printf("BUG: processed a short non-param packet (returned true)\n"); + return 1; + } + printf("skipped (returned false) -- correct\n"); + return 0; +} + +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 = 42; /* arbitrary non-zero source node */ + p->id.flags = flags; + p->length = length; + return p; +} + +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. 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. */ + bug |= run_child("non-13 sport, RDP (len=8, must be skipped):", make_pkt(40, CSP_FRDP, 8)); + + if (bug) { + printf("\nhk_sniffer_host: FAIL -- hk_param_sniffer crashed/hung on a short sport-13 packet.\n"); + printf("This is the data_len size_t underflow at hk_param_sniffer.c:215.\n"); + return 1; + } + printf("\nhk_sniffer_host: PASS -- short sport-13 packets are skipped, no underflow walk-off.\n"); + return 0; +} diff --git a/tests/meson.build b/tests/meson.build index 07247b15..63091644 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -36,3 +36,13 @@ set_array_tests = executable( dependencies: [csp_dep, slash_dep, param_dep, utils_lib_dep], ) test('set_array_tests', set_array_tests) + +# Regression for the hk_param_sniffer() data_len underflow segfault (short sport-13 +# DIPP RPC replies). Compiles the real hk_param_sniffer.c; crash is caught via fork(). +hk_sniffer_tests = executable( + 'hk_sniffer_tests', + sources: ['hk_sniffer_host.c', '../src/hk_param_sniffer.c'], + include_directories: ['../src'], + dependencies: [csp_dep, slash_dep, param_dep, utils_lib_dep], +) +test('hk_sniffer_tests', hk_sniffer_tests)