[ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
Otherwise a malformed packet could cause a read up to about 40 bytes past
the end of the packet. The packet would still likely be dropped because
of checksum verification.
Reported-by: Bhargava Shastry
Signed-off-by: Ben Pfaff
---
lib/conntrack.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 9bea3d93e4ad..9c1dd63648b8 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const void *data,
size_t size,
const char **new_data)
{
const struct ovs_16aligned_ip6_hdr *ip6 = data;
+if (size < sizeof *ip6) {
+return false;
+}
+
uint8_t nw_proto = ip6->ip6_nxt;
uint8_t nw_frag = 0;
@@ -623,8 +627,11 @@ check_l4_tcp(const struct conn_key *key, const void *data,
size_t size,
const void *l3)
{
const struct tcp_header *tcp = data;
-size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+if (size < sizeof *tcp) {
+return false;
+}
+size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
if (OVS_UNLIKELY(tcp_len < TCP_HEADER_LEN || tcp_len > size)) {
return false;
}
@@ -637,8 +644,11 @@ check_l4_udp(const struct conn_key *key, const void *data,
size_t size,
const void *l3)
{
const struct udp_header *udp = data;
-size_t udp_len = ntohs(udp->udp_len);
+if (size < sizeof *udp) {
+return false;
+}
+size_t udp_len = ntohs(udp->udp_len);
if (OVS_UNLIKELY(udp_len < UDP_HEADER_LEN || udp_len > size)) {
return false;
}
--
2.10.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
2017-03-03 14:08 GMT-08:00 Ben Pfaff :
> Otherwise a malformed packet could cause a read up to about 40 bytes past
> the end of the packet. The packet would still likely be dropped because
> of checksum verification.
>
> Reported-by: Bhargava Shastry
> Signed-off-by: Ben Pfaff
Oops, thanks for the fix, Ben
Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
One minor comment below,
Acked-by: Daniele Di Proietto
> ---
> lib/conntrack.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 9bea3d93e4ad..9c1dd63648b8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const void *data,
> size_t size,
> const char **new_data)
> {
> const struct ovs_16aligned_ip6_hdr *ip6 = data;
> +if (size < sizeof *ip6) {
> +return false;
> +}
> +
We can read 'ip6->ip6_nxt' even though there's not enough data. It
cannot happen
for regular TCP and UDP packets (those are covered my
miniflow_extract), but only
when parsing the nested l3 header in an ICMP error message.
The code has the same check two lines below, maybe we can reuse that.
Technically
the check is necessary only if new_data != NULL, as explained by the comment
above, but perhaps it's more clear to always perform it.
> uint8_t nw_proto = ip6->ip6_nxt;
> uint8_t nw_frag = 0;
>
> @@ -623,8 +627,11 @@ check_l4_tcp(const struct conn_key *key, const void
> *data, size_t size,
> const void *l3)
> {
> const struct tcp_header *tcp = data;
> -size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
> +if (size < sizeof *tcp) {
> +return false;
> +}
>
> +size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
> if (OVS_UNLIKELY(tcp_len < TCP_HEADER_LEN || tcp_len > size)) {
> return false;
> }
> @@ -637,8 +644,11 @@ check_l4_udp(const struct conn_key *key, const void
> *data, size_t size,
> const void *l3)
> {
> const struct udp_header *udp = data;
> -size_t udp_len = ntohs(udp->udp_len);
> +if (size < sizeof *udp) {
> +return false;
> +}
>
> +size_t udp_len = ntohs(udp->udp_len);
> if (OVS_UNLIKELY(udp_len < UDP_HEADER_LEN || udp_len > size)) {
> return false;
> }
> --
> 2.10.2
>
> ___
> 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
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote:
> 2017-03-03 14:08 GMT-08:00 Ben Pfaff :
> > Otherwise a malformed packet could cause a read up to about 40 bytes past
> > the end of the packet. The packet would still likely be dropped because
> > of checksum verification.
> >
> > Reported-by: Bhargava Shastry
> > Signed-off-by: Ben Pfaff
>
> Oops, thanks for the fix, Ben
>
> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
>
> One minor comment below,
>
> Acked-by: Daniele Di Proietto
>
>
> > ---
> > lib/conntrack.c | 14 --
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 9bea3d93e4ad..9c1dd63648b8 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const void
> > *data, size_t size,
> > const char **new_data)
> > {
> > const struct ovs_16aligned_ip6_hdr *ip6 = data;
> > +if (size < sizeof *ip6) {
> > +return false;
> > +}
> > +
>
> We can read 'ip6->ip6_nxt' even though there's not enough data. It
> cannot happen
> for regular TCP and UDP packets (those are covered my
> miniflow_extract), but only
> when parsing the nested l3 header in an ICMP error message.
>
> The code has the same check two lines below, maybe we can reuse that.
> Technically
> the check is necessary only if new_data != NULL, as explained by the comment
> above, but perhaps it's more clear to always perform it.
Thanks for pointing out the duplicate check. I guess that I did not
read the code carefully enough.
Usually, I would argue to always do the check, but I prefer to make this
a minimal change.
I will post a v2.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
Hi Ben,
Question regarding patch: Shouldn't the fix be applied in flow extract code
itself? I think the malformedness is evident during flow extraction. Might save
you a few cycles/more secure.
On March 4, 2017 6:18:54 AM GMT+01:00, Ben Pfaff wrote:
>On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote:
>> 2017-03-03 14:08 GMT-08:00 Ben Pfaff :
>> > Otherwise a malformed packet could cause a read up to about 40
>bytes past
>> > the end of the packet. The packet would still likely be dropped
>because
>> > of checksum verification.
>> >
>> > Reported-by: Bhargava Shastry
>> > Signed-off-by: Ben Pfaff
>>
>> Oops, thanks for the fix, Ben
>>
>> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
>>
>> One minor comment below,
>>
>> Acked-by: Daniele Di Proietto
>>
>>
>> > ---
>> > lib/conntrack.c | 14 --
>> > 1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/lib/conntrack.c b/lib/conntrack.c
>> > index 9bea3d93e4ad..9c1dd63648b8 100644
>> > --- a/lib/conntrack.c
>> > +++ b/lib/conntrack.c
>> > @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const
>void *data, size_t size,
>> > const char **new_data)
>> > {
>> > const struct ovs_16aligned_ip6_hdr *ip6 = data;
>> > +if (size < sizeof *ip6) {
>> > +return false;
>> > +}
>> > +
>>
>> We can read 'ip6->ip6_nxt' even though there's not enough data. It
>> cannot happen
>> for regular TCP and UDP packets (those are covered my
>> miniflow_extract), but only
>> when parsing the nested l3 header in an ICMP error message.
>>
>> The code has the same check two lines below, maybe we can reuse that.
>> Technically
>> the check is necessary only if new_data != NULL, as explained by the
>comment
>> above, but perhaps it's more clear to always perform it.
>
>Thanks for pointing out the duplicate check. I guess that I did not
>read the code carefully enough.
>
>Usually, I would argue to always do the check, but I prefer to make
>this
>a minimal change.
>
>I will post a v2.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
What bug do you see in the flow extract code?
On Sat, Mar 04, 2017 at 10:09:26AM +0100, Bhargava Shastry wrote:
> Hi Ben,
>
> Question regarding patch: Shouldn't the fix be applied in flow extract code
> itself? I think the malformedness is evident during flow extraction. Might
> save you a few cycles/more secure.
>
>
> On March 4, 2017 6:18:54 AM GMT+01:00, Ben Pfaff wrote:
> >On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote:
> >> 2017-03-03 14:08 GMT-08:00 Ben Pfaff :
> >> > Otherwise a malformed packet could cause a read up to about 40
> >bytes past
> >> > the end of the packet. The packet would still likely be dropped
> >because
> >> > of checksum verification.
> >> >
> >> > Reported-by: Bhargava Shastry
> >> > Signed-off-by: Ben Pfaff
> >>
> >> Oops, thanks for the fix, Ben
> >>
> >> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
> >>
> >> One minor comment below,
> >>
> >> Acked-by: Daniele Di Proietto
> >>
> >>
> >> > ---
> >> > lib/conntrack.c | 14 --
> >> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> >> > index 9bea3d93e4ad..9c1dd63648b8 100644
> >> > --- a/lib/conntrack.c
> >> > +++ b/lib/conntrack.c
> >> > @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const
> >void *data, size_t size,
> >> > const char **new_data)
> >> > {
> >> > const struct ovs_16aligned_ip6_hdr *ip6 = data;
> >> > +if (size < sizeof *ip6) {
> >> > +return false;
> >> > +}
> >> > +
> >>
> >> We can read 'ip6->ip6_nxt' even though there's not enough data. It
> >> cannot happen
> >> for regular TCP and UDP packets (those are covered my
> >> miniflow_extract), but only
> >> when parsing the nested l3 header in an ICMP error message.
> >>
> >> The code has the same check two lines below, maybe we can reuse that.
> >> Technically
> >> the check is necessary only if new_data != NULL, as explained by the
> >comment
> >> above, but perhaps it's more clear to always perform it.
> >
> >Thanks for pointing out the duplicate check. I guess that I did not
> >read the code carefully enough.
> >
> >Usually, I would argue to always do the check, but I prefer to make
> >this
> >a minimal change.
> >
> >I will post a v2.
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
My point is "miniflow_extract" has these checks that indicate a failed
parsing attempt for the packets in question. For example,
```C
else if (OVS_LIKELY(nw_proto == IPPROTO_ICMP)) {
if (OVS_LIKELY(size >= ICMP_HEADER_LEN)) {
do_something_with_valid_icmp_packet();
}
// Signaling of failed parsing attempt does not take place //
// i.e., no else corresponding to above predicate //
}
...
out:
dst->map = mf.map
```
So when you know that a packet is malformed during flow extraction
itself, why would you let the packet float around in your downstream
packet processing pipeline? Similar argument for malformed TCP/UDP packets.
I'm afraid I don't know the code well, but I feel that a failed parsing
attempt must be signaled by flow_extract _somehow_ and no subsequent
processing of the invalid packet should take place. This is because, in
the absence of a fail signal, downstream processing code (e.g.
conntrack) implicitly trusts all packets to be valid. Of course, fixing
the bug at conntrack will close the specific holes in point, but there
may be bugs elsewhere that share the same root cause. Am I making sense?
Regards,
Bhargava
On 03/04/2017 05:03 PM, Ben Pfaff wrote:
> What bug do you see in the flow extract code?
>
> On Sat, Mar 04, 2017 at 10:09:26AM +0100, Bhargava Shastry wrote:
>> Hi Ben,
>>
>> Question regarding patch: Shouldn't the fix be applied in flow extract code
>> itself? I think the malformedness is evident during flow extraction. Might
>> save you a few cycles/more secure.
>>
>>
>> On March 4, 2017 6:18:54 AM GMT+01:00, Ben Pfaff wrote:
>>> On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote:
2017-03-03 14:08 GMT-08:00 Ben Pfaff :
> Otherwise a malformed packet could cause a read up to about 40
>>> bytes past
> the end of the packet. The packet would still likely be dropped
>>> because
> of checksum verification.
>
> Reported-by: Bhargava Shastry
> Signed-off-by: Ben Pfaff
Oops, thanks for the fix, Ben
Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
One minor comment below,
Acked-by: Daniele Di Proietto
> ---
> lib/conntrack.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 9bea3d93e4ad..9c1dd63648b8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const
>>> void *data, size_t size,
> const char **new_data)
> {
> const struct ovs_16aligned_ip6_hdr *ip6 = data;
> +if (size < sizeof *ip6) {
> +return false;
> +}
> +
We can read 'ip6->ip6_nxt' even though there's not enough data. It
cannot happen
for regular TCP and UDP packets (those are covered my
miniflow_extract), but only
when parsing the nested l3 header in an ICMP error message.
The code has the same check two lines below, maybe we can reuse that.
Technically
the check is necessary only if new_data != NULL, as explained by the
>>> comment
above, but perhaps it's more clear to always perform it.
>>>
>>> Thanks for pointing out the duplicate check. I guess that I did not
>>> read the code carefully enough.
>>>
>>> Usually, I would argue to always do the check, but I prefer to make
>>> this
>>> a minimal change.
>>>
>>> I will post a v2.
>>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Bhargava Shastry
Security in Telecommunications
TU Berlin / Telekom Innovation Laboratories
Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany
phone: +49 30 8353 58235
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
On Sat, Mar 04, 2017 at 08:32:39PM +0100, Bhargava Shastry wrote:
> My point is "miniflow_extract" has these checks that indicate a failed
> parsing attempt for the packets in question. For example,
>
> ```C
> else if (OVS_LIKELY(nw_proto == IPPROTO_ICMP)) {
>if (OVS_LIKELY(size >= ICMP_HEADER_LEN)) {
> do_something_with_valid_icmp_packet();
>}
>// Signaling of failed parsing attempt does not take place //
>// i.e., no else corresponding to above predicate //
> }
>
> ...
>
> out:
>dst->map = mf.map
> ```
>
> So when you know that a packet is malformed during flow extraction
> itself, why would you let the packet float around in your downstream
> packet processing pipeline? Similar argument for malformed TCP/UDP packets.
OVS isn't just a firewall. It's also a switch that should be able to
handle any packet, not just the ones that pass some kind of firewall
check.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
