From: Klement Sekera Date: Mon, 10 Dec 2018 12:46:09 +0000 (+0100) Subject: VPP-1522: harden reassembly code X-Git-Tag: v19.04-rc0~172 X-Git-Url: https://gerrit.fd.io/r/gitweb?p=vpp.git;a=commitdiff_plain;h=14d7e90788f87a4b7b174bb5a67171f8c9a7f842 VPP-1522: harden reassembly code Change-Id: Ib5a20bff7d8a340ecf50bcd4a023d6bf36382ba3 Signed-off-by: Klement Sekera --- diff --git a/src/vnet/ip/ip4_error.h b/src/vnet/ip/ip4_error.h index 52c43adb12c..15828e39ac5 100644 --- a/src/vnet/ip/ip4_error.h +++ b/src/vnet/ip/ip4_error.h @@ -86,7 +86,8 @@ /* Errors signalled by ip4-reassembly */ \ _ (REASS_DUPLICATE_FRAGMENT, "duplicate/overlapping fragments") \ _ (REASS_LIMIT_REACHED, "drops due to concurrent reassemblies limit") \ - _ (REASS_TIMEOUT, "fragments dropped due to reassembly timeout") + _ (REASS_TIMEOUT, "fragments dropped due to reassembly timeout") \ + _ (REASS_MALFORMED_PACKET, "malformed packets") typedef enum { diff --git a/src/vnet/ip/ip4_reassembly.c b/src/vnet/ip/ip4_reassembly.c index bd33026db85..9bcc20a2285 100644 --- a/src/vnet/ip/ip4_reassembly.c +++ b/src/vnet/ip/ip4_reassembly.c @@ -654,12 +654,12 @@ ip4_reass_update (vlib_main_t * vm, vlib_node_runtime_t * node, ASSERT (fb->current_length >= sizeof (*fip)); vnet_buffer_opaque_t *fvnb = vnet_buffer (fb); reass->next_index = fvnb->ip.reass.next_index; // store next_index before it's overwritten - u32 fragment_first = fvnb->ip.reass.fragment_first = - ip4_get_fragment_offset_bytes (fip); - u32 fragment_length = + const u32 fragment_first = ip4_get_fragment_offset_bytes (fip); + const u32 fragment_length = clib_net_to_host_u16 (fip->length) - ip4_header_bytes (fip); - u32 fragment_last = fvnb->ip.reass.fragment_last = - fragment_first + fragment_length - 1; + const u32 fragment_last = fragment_first + fragment_length - 1; + fvnb->ip.reass.fragment_first = fragment_first; + fvnb->ip.reass.fragment_last = fragment_last; int more_fragments = ip4_get_fragment_more (fip); u32 candidate_range_bi = reass->first_bi; u32 prev_range_bi = ~0; @@ -927,28 +927,43 @@ ip4_reassembly_inline (vlib_main_t * vm, } else { - ip4_reass_key_t k; - k.as_u64[0] = - (u64) vnet_buffer (b0)->sw_if_index[VLIB_RX] << 32 | (u64) - ip0->src_address.as_u32; - k.as_u64[1] = - (u64) ip0->dst_address. - as_u32 << 32 | (u64) ip0->fragment_id << 16 | (u64) ip0-> - protocol << 8; - - ip4_reass_t *reass = - ip4_reass_find_or_create (vm, rm, rt, &k, &vec_drop_timeout); - - if (reass) + ip4_header_t *fip = vlib_buffer_get_current (b0); + const u32 fragment_first = ip4_get_fragment_offset_bytes (fip); + const u32 fragment_length = + clib_net_to_host_u16 (fip->length) - ip4_header_bytes (fip); + const u32 fragment_last = fragment_first + fragment_length - 1; + if (fragment_first > fragment_last + || fragment_first + fragment_length > UINT16_MAX - 20) { - ip4_reass_update (vm, node, rm, rt, reass, &bi0, &next0, - &error0, &vec_drop_overlap, - &vec_drop_compress, is_feature); + next0 = IP4_REASSEMBLY_NEXT_DROP; + error0 = IP4_ERROR_REASS_MALFORMED_PACKET; } else { - next0 = IP4_REASSEMBLY_NEXT_DROP; - error0 = IP4_ERROR_REASS_LIMIT_REACHED; + ip4_reass_key_t k; + k.as_u64[0] = + (u64) vnet_buffer (b0)->sw_if_index[VLIB_RX] << 32 | (u64) + ip0->src_address.as_u32; + k.as_u64[1] = + (u64) ip0->dst_address. + as_u32 << 32 | (u64) ip0->fragment_id << 16 | (u64) ip0-> + protocol << 8; + + ip4_reass_t *reass = + ip4_reass_find_or_create (vm, rm, rt, &k, + &vec_drop_timeout); + + if (reass) + { + ip4_reass_update (vm, node, rm, rt, reass, &bi0, &next0, + &error0, &vec_drop_overlap, + &vec_drop_compress, is_feature); + } + else + { + next0 = IP4_REASSEMBLY_NEXT_DROP; + error0 = IP4_ERROR_REASS_LIMIT_REACHED; + } } b0->error = node->errors[error0]; diff --git a/test/framework.py b/test/framework.py index f82f0750387..859010cf325 100644 --- a/test/framework.py +++ b/test/framework.py @@ -916,15 +916,18 @@ class VppTestCase(unittest.TestCase): self.assert_checksum_valid(pkt, 'ICMPv6EchoReply', 'cksum') def assert_packet_counter_equal(self, counter, expected_value): - counters = self.vapi.cli("sh errors").split('\n') - counter_value = -1 - for i in range(1, len(counters)): - results = counters[i].split() - if results[1] == counter: - counter_value = int(results[0]) - break - self.assert_equal(counter_value, expected_value, - "packet counter `%s'" % counter) + if counter.startswith("/"): + counter_value = self.statistics.get_counter(counter) + self.assert_equal(counter_value, expected_value, + "packet counter `%s'" % counter) + else: + counters = self.vapi.cli("sh errors").split('\n') + counter_value = -1 + for i in range(1, len(counters) - 1): + results = counters[i].split() + if results[1] == counter: + counter_value = int(results[0]) + break @classmethod def sleep(cls, timeout, remark=None): diff --git a/test/template_ipsec.py b/test/template_ipsec.py index 961f63aa59c..d35cf420d37 100644 --- a/test/template_ipsec.py +++ b/test/template_ipsec.py @@ -233,9 +233,8 @@ class IpsecTraTests(object): seq_num=17)) self.send_and_assert_no_replies(self.tra_if, pkt * 17) - err = self.statistics.get_counter( - '/err/%s/SA replayed packet' % self.tra4_decrypt_node_name) - self.assertEqual(err, 17) + self.assert_packet_counter_equal( + '/err/%s/SA replayed packet' % self.tra4_decrypt_node_name, 17) # a packet that does not decrypt does not move the window forward bogus_sa = SecurityAssociation(self.encryption_type, @@ -248,9 +247,8 @@ class IpsecTraTests(object): seq_num=350)) self.send_and_assert_no_replies(self.tra_if, pkt * 17) - err = self.statistics.get_counter( - '/err/%s/Integrity check failed' % self.tra4_decrypt_node_name) - self.assertEqual(err, 17) + self.assert_packet_counter_equal( + '/err/%s/Integrity check failed' % self.tra4_decrypt_node_name, 17) # which we can determine since this packet is still in the window pkt = (Ether(src=self.tra_if.remote_mac, @@ -259,7 +257,7 @@ class IpsecTraTests(object): dst=self.tra_if.local_ip4) / ICMP(), seq_num=234)) - recv_pkts = self.send_and_expect(self.tra_if, [pkt], self.tra_if) + self.send_and_expect(self.tra_if, [pkt], self.tra_if) # move the security-associations seq number on to the last we used p.scapy_tra_sa.seq_num = 351 diff --git a/test/test_reassembly.py b/test/test_reassembly.py index 0c11d9b3ce8..65a5eb0914d 100644 --- a/test/test_reassembly.py +++ b/test/test_reassembly.py @@ -178,6 +178,31 @@ class TestIPv4Reassembly(VppTestCase): self.verify_capture(packets) self.src_if.assert_nothing_captured() + def test_5737(self): + """ fragment length + ip header size > 65535 """ + raw = ('E\x00\x00\x88,\xf8\x1f\xfe@\x01\x98\x00\xc0\xa8\n-\xc0\xa8\n' + '\x01\x08\x00\xf0J\xed\xcb\xf1\xf5Test-group: IPv4.IPv4.ipv4-' + 'message.Ethernet-Payload.IPv4-Packet.IPv4-Header.Fragment-Of' + 'fset; Test-case: 5737') + + malformed_packet = (Ether(dst=self.src_if.local_mac, + src=self.src_if.remote_mac) / + IP(raw)) + p = (Ether(dst=self.src_if.local_mac, src=self.src_if.remote_mac) / + IP(id=1000, src=self.src_if.remote_ip4, + dst=self.dst_if.remote_ip4) / + UDP(sport=1234, dport=5678) / + Raw("X" * 1000)) + valid_fragments = fragment_rfc791(p, 400) + + self.pg_enable_capture() + self.src_if.add_stream([malformed_packet] + valid_fragments) + self.pg_start() + + self.dst_if.get_capture(1) + self.assert_packet_counter_equal( + "/err/ip4-reassembly-feature/malformed packets", 1) + @unittest.skipIf(is_skip_aarch64_set() and is_platform_aarch64(), "test doesn't work on aarch64") def test_random(self): @@ -838,7 +863,7 @@ class TestIPv4ReassemblyLocalNode(VppTestCase): seen.add(packet_index) self.assertEqual(payload_info.dst, self.src_dst_if.sw_if_index) info = self._packet_infos[packet_index] - self.assertTrue(info is not None) + self.assertIsNotNone(info) self.assertEqual(packet_index, info.index) saved_packet = info.data self.assertEqual(ip.src, saved_packet[IP].dst) @@ -850,8 +875,8 @@ class TestIPv4ReassemblyLocalNode(VppTestCase): self.logger.error(ppp("Unexpected or invalid packet:", packet)) raise for index in self._packet_infos: - self.assertTrue(index in seen or index in dropped_packet_indexes, - "Packet with packet_index %d not received" % index) + self.assertIn(index, seen, + "Packet with packet_index %d not received" % index) def test_reassembly(self): """ basic reassembly """