From: Jason Ish Date: Tue, 28 Nov 2023 18:35:26 +0000 (-0600) Subject: defrag: check next fragment for overlap before stopping re-assembly X-Git-Tag: suricata-8.0.0-beta1~1437 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d0fd0782505d837e691ceef1b801776f0db82726;p=thirdparty%2Fsuricata.git defrag: check next fragment for overlap before stopping re-assembly Instead of breaking the loop when the current fragment does not have any more fragments, set a flag and continue to the next fragment as the next fragment may have data that occurs before this fragment, but overlaps it. Then break if the next fragment does not overlap the previous. Bug: #6668 --- diff --git a/src/defrag.c b/src/defrag.c index 8634d14126..85bb33c94d 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -275,10 +275,20 @@ Defrag4Reassemble(ThreadVars *tv, DefragTracker *tracker, Packet *p) uint16_t hlen = 0; int ip_hdr_offset = 0; + /* Assume more frags. */ + uint16_t prev_offset = 0; + bool more_frags = 1; + RB_FOREACH(frag, IP_FRAGMENTS, &tracker->fragment_tree) { SCLogDebug("frag %p, data_len %u, offset %u, pcap_cnt %"PRIu64, frag, frag->data_len, frag->offset, frag->pcap_cnt); + /* Previous fragment has no more fragments, and this packet + * doesn't overlap. We're done. */ + if (!more_frags && frag->offset > prev_offset) { + break; + } + if (frag->skip) continue; if (frag->ltrim >= frag->data_len) @@ -319,9 +329,16 @@ Defrag4Reassemble(ThreadVars *tv, DefragTracker *tracker, Packet *p) fragmentable_len = frag->offset + frag->data_len; } - if (!frag->more_frags) { - break; - } + /* Even if this fragment is flagged as having no more + * fragments, still continue. The next fragment may have the + * same offset with data that is preferred. + * + * For example, DefragBsdFragmentAfterNoMfIpv{4,6}Test + * + * This is due to not all fragments being completely trimmed, + * but relying on the copy ordering. */ + more_frags = frag->more_frags; + prev_offset = frag->offset; } SCLogDebug("ip_hdr_offset %u, hlen %" PRIu16 ", fragmentable_len %" PRIu16, ip_hdr_offset, hlen, @@ -418,7 +435,15 @@ Defrag6Reassemble(ThreadVars *tv, DefragTracker *tracker, Packet *p) uint16_t fragmentable_len = 0; int ip_hdr_offset = 0; uint8_t next_hdr = 0; + + /* Assume more frags. */ + uint16_t prev_offset = 0; + bool more_frags = 1; + RB_FOREACH(frag, IP_FRAGMENTS, &tracker->fragment_tree) { + if (!more_frags && frag->offset > prev_offset) { + break; + } if (frag->skip) continue; if (frag->data_len - frag->ltrim <= 0) @@ -463,9 +488,16 @@ Defrag6Reassemble(ThreadVars *tv, DefragTracker *tracker, Packet *p) fragmentable_len = frag->offset + frag->data_len; } - if (!frag->more_frags) { - break; - } + /* Even if this fragment is flagged as having no more + * fragments, still continue. The next fragment may have the + * same offset with data that is preferred. + * + * For example, DefragBsdFragmentAfterNoMfIpv{4,6}Test + * + * This is due to not all fragments being completely trimmed, + * but relying on the copy ordering. */ + more_frags = frag->more_frags; + prev_offset = frag->offset; } rp->ip6h = (IPV6Hdr *)(GET_PKT_DATA(rp) + ip_hdr_offset); @@ -2423,6 +2455,10 @@ static int DefragMfIpv4Test(void) * fragments should be in the re-assembled packet. */ FAIL_IF(IPV4_GET_IPLEN(p) != 36); + /* Verify the payload of the IPv4 packet. */ + uint8_t expected_payload[] = "AAAAAAAABBBBBBBB"; + FAIL_IF(memcmp(GET_PKT_DATA(p) + sizeof(IPV4Hdr), expected_payload, sizeof(expected_payload))); + SCFree(p1); SCFree(p2); SCFree(p3); @@ -2470,6 +2506,10 @@ static int DefragMfIpv6Test(void) * of 2 fragments, so 16. */ FAIL_IF(IPV6_GET_PLEN(p) != 16); + /* Verify the payload of the IPv4 packet. */ + uint8_t expected_payload[] = "AAAAAAAABBBBBBBB"; + FAIL_IF(memcmp(GET_PKT_DATA(p) + sizeof(IPV6Hdr), expected_payload, sizeof(expected_payload))); + SCFree(p1); SCFree(p2); SCFree(p3); @@ -2571,6 +2611,96 @@ static int DefragTestJeremyLinux(void) PASS; } +static int DefragBsdFragmentAfterNoMfIpv4Test(void) +{ + DefragInit(); + default_policy = DEFRAG_POLICY_BSD; + Packet *packets[4]; + + packets[0] = BuildIpv4TestPacket(IPPROTO_ICMP, 0x96, 24 >> 3, 0, 'A', 16); + packets[1] = BuildIpv4TestPacket(IPPROTO_ICMP, 0x96, 8 >> 3, 1, 'B', 16); + packets[2] = BuildIpv4TestPacket(IPPROTO_ICMP, 0x96, 16 >> 3, 1, 'C', 16); + packets[3] = BuildIpv4TestPacket(IPPROTO_ICMP, 0x96, 0, 1, 'D', 8); + + Packet *r = Defrag(NULL, NULL, packets[0]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[1]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[2]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[3]); + FAIL_IF_NULL(r); + + // clang-format off + uint8_t expected[] = { + 'D', 'D', 'D', 'D', 'D', 'D', 'D', 'D', + 'B', 'B', 'B', 'B', 'B', 'B', 'B', 'B', + 'B', 'B', 'B', 'B', 'B', 'B', 'B', 'B', + 'C', 'C', 'C', 'C', 'C', 'C', 'C', 'C', + 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', + }; + // clang-format on + + if (memcmp(expected, GET_PKT_DATA(r) + 20, sizeof(expected)) != 0) { + printf("Expected:\n"); + PrintRawDataFp(stdout, expected, sizeof(expected)); + printf("Got:\n"); + PrintRawDataFp(stdout, GET_PKT_DATA(r) + 20, GET_PKT_LEN(r) - 20); + FAIL; + } + + DefragDestroy(); + PASS; +} + +static int DefragBsdFragmentAfterNoMfIpv6Test(void) +{ + DefragInit(); + default_policy = DEFRAG_POLICY_BSD; + Packet *packets[4]; + + packets[0] = BuildIpv6TestPacket(IPPROTO_ICMP, 0x96, 24 >> 3, 0, 'A', 16); + packets[1] = BuildIpv6TestPacket(IPPROTO_ICMP, 0x96, 8 >> 3, 1, 'B', 16); + packets[2] = BuildIpv6TestPacket(IPPROTO_ICMP, 0x96, 16 >> 3, 1, 'C', 16); + packets[3] = BuildIpv6TestPacket(IPPROTO_ICMP, 0x96, 0, 1, 'D', 8); + + Packet *r = Defrag(NULL, NULL, packets[0]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[1]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[2]); + FAIL_IF_NOT_NULL(r); + + r = Defrag(NULL, NULL, packets[3]); + FAIL_IF_NULL(r); + + // clang-format off + uint8_t expected[] = { + 'D', 'D', 'D', 'D', 'D', 'D', 'D', 'D', + 'B', 'B', 'B', 'B', 'B', 'B', 'B', 'B', + 'B', 'B', 'B', 'B', 'B', 'B', 'B', 'B', + 'C', 'C', 'C', 'C', 'C', 'C', 'C', 'C', + 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', + }; + // clang-format on + + if (memcmp(expected, GET_PKT_DATA(r) + 40, sizeof(expected)) != 0) { + printf("Expected:\n"); + PrintRawDataFp(stdout, expected, sizeof(expected)); + printf("Got:\n"); + PrintRawDataFp(stdout, GET_PKT_DATA(r) + 40, GET_PKT_LEN(r) - 40); + FAIL; + } + + DefragDestroy(); + PASS; +} + #endif /* UNITTESTS */ void DefragRegisterTests(void) @@ -2610,5 +2740,8 @@ void DefragRegisterTests(void) UtRegisterTest("DefragTestBadProto", DefragTestBadProto); UtRegisterTest("DefragTestJeremyLinux", DefragTestJeremyLinux); + + UtRegisterTest("DefragBsdFragmentAfterNoMfIpv4Test", DefragBsdFragmentAfterNoMfIpv4Test); + UtRegisterTest("DefragBsdFragmentAfterNoMfIpv6Test", DefragBsdFragmentAfterNoMfIpv6Test); #endif /* UNITTESTS */ }