The ICMP need frag copies part of the IP packet, which is limited by
the space after ICMP header. However the packet size would be taken
from the IP header itself. That is problematic because we could
receive empty packet with the IP header packet size set to arbitrary
number. To prevent that limit the size to the buffer size so we will
never copy more than what is in the packet data.
Fixes: c2339d87268d ("ovn: Add a new OVN action 'icmp4_error'")
Reported-by: Seiji Sakurai <[email protected]>
Co-authored-by: Seiji Sakurai <[email protected]>
Acked-by: Dumitru Ceara <[email protected]>
Signed-off-by: Seiji Sakurai <[email protected]>
Signed-off-by: Ales Musil <[email protected]>
---
controller/pinctrl.c | 7 ++--
tests/system-ovn.at | 83 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 18b7b0df2..682b88b1a 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1674,7 +1674,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct
flow *ip_flow,
if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
struct ip_header *in_ip = dp_packet_l3(pkt_in);
- uint16_t in_ip_len = ntohs(in_ip->ip_tot_len);
+ uint16_t in_ip_len =
+ MIN(ntohs(in_ip->ip_tot_len), dp_packet_l3_size(pkt_in));
if (in_ip_len < IP_HEADER_LEN) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl,
@@ -1734,7 +1735,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct
flow *ip_flow,
ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len);
} else {
struct ovs_16aligned_ip6_hdr *in_ip = dp_packet_l3(pkt_in);
- uint16_t in_ip_len = (uint16_t) sizeof *in_ip + ntohs(in_ip->ip6_plen);
+ uint16_t pkt_in_ip_len =
+ (uint16_t) sizeof *in_ip + ntohs(in_ip->ip6_plen);
+ uint16_t in_ip_len = MIN(pkt_in_ip_len, dp_packet_l3_size(pkt_in));
const struct in6_addr *ip6_src =
loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 8d1f21609..06c5c4b2c 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -21665,3 +21665,86 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
patch-.*/d
/connection dropped.*/d"])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ACL - ICMP unreachable heap overread])
+AT_SKIP_IF([test $HAVE_SCAPY = no])
+
+ovn_start
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller.
+check ovs-vsctl \
+ -- set Open_vSwitch . external-ids:system-id=hv1 \
+ -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+ -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+ -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+ -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+
+# Create a logical switch with a port and a reject ACL.
+check ovn-nbctl ls-add ls1
+check ovn-nbctl lsp-add ls1 ls1-lp1 \
+ -- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01 10.0.0.4 fd10::4"
+
+# Add a reject ACL: any traffic to ls1-lp1 gets ICMP Destination Unreachable.
+check ovn-nbctl acl-add ls1 to-lport 1000 "outport == \"ls1-lp1\"" reject
+
+# We need a second port as the "sender".
+check ovn-nbctl lsp-add ls1 ls1-lp2 \
+ -- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02 10.0.0.5 fd10::5"
+
+ADD_NAMESPACES(ls1-lp1)
+ADD_VETH(ls1-lp1, ls1-lp1, br-int, "fd10::4/96", "f0:00:00:00:00:01", \
+ "fd10::1", "nodad", "10.0.0.4/24", "10.0.0.1")
+
+ADD_NAMESPACES(ls1-lp2)
+ADD_VETH(ls1-lp2, ls1-lp2, br-int, "fd10::5/96", "f0:00:00:00:00:02", \
+ "fd10::1", "nodad", "10.0.0.5/24", "10.0.0.1")
+
+NETNS_START_TCPDUMP([ls1-lp2], [-nnne -i ls1-lp2 icmp or icmp6], [ls1-lp2])
+
+OVN_POPULATE_ARP
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+# UDP but IP length claims 508 bytes while actual packet is smaller.
+ip netns exec ls1-lp2 scapy -H <<-EOF
+p = Ether(dst='f0:00:00:00:00:01', src='f0:00:00:00:00:02') / \
+ IP(src='10.0.0.5', dst='10.0.0.4', ttl=64, len=508) / \
+ UDP(sport=12345, dport=5050) / \
+ Raw(load=b'AAAA')
+sendp (p, iface='ls1-lp2', loop = 0, verbose = 0, count = 1)
+EOF
+
+# UDP but IPv6 length claims 508 bytes while actual packet is smaller.
+ip netns exec ls1-lp2 scapy -H <<-EOF
+p = Ether(dst='f0:00:00:00:00:01', src='f0:00:00:00:00:02') / \
+ IPv6(src='fd10::5', dst='fd10::4', plen=508) / \
+ UDP(sport=12345, dport=5050) / \
+ Raw(load=b'AAAA')
+sendp (p, iface='ls1-lp2', loop = 0, verbose = 0, count = 1)
+EOF
+
+OVS_WAIT_UNTIL([
+ test "$(grep 'unreachable' -c ls1-lp2.tcpdump)" = "2"
+])
+
+ip4_length=$(grep "ICMP " ls1-lp2.tcpdump | grep "unreachable" | rev | cut -d
" " -f1 | rev)
+ip6_length=$(grep "ICMP6" ls1-lp2.tcpdump | grep "unreachable" | rev | cut -d
" " -f1 | rev)
+
+check test $ip4_length -eq 40
+check test $ip6_length -eq 60
+
+OVN_CLEANUP_CONTROLLER([hv1])
+OVN_CLEANUP_NORTHD
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+
+AT_CLEANUP
+])
--
2.53.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
