From: Numan Siddique <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Michelson <[email protected]>
---
controller/pinctrl.c | 27 ++++++++++
include/ovn/logical-fields.h | 1 +
lib/logical-fields.c | 3 ++
northd/northd.c | 3 +-
tests/ovn.at | 101 ++++++++++++++++++++++++++++++++++-
5 files changed, 132 insertions(+), 3 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index c222d02b7..e8069f106 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -394,6 +394,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);
@@ -3156,6 +3158,10 @@ exit:
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);
@@ -8466,3 +8472,24 @@ pinctrl_handle_put_fdb(const struct flow *md, const
struct flow *headers)
ovn_fdb_add(&put_fdbs, dp_key, headers->dl_src, port_key);
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 f8c184081..70d7ad4c1 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 d84528ef5..7ed75936c 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 70351969f..3bec905cb 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7060,7 +7060,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 dca86b307..2abc62dfa 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -11659,6 +11659,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
])
@@ -16467,7 +16476,7 @@ ovs-vsctl set interface hv2-vif0
external-ids:iface-id=lsp0
OVS_WAIT_UNTIL([test 1 = $(grep -c "Not claiming lport lsp0"
hv2/ovn-controller.log)])
wait_row_count Port_Binding 1 logical_port=lsp0 'chassis=[[]]'
-# (2) Chassis hv2 should not add flows in OFTABLE_PHY_TO_LOG and OFTABLE_LOG_TO_PHY tables.
+# (2) Chassis hv2 should not add flows in table 0 and OFTABLE_LOG_TO_PHY
tables.
