[ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.
Ben Pfaff
blp at ovn.org
Sat Mar 4 05:18:54 UTC 2017
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 at 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 at sec.t-labs.tu-berlin.de>
> > Signed-off-by: Ben Pfaff <blp at 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 at 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.
More information about the dev
mailing list