| Message ID | 20170520235753.14374-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, 2017-05-20 at 16:57 -0700, Ben Pfaff wrote: > pinctrl_handle_put_dhcpv6_opts() and pinctrl_handle_dns_lookup() were not > checking that a full UDP header was present before reading its udp_len > field. This patch fixes the problem. > > I don't think that the system as a whole, as normally installed, was > exploitable. This is because pinctrl processes a packet sent to it from > ovs-vswitchd. ovs-vswitchd only sends it UDPv6 DHCPv6 packets. To > determine that the packets are DHCPv6, ovs-vswitchd has to see its UDP port > numbers are those for DHCPv6, and it's only going to see that if an entire > UDP header is present. Therefore, this part of pinctrl will only ever > process a packet for which udp_len is there. > > I believe that pinctrl_handle_dns_lookup() is similar. > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/controller/pinctrl.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > index 9ad413376736..225f6a7563dc 100644 > --- a/ovn/controller/pinctrl.c > +++ b/ovn/controller/pinctrl.c > @@ -526,6 +526,11 @@ pinctrl_handle_put_dhcpv6_opts( > > struct udp_header *in_udp = dp_packet_l4(pkt_in); > const uint8_t *in_dhcpv6_data = dp_packet_get_udp_payload(pkt_in); > + if (!in_udp || !in_dhcpv6_data) { > + VLOG_WARN_RL(&rl, "truncated dhcpv6 packet"); > + goto exit; > + } > + > uint8_t out_dhcpv6_msg_type; > switch(*in_dhcpv6_data) { > case DHCPV6_MSG_TYPE_SOLICIT: > @@ -710,6 +715,10 @@ pinctrl_handle_dns_lookup( > > /* Extract the DNS header */ > struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in); > + if (!in_dns_header) { > + VLOG_WARN_RL(&rl, "truncated dns packet"); > + goto exit; > + } > > /* Check if it is DNS request or not */ > if (in_dns_header->lo_flag & 0x80) { LGTM Acked-by: Greg Rose <gvrose8192@gmail.com>
On Sat, May 20, 2017 at 09:01:34PM -0700, Greg Rose wrote: > On Sat, 2017-05-20 at 16:57 -0700, Ben Pfaff wrote: > > pinctrl_handle_put_dhcpv6_opts() and pinctrl_handle_dns_lookup() were not > > checking that a full UDP header was present before reading its udp_len > > field. This patch fixes the problem. > > > > I don't think that the system as a whole, as normally installed, was > > exploitable. This is because pinctrl processes a packet sent to it from > > ovs-vswitchd. ovs-vswitchd only sends it UDPv6 DHCPv6 packets. To > > determine that the packets are DHCPv6, ovs-vswitchd has to see its UDP port > > numbers are those for DHCPv6, and it's only going to see that if an entire > > UDP header is present. Therefore, this part of pinctrl will only ever > > process a packet for which udp_len is there. > > > > I believe that pinctrl_handle_dns_lookup() is similar. > > > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > ovn/controller/pinctrl.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > > index 9ad413376736..225f6a7563dc 100644 > > --- a/ovn/controller/pinctrl.c > > +++ b/ovn/controller/pinctrl.c > > @@ -526,6 +526,11 @@ pinctrl_handle_put_dhcpv6_opts( > > > > struct udp_header *in_udp = dp_packet_l4(pkt_in); > > const uint8_t *in_dhcpv6_data = dp_packet_get_udp_payload(pkt_in); > > + if (!in_udp || !in_dhcpv6_data) { > > + VLOG_WARN_RL(&rl, "truncated dhcpv6 packet"); > > + goto exit; > > + } > > + > > uint8_t out_dhcpv6_msg_type; > > switch(*in_dhcpv6_data) { > > case DHCPV6_MSG_TYPE_SOLICIT: > > @@ -710,6 +715,10 @@ pinctrl_handle_dns_lookup( > > > > /* Extract the DNS header */ > > struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in); > > + if (!in_dns_header) { > > + VLOG_WARN_RL(&rl, "truncated dns packet"); > > + goto exit; > > + } > > > > /* Check if it is DNS request or not */ > > if (in_dns_header->lo_flag & 0x80) { > > LGTM > > Acked-by: Greg Rose <gvrose8192@gmail.com> Thanks, I applied this to master and branch-2.7.
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index 9ad413376736..225f6a7563dc 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/controller/pinctrl.c @@ -526,6 +526,11 @@ pinctrl_handle_put_dhcpv6_opts( struct udp_header *in_udp = dp_packet_l4(pkt_in); const uint8_t *in_dhcpv6_data = dp_packet_get_udp_payload(pkt_in); + if (!in_udp || !in_dhcpv6_data) { + VLOG_WARN_RL(&rl, "truncated dhcpv6 packet"); + goto exit; + } + uint8_t out_dhcpv6_msg_type; switch(*in_dhcpv6_data) { case DHCPV6_MSG_TYPE_SOLICIT: @@ -710,6 +715,10 @@ pinctrl_handle_dns_lookup( /* Extract the DNS header */ struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in); + if (!in_dns_header) { + VLOG_WARN_RL(&rl, "truncated dns packet"); + goto exit; + } /* Check if it is DNS request or not */ if (in_dns_header->lo_flag & 0x80) {
pinctrl_handle_put_dhcpv6_opts() and pinctrl_handle_dns_lookup() were not checking that a full UDP header was present before reading its udp_len field. This patch fixes the problem. I don't think that the system as a whole, as normally installed, was exploitable. This is because pinctrl processes a packet sent to it from ovs-vswitchd. ovs-vswitchd only sends it UDPv6 DHCPv6 packets. To determine that the packets are DHCPv6, ovs-vswitchd has to see its UDP port numbers are those for DHCPv6, and it's only going to see that if an entire UDP header is present. Therefore, this part of pinctrl will only ever process a packet for which udp_len is there. I believe that pinctrl_handle_dns_lookup() is similar. Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/controller/pinctrl.c | 9 +++++++++ 1 file changed, 9 insertions(+)