[ovs-dev] [PATCH 1/1] datapath: add key support to CAPWAP tunnel

[Jesse Gross]

On Tue, Aug 2, 2011 at 12:01 PM, Simon Horman <horms at verge.net.au> wrote:
> Hi,
>
> in an effort to move things forward I decided to take the liberty
> of posting a fresh version of this patch which addresses each
> of the issues that Jesse raises below.

Thanks for picking this up Simon.

> On Fri, Jul 22, 2011 at 02:18:47PM -0700, Jesse Gross wrote:
>> On Wed, Jul 13, 2011 at 10:05 PM, Valient Gough <vgough at pobox.com> wrote:
>> > diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
>> > index f0bb327..161c6d4 100644
>> > --- a/datapath/vport-capwap.c
>> > +++ b/datapath/vport-capwap.c
>> > +/*
>> > + * Should be sized as multiple of u32, for hash functions.
>> > + */
>> >  struct frag_match {
>> >        __be32 saddr;
>> >        __be32 daddr;
>> >        __be16 id;
>> > +       __be16 reserved;
>> > +       __be64 key;
>> >  };
>>
>> I think this violates the CAPWAP spec because it changes the
>> parameters that reassembly is based on.  A device that understands
>> keys can now put together packets in a different way than one that
>> does not if there are overlapping IDs.  If you go down the route of
>> checking that all of the fragments have the same ID, I think you need
>> to do reassembly as before and check to see whether all segments have
>> the same key and drop if not.  Probably the simpler implementation
>> would be to just declare that only the key in the first fragment
>> matters.
>
> My reading is that can be achieved by simply not
> adding reserved and key to struct frag_match, not
> setting those values in defrag() and not comparing key
> in capwap_frag_match(). Is that correct?

I believe that this is similar to what Valient had in his original
patch.  My concern is that it is somewhat non-deterministic as it will
take the key from the last fragment that arrived.  IPv4 has security
problems with fragment reassembly by being somewhat loose in the
specification of how invalid packets should be handled.  This led to
problems because different implementations would do different things
and it would be possible to slip attacks past a security appliance by
taking advantage of these differences if, for example, the appliance
was running Linux and the target ran Windows.  IPv6 addresses this by
prescribing the exact behavior and specifying that anything else must
be dropped.  Since we're effectively defining a new protocol here, I
want to avoid the mistakes of the past by not leaving things up to
chance.  I think if we simply moved the key extraction after
reassembly then we can avoid this by mandating that it is the first
key in the stream that is used.

> I have reworked process_capwap_wsi() as follows which
> I believe addresses both the potentially invalid pointer and
> incorrect length check issues.
>
> static int process_capwap_wsi(struct sk_buff *skb, __be64 *key, int *hdr_len)
> {
>        struct capwaphdr *cwh;
>        struct capwaphdr_wsi *wsi;
>        int min_wsi_len = sizeof(struct capwaphdr_wsi);
>        int wsi_len;
>
>        if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + min_wsi_len + ETH_HLEN)))
>                return -ENOMEM;
>
>        /* read wsi header to find out how big it really is */
>        cwh = capwap_hdr(skb);
>        wsi = (struct capwaphdr_wsi *)(cwh + 1);
>        /* +1 for length byte not included in wsi_len */
>        wsi_len = 1 + (unsigned int)wsi->wsi_len;
>        *hdr_len += wsi_len;
>
>        /* parse wsi field */
>        if (wsi_len > min_wsi_len && wsi->flags & CAPWAP_WSI_F_KEY64) {
>                struct capwaphdr_wsi_key *opt;
>
>                if (unlikely(wsi_len < min_wsi_len + sizeof(struct capwaphdr_wsi_key)))
>                        return -EINVAL;

Depending on the length we'll get different results for invalid
packets - the WSI header is too short for anything we will accept the
packet but won't get a key.  On the other hand, if the header is above
the minimum length but too short for the key that it indicates is
there then we will reject the packet.  I think for consistency we
should reject all invalid packets.

>> > +static inline struct sk_buff *process_capwap_proto(struct sk_buff *skb,
>> > +                                                  __be64 *key)
>> > +{
>> > +       struct capwaphdr *cwh = capwap_hdr(skb);
>> > +       int hdr_len = CAPWAP_MIN_HLEN;
>> > +       int wbid = cwh->begin & CAPWAP_WBID_MASK;
>>
>> This triggers sparse errors; it can be fixed by using __be32 instead.
>
> Done.

One other thing that I noticed in this function is that we have
somewhat inconsistent handling regarding the header length.  If it is
using the new format we ignore the header length, always pulling the
base header length plus the WSI length.  If it is using the old format
then we require a specific length.  Since we're enforcing a specific
set of flags probably we should check the length in the new format.
Either that or learn to skip header fields that we don't understand.

>> > @@ -628,7 +768,8 @@ static int capwap_frag_match(struct inet_frag_queue *ifq, void *a_)
>> >        struct frag_match *a = a_;
>> >        struct frag_match *b = &ifq_cast(ifq)->match;
>> >
>> > -       return a->id == b->id && a->saddr == b->saddr && a->daddr == b->daddr;
>> > +       return a->id == b->id && a->saddr == b->saddr &&
>> > +               a->daddr == b->daddr && a->key == b->key;
>>
>> Might as well use memcmp() here now that the struct won't have any
>> extra padding in it.
>
> Done.

This only works with the extra padding that was introduced in previous
patch.  With the current struct layout the compiler will insert some
padding composed of uninitialized memory that will lead to comparison
failures.