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
pull/16253/head
Samaresh Kumar Singh 5 months ago committed by Victor Julien
parent 79fd1826ef
commit 38c9b1e0e0

@ -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 */
}

Loading…
Cancel
Save