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]> Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
