From 38c9b1e0e0e35b333cea126a348adc8189269b8d Mon Sep 17 00:00:00 2001 From: Samaresh Kumar Singh Date: Sat, 16 May 2026 19:01:26 -0500 Subject: [PATCH] defrag: avoid unsigned underflow in RB_NFIND search key The seek into the fragment tree uses frag_offset - 1 as the RB_NFIND key. frag_offset is uint16_t, so the subtraction promotes to int, and for the first fragment of a datagram (offset 0) the -1 narrows back to 65535 when it is stored in the key. -fsanitize=implicit-conversion flags this. The - 1 has to stay. DefragRbFragCompare never returns 0, so RB_NFIND is strictly greater rather than inclusive. With key = frag_offset it returns the fragment after an existing node at the same offset and leaves prev on that node, so the preceding fragment, which may overlap from the left, is skipped. With key = frag_offset - 1 the same-offset node is included and prev ends up on the real predecessor. So guard only the offset 0 case: .offset = frag_offset ? frag_offset - 1 : 0, For frag_offset == 0 the seek starts at RB_MIN either way, so this keeps the current behaviour and drops the wrap to 65535. Add two unit tests. DefragOutOfOrderFirstFragmentTest covers offset 0 arriving into a populated tree. DefragSameOffsetPrecedingOverlapTest pins down the same-offset plus left-overlap case that tells frag_offset - 1 and frag_offset apart. Ticket: 8232 --- src/defrag.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/src/defrag.c b/src/defrag.c index de1d884ac7..d64a4e9cbb 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -651,7 +651,7 @@ DefragInsertFrag(ThreadVars *tv, DecodeThreadVars *dtv, DefragTracker *tracker, if (!RB_EMPTY(&tracker->fragment_tree)) { Frag key = { - .offset = frag_offset - 1, + .offset = frag_offset ? frag_offset - 1 : 0, }; next = RB_NFIND(IP_FRAGMENTS, &tracker->fragment_tree, &key); if (next == NULL) { @@ -3143,6 +3143,117 @@ static int DefragBsdMissingFragmentIpv6Test(void) PASS; } +/** + * \test Reassemble a datagram where the fragment at offset 0 comes in last, + * with the other fragments already in the tree. This is the RB_NFIND + * search key case from ticket 8232. + */ +static int DefragOutOfOrderFirstFragmentTest(void) +{ + Packet *p1 = NULL, *p2 = NULL, *p3 = NULL, *p4 = NULL; + Packet *reassembled = NULL; + int id = 8232; + + DefragInit(); + default_policy = DEFRAG_POLICY_BSD; + + /* four fragments, no overlaps, offset 0 delivered last */ + p1 = BuildIpv4TestPacket(IPPROTO_ICMP, id, 24 >> 3, 0, 'D', 8); + FAIL_IF_NULL(p1); + p2 = BuildIpv4TestPacket(IPPROTO_ICMP, id, 8 >> 3, 1, 'B', 8); + FAIL_IF_NULL(p2); + p3 = BuildIpv4TestPacket(IPPROTO_ICMP, id, 16 >> 3, 1, 'C', 8); + FAIL_IF_NULL(p3); + p4 = BuildIpv4TestPacket(IPPROTO_ICMP, id, 0, 1, 'A', 8); + FAIL_IF_NULL(p4); + + FAIL_IF(Defrag(&test_tv, &test_dtv, p1) != NULL); + FAIL_IF(Defrag(&test_tv, &test_dtv, p2) != NULL); + FAIL_IF(Defrag(&test_tv, &test_dtv, p3) != NULL); + + reassembled = Defrag(&test_tv, &test_dtv, p4); + FAIL_IF_NULL(reassembled); + + FAIL_IF(IPV4_GET_RAW_HLEN(PacketGetIPv4(reassembled)) != 20); + FAIL_IF(IPV4_GET_RAW_IPLEN(PacketGetIPv4(reassembled)) != 20 + 32); + + for (int i = 20 + 0; i < 20 + 8; i++) + FAIL_IF(GET_PKT_DATA(reassembled)[i] != 'A'); + for (int i = 20 + 8; i < 20 + 16; i++) + FAIL_IF(GET_PKT_DATA(reassembled)[i] != 'B'); + for (int i = 20 + 16; i < 20 + 24; i++) + FAIL_IF(GET_PKT_DATA(reassembled)[i] != 'C'); + for (int i = 20 + 24; i < 20 + 32; i++) + FAIL_IF(GET_PKT_DATA(reassembled)[i] != 'D'); + + PacketFree(p1); + PacketFree(p2); + PacketFree(p3); + PacketFree(p4); + PacketFree(reassembled); + + DefragDestroy(); + PASS; +} + +/** + * \test A fragment arrives at an offset that is already in the tree, with an + * earlier fragment overlapping it from the left. Keeps the frag_offset - 1 + * search key honest: with frag_offset the seek skips the overlapping + * fragment and 16..24 comes out as 'B' instead of 'D'. + */ +static int DefragSameOffsetPrecedingOverlapTest(void) +{ + Packet *packets[5] = { NULL }; + Packet *reassembled = NULL; + int id = 8232; + + DefragInit(); + default_policy = DEFRAG_POLICY_BSD; + + /* B 8..24 goes in first so it keeps offset 8. A only sets its ltrim later, + * otherwise BSD would bake the trim into the stored offset. */ + FAIL_IF_NOT(BuildIpv4TestPacketWithContent( + &packets[0], IPPROTO_ICMP, id, 8 >> 3, 1, (uint8_t *)"BBBBBBBBBBBBBBBB", 16)); + /* A 0..16, overlaps B and D from the left */ + FAIL_IF_NOT(BuildIpv4TestPacketWithContent( + &packets[1], IPPROTO_ICMP, id, 0, 1, (uint8_t *)"AAAAAAAAAAAAAAAA", 16)); + /* C 24..32, the node RB_NFIND(8) lands on */ + FAIL_IF_NOT(BuildIpv4TestPacketWithContent( + &packets[2], IPPROTO_ICMP, id, 24 >> 3, 1, (uint8_t *)"CCCCCCCC", 8)); + /* D 8..24, same offset as B, has to be reconciled against A */ + FAIL_IF_NOT(BuildIpv4TestPacketWithContent( + &packets[3], IPPROTO_ICMP, id, 8 >> 3, 1, (uint8_t *)"DDDDDDDDDDDDDDDD", 16)); + /* E 32..40, last fragment, kicks off reassembly */ + FAIL_IF_NOT(BuildIpv4TestPacketWithContent( + &packets[4], IPPROTO_ICMP, id, 32 >> 3, 0, (uint8_t *)"EEEEEEEE", 8)); + + for (int i = 0; i < 4; i++) + FAIL_IF(Defrag(&test_tv, &test_dtv, packets[i]) != NULL); + reassembled = Defrag(&test_tv, &test_dtv, packets[4]); + FAIL_IF_NULL(reassembled); + + FAIL_IF(IPV4_GET_RAW_IPLEN(PacketGetIPv4(reassembled)) != 20 + 40); + + // clang-format off + const uint8_t expected[] = { + 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', + 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', + 'D', 'D', 'D', 'D', 'D', 'D', 'D', 'D', + 'C', 'C', 'C', 'C', 'C', 'C', 'C', 'C', + 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', + }; + // clang-format on + FAIL_IF(memcmp(expected, GET_PKT_DATA(reassembled) + 20, sizeof(expected)) != 0); + + for (int i = 0; i < 5; i++) + PacketFree(packets[i]); + PacketFree(reassembled); + + DefragDestroy(); + PASS; +} + #endif /* UNITTESTS */ void DefragRegisterTests(void) @@ -3195,5 +3306,7 @@ void DefragRegisterTests(void) DefragBsdSubsequentOverlapsStartOfOriginalIpv6Test_2); UtRegisterTest("DefragBsdMissingFragmentIpv4Test", DefragBsdMissingFragmentIpv4Test); UtRegisterTest("DefragBsdMissingFragmentIpv6Test", DefragBsdMissingFragmentIpv6Test); + UtRegisterTest("DefragOutOfOrderFirstFragmentTest", DefragOutOfOrderFirstFragmentTest); + UtRegisterTest("DefragSameOffsetPrecedingOverlapTest", DefragSameOffsetPrecedingOverlapTest); #endif /* UNITTESTS */ }