On 1/22/25 12:03 AM, Mark Michelson wrote: > On 1/21/25 17:54, Dumitru Ceara wrote: >> On 1/21/25 6:33 PM, Mark Michelson wrote: >>> 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. >> >> Nit: this differs from the original patch posted for the main branch. >> It seems unrelated, I'd leave this change out (it's only in a comment >> anyway). >> >>> AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=0 | grep >>> in_port=$hv2_ofport], [1], []) >>> AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=65 | grep >>> output], [1], []) >>> @@ -16479,7 +16488,7 @@ ovs-vsctl set interface hv1-vif0 external- >>> ids:iface-id=lsp0 >>> OVS_WAIT_UNTIL([test 1 = $(grep -c "Claiming lport lsp0" hv1/ovn- >>> controller.log)]) >>> wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp0 >>> -# (4) Chassis hv1 should add flows in OFTABLE_PHY_TO_LOG and >>> OFTABLE_LOG_TO_PHY tables. >>> +# (4) Chassis hv1 should add flows in table 0 and OFTABLE_LOG_TO_PHY >>> tables. >> >> Nit: this differs from the original patch posted for the main branch. >> It seems unrelated, I'd leave this change out (it's only in a comment >> anyway). >> >> With the above nits addressed: >> >> Acked-by: Dumitru Ceara <[email protected]> > > I addressed the nits and pushed this (and the other patches) to all > branches. >
It seems we missed the fact that the EDNS test doesn't run properly with this change on 22.03. I sent a follow up patch for 22.03 here: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Without it 22.03 branch CI perma-fails. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
