[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