From 249c52ad011cacb4c182dc64e88977ac7c61f668 Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Thu, 28 Nov 2024 11:05:26 -0500 Subject: [PATCH] Skip only OVN DNS responder packets from OUT_ACL. When OVN's DNS caching feature is enabled, due to the OpenFlow rules that OVN installs in Open vSwitch, it is possible for an attacker to craft a UDP packet that can bypass egress ACL rules configured on the same switch that has DNS caching configured. This patch fixes the issue by setting a register bit when OVN's DNS responder replies to an incoming request. Then the flow that allows egress ACL bypass only applies to packets that have this register bit set. This gives the intended effect of allowing internally-generated DNS responses to not be blocked by user-defined ACLs without potentially compromising the security of the switch. Signed-off-by: Numan Siddique Signed-off-by: Mark Michelson Acked-by: Dumitru Ceara --- controller/pinctrl.c | 27 ++++++++++ include/ovn/logical-fields.h | 1 + lib/logical-fields.c | 3 ++ northd/northd.c | 3 +- tests/ovn.at | 97 ++++++++++++++++++++++++++++++++++++ 5 files changed, 130 insertions(+), 1 deletion(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 1d50283b98..873879d3c9 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -389,6 +389,8 @@ static void pinctrl_handle_put_fdb(const struct flow *md, const struct flow *headers) OVS_REQUIRES(pinctrl_mutex); +static void set_from_ctrl_flag_in_pkt_metadata(struct ofputil_packet_in *); + COVERAGE_DEFINE(pinctrl_drop_put_mac_binding); COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map); COVERAGE_DEFINE(pinctrl_drop_controller_event); @@ -3586,6 +3588,10 @@ pinctrl_handle_dns_lookup( union mf_subvalue sv; sv.u8_val = success; mf_write_subfield(&dst, &sv, &pin->flow_metadata); + + /* Indicate that this packet is from ovn-controller. */ + set_from_ctrl_flag_in_pkt_metadata(pin); + } queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto)); dp_packet_uninit(pkt_out_ptr); @@ -8910,3 +8916,24 @@ pinctrl_handle_put_fdb(const struct flow *md, const struct flow *headers) fdb_add(&put_fdbs, fdb_data, timestamp); notify_pinctrl_main(); } + +/* This function sets the register bit 'MLF_FROM_CTRL_BIT' + * in the register 'MFF_LOG_FLAGS' to indicate that this packet + * is generated/sent by ovn-controller. + * ovn-northd can add logical flows to match on "flags.from_ctrl". + */ +static void +set_from_ctrl_flag_in_pkt_metadata(struct ofputil_packet_in *pin) +{ + const struct mf_field *f = mf_from_id(MFF_LOG_FLAGS); + + struct mf_subfield dst = { + .field = f, + .ofs = MLF_FROM_CTRL_BIT, + .n_bits = 1, + }; + + union mf_subvalue sv; + sv.u8_val = 1; + mf_write_subfield(&dst, &sv, &pin->flow_metadata); +} diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index da7f9a2ca9..59e4ac3da0 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -88,6 +88,7 @@ enum mff_log_flags_bits { MLF_RX_FROM_TUNNEL_BIT = 16, MLF_ICMP_SNAT_BIT = 17, MLF_OVERRIDE_LOCAL_ONLY_BIT = 18, + MLF_FROM_CTRL_BIT = 19, }; /* MFF_LOG_FLAGS_REG flag assignments */ diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 5a8b53f2b6..f49a0a79db 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -140,6 +140,9 @@ ovn_init_symtab(struct shash *symtab) snprintf(flags_str, sizeof flags_str, "flags[%d]", MLF_RX_FROM_TUNNEL_BIT); expr_symtab_add_subfield(symtab, "flags.tunnel_rx", NULL, flags_str); + snprintf(flags_str, sizeof flags_str, "flags[%d]", MLF_FROM_CTRL_BIT); + expr_symtab_add_subfield(symtab, "flags.from_ctrl", NULL, flags_str); + /* Connection tracking state. */ expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false, WR_CT_COMMIT); diff --git a/northd/northd.c b/northd/northd.c index b2216c0621..48897e95b6 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7717,7 +7717,8 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, "ct_commit; next;" : REGBIT_ACL_VERDICT_ALLOW" = 1; next;"; ovn_lflow_add( - lflows, od, S_SWITCH_OUT_ACL_EVAL, 34000, "udp.src == 53", + lflows, od, S_SWITCH_OUT_ACL_EVAL, 34000, + "flags.from_ctrl && udp.src == 53", dns_actions, lflow_ref); } diff --git a/tests/ovn.at b/tests/ovn.at index 443c215249..e7d04e8d68 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -11722,6 +11722,15 @@ echo ${dns_reply} > expected as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 ${dns_req} OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv1/vif1-tx.pcap], [expected]) +AS_BOX([Add ACL to drop udp.src == 53 in egress stage]) + +# DNS reply from ovn-controller should still be delivered. +check ovn-nbctl --wait=hv acl-add ls to-lport 1002 "udp.src == 53" drop +as hv1 reset_pcap_file hv1-vif1 hv1/vif1 + +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 ${dns_req} +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected]) + OVN_CLEANUP([hv1]) AT_CLEANUP ]) @@ -39339,3 +39348,91 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY,metadata=0x${ls_t OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([DNS reply packet with ACL to drop it]) +AT_SKIP_IF([test $HAVE_SCAPY = no]) +ovn_start +net_add n1 +sim_add hv1 + +as hv1 +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.11 +ovs-vsctl -- add-port br-int vif1 -- \ + set interface vif1 external-ids:iface-id=sw0p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 +ovs-vsctl -- add-port br-int vif2 -- \ + set interface vif2 external-ids:iface-id=sw0p2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0p1 -- \ + lsp-set-addresses sw0p1 "50:54:00:00:00:03 10.0.0.3" +check ovn-nbctl --wait=hv lsp-add sw0 sw0p2 -- \ + lsp-set-addresses sw0p2 "50:54:00:00:00:04 10.0.0.4" + +wait_for_ports_up + +# Send 2 UDP packets from sw0p1 to sw0p2. +# One with udp.src = 53 and the other with udp.src = 54. +# If dropped is yes, then the udp pkt with udp.src == 53 should +# be dropped. +send_udp_packets() { + rm -f expected + dropped=$1 + packet=$(fmt_pkt " + Ether(dst='50:54:00:00:00:04', src='50:54:00:00:00:03') / + IP(src='10.0.0.3', dst='10.0.0.4') / + UDP(sport=53, dport=1235) + ") + ovs-appctl netdev-dummy/receive vif1 $packet + if [[ "$dropped" != "yes" ]]; then + echo $packet > expected + fi + packet=$(fmt_pkt " + Ether(dst='50:54:00:00:00:04', src='50:54:00:00:00:03') / + IP(src='10.0.0.3', dst='10.0.0.4') / + UDP(sport=54, dport=1235) + ") + ovs-appctl netdev-dummy/receive vif1 $packet + echo $packet >> expected +} + +# Send UDP packets with src port 53 and 54 from sw0p1 to sw0p2. Both should be +# delivered to sw0p2. +as hv1 reset_pcap_file vif1 hv1/vif1 +as hv1 reset_pcap_file vif2 hv1/vif2 + +send_udp_packets no + +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [expected]) + +AS_BOX([Create DNS records, add ACL to drop DNS reply packet]) +DNS1=$(ovn-nbctl create DNS records={vm1=10.0.0.3}) +check ovn-nbctl set logical_switch sw0 dns_records=$DNS1 +check ovn-nbctl --wait=hv acl-add sw0 to-lport 1002 "udp.src == 53" drop +ovn-sbctl dump-flows sw0 > sw0flows +AT_CAPTURE_FILE([sw0flows]) + +AT_CHECK([grep "ls_out_acl_eval" sw0flows | ovn_strip_lflows | grep "udp.src"], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((udp.src == 53)), action=(reg8[[17]] = 1; next;) + table=??(ls_out_acl_eval ), priority=34000, match=(flags.from_ctrl && udp.src == 53), action=(reg8[[16]] = 1; next;) +]) + +AS_BOX([Send UDP packets from sw0p1 to sw0p2 and pkt with udp.src == 53 should be dropped]) +as hv1 reset_pcap_file vif1 hv1/vif1 +as hv1 reset_pcap_file vif2 hv1/vif2 +# Send UDP packets with src port 53 and 54 from sw0p1 to sw0p2. Only with udp.src = 54 +# should be delivered to sw0p2. +send_udp_packets yes + +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])