| Message ID | 20170303220829.22050-1-blp@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
2017-03-03 14:08 GMT-08:00 Ben Pfaff <blp@ovn.org>: > 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 <bshastry@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> Oops, thanks for the fix, Ben Fixes: a489b16854b5("conntrack: New userspace connection tracker.") One minor comment below, Acked-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote: > 2017-03-03 14:08 GMT-08:00 Ben Pfaff <blp@ovn.org>: > > 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 <bshastry@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Oops, thanks for the fix, Ben > > Fixes: a489b16854b5("conntrack: New userspace connection tracker.") > > One minor comment below, > > Acked-by: Daniele Di Proietto <diproiettod@vmware.com> > > > > --- > > 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.
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 <blp@ovn.org> 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 <blp@ovn.org>: >> > 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 <bshastry@sec.t-labs.tu-berlin.de> >> > Signed-off-by: Ben Pfaff <blp@ovn.org> >> >> Oops, thanks for the fix, Ben >> >> Fixes: a489b16854b5("conntrack: New userspace connection tracker.") >> >> One minor comment below, >> >> Acked-by: Daniele Di Proietto <diproiettod@vmware.com> >> >> >> > --- >> > 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.
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 <blp@ovn.org> 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 <blp@ovn.org>: > >> > 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 <bshastry@sec.t-labs.tu-berlin.de> > >> > Signed-off-by: Ben Pfaff <blp@ovn.org> > >> > >> Oops, thanks for the fix, Ben > >> > >> Fixes: a489b16854b5("conntrack: New userspace connection tracker.") > >> > >> One minor comment below, > >> > >> Acked-by: Daniele Di Proietto <diproiettod@vmware.com> > >> > >> > >> > --- > >> > 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.
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 <blp@ovn.org> 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 <blp@ovn.org>:
>>>>> 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 <bshastry@sec.t-labs.tu-berlin.de>
>>>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>>>
>>>> Oops, thanks for the fix, Ben
>>>>
>>>> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
>>>>
>>>> One minor comment below,
>>>>
>>>> Acked-by: Daniele Di Proietto <diproiettod@vmware.com>
>>>>
>>>>
>>>>> ---
>>>>> 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.
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.
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; }
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 <bshastry@sec.t-labs.tu-berlin.de> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/conntrack.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)