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