On Mon, Mar 27, 2023 at 03:34:52PM +0200, Simon Horman wrote:
> On Wed, Mar 15, 2023 at 05:11:01PM +0800, Faicker Mo wrote:
> > Derivation cases of CVE-2020-35498:
> > 1. invalid ipv4 header total-length field
> > 2. invalid ipv6 header payload-length field
> > These may cause unwanted flow to send to datapath.
> >
> >
> > Signed-off-by: Faicker Mo <faicker...@ucloud.cn>
>
> I think the immediate question here is how to correctly handle invalid
> packets which claim to have a total length that is greater than the length
> received (size) by OVS.
OVS strives to forward all packets but at the same time it
can't rely on data from non-conforming packets.
You can see with checksum that when OVS changes a packet
it recalculates the checksum in a way that it doesn't
change the original state (good/bad).
> It doesn't seem to me that truncating the total length to size,
> and thus pretending the packets are valid in this regard,
> is the right approach.
+1
> Is there a bigger problem in that any packets that fail the sanity check
> are problematic with regards to megaflow creation? If so, I think we should
> aim for a more comprehensive solution.
+1
fbl
>
> > ---
> > lib/flow.c | 11 +++++------
> > tests/classifier.at | 42 ++++++++++++++++++++++++++++++++++++++++++
> > tests/ofp-print.at | 2 +-
> > 3 files changed, 48 insertions(+), 7 deletions(-)
> >
> >
> > diff --git a/lib/flow.c b/lib/flow.c
> > index c3a3aa3ce..d96d02213 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -662,9 +662,8 @@ ipv4_sanity_check(const struct ip_header *nh, size_t
> > size,
> > return false;
> > }
> >
> > - tot_len = ntohs(nh->ip_tot_len);
> > - if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
> > - size - tot_len > UINT16_MAX)) {
> > + tot_len = MIN(size, ntohs(nh->ip_tot_len));
> > + if (OVS_UNLIKELY(ip_len > tot_len || size - tot_len > UINT16_MAX)) {
> > COVERAGE_INC(miniflow_extract_ipv4_pkt_len_error);
> > return false;
> > }
> > @@ -700,7 +699,7 @@ ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr
> > *nh, size_t size)
> > return false;
> > }
> >
> > - plen = ntohs(nh->ip6_plen);
> > + plen = MIN(size - sizeof *nh, ntohs(nh->ip6_plen));
> > if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> > COVERAGE_INC(miniflow_extract_ipv6_pkt_len_error);
> > return false;
> > @@ -920,7 +919,7 @@ miniflow_extract(struct dp_packet *packet, struct
> > miniflow *dst)
> > }
> > data_pull(&data, &size, sizeof *nh);
> >
> > - plen = ntohs(nh->ip6_plen);
> > + plen = MIN(size, ntohs(nh->ip6_plen));
> > dp_packet_set_l2_pad_size(packet, size - plen);
> > size = plen; /* Never pull padding. */
> >
> > @@ -1197,7 +1196,7 @@ parse_tcp_flags(struct dp_packet *packet,
> > }
> > data_pull(&data, &size, sizeof *nh);
> >
> > - plen = ntohs(nh->ip6_plen); /* Never pull padding. */
> > + plen = MIN(size, ntohs(nh->ip6_plen)); /* Never pull padding. */
> > dp_packet_set_l2_pad_size(packet, size - plen);
> > size = plen;
> > const struct ovs_16aligned_ip6_frag *frag_hdr;
> > diff --git a/tests/classifier.at b/tests/classifier.at
> > index de2705653..1a1615bb5 100644
> > --- a/tests/classifier.at
> > +++ b/tests/classifier.at
> > @@ -418,6 +418,7 @@ ovs-ofctl: "conjunction" actions may be used along with
> > "note" but not any other
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_BANNER([flow classifier abnormal packet])
> > # Flow classifier a packet with excess of padding.
> > AT_SETUP([flow classifier - packet with extra padding])
> > OVS_VSWITCHD_START
> > @@ -453,3 +454,44 @@ Datapath actions: 2
> > ])
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> > +
> > +dnl Flow classifier a packet with invalid total-length field of ipv4 header
> > +AT_SETUP([flow classifier - packet with invalid total-length field of ipv4
> > header])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +AT_DATA([flows.txt], [dnl
> > +priority=5,ip,ip_dst=1.1.1.1,actions=1
> > +priority=5,ip,ip_dst=1.1.1.2,actions=2
> > +priority=0,actions=drop
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +packet=00000000000000000000000008004500003c00010000401176ac0101010101010102003500350008fb4f
> > +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1 $packet] , [0], [stdout])
> > +dnl the problem flow,
> > +dnl Megaflow: recirc_id=0,eth,ip,in_port=1,nw_dst=0.0.0.0/8,nw_frag=no
> > +dnl Datapath actions: drop
> > +AT_CHECK([tail -2 stdout], [0],
> > + [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_dst=1.1.1.2,nw_frag=no
> > +Datapath actions: 2
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +dnl Flow classifier a packet with invalid payload-length field of ipv4
> > header
> > +AT_SETUP([flow classifier - packet with invalid payload-length field of
> > ipv6 header])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +AT_DATA([flows.txt], [dnl
> > +priority=5,ipv6,ipv6_dst=1::1,actions=1
> > +priority=5,ipv6,ipv6_dst=1::2,actions=2
> > +priority=0,actions=drop
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +packet=00000000000000000000000086dd60000000001e11400001000000000000000000000000000100010000000000000000000000000002003500350008ff6f
> > +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1 $packet] , [0], [stdout])
> > +AT_CHECK([tail -2 stdout], [0],
> > + [Megaflow: recirc_id=0,eth,ipv6,in_port=1,ipv6_dst=1::2,nw_frag=no
> > +Datapath actions: 2
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> > index 14aa55416..0c786a542 100644
> > --- a/tests/ofp-print.at
> > +++ b/tests/ofp-print.at
> > @@ -3151,7 +3151,7 @@ AT_CHECK([ovs-ofctl ofp-print "
> > ], [0], [dnl
> > NXT_PACKET_IN2 (xid=0x0): table_id=7 cookie=0xfedcba9876543210
> > total_len=64 metadata=0x5a5a5a5a5a5a5a5a (via action) data_len=48
> > buffer=0x00000114
> > userdata=01.02.03.04.05
> > -ip,dl_vlan=80,dl_vlan_pcp=0,vlan_tci1=0x0000,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=0.0.0.0,nw_dst=0.0.0.0,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
> > +tcp,dl_vlan=80,dl_vlan_pcp=0,vlan_tci1=0x0000,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=83.83.83.83,nw_dst=84.84.84.84,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no,tp_src=0,tp_dst=0,tcp_flags=0
> > ])
> > AT_CLEANUP
> >
> > --
> > 2.31.1
> >
> >
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
