On Tue, Aug 23, 2011 at 11:11 PM, Simon Horman <ho...@verge.net.au> wrote:
> On Tue, Aug 02, 2011 at 01:56:22PM +0700, Jesse Gross wrote:
>> On Tue, Aug 2, 2011 at 12:01 PM, Simon Horman <ho...@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 <vgo...@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.
>
> Thanks, I now understand :-)
>
> I'm a little unsure what a clean/efficient implementation would be
> as process_capwap_proto() is needed when a fragment is received
> in order to determine its length (assuming we don't trust cwh->begin &
> CAPWAP_HLEN_MASK) and it is now also needed to extract the key later on.
I think it's OK to trust (with appropriate checks) the header length
for the purposes of pulling it off, we just need to later verify that
the WSI length fits within that length. I think it's also desirable
to do this to have a little more flexible implementation that just
skips over unknown fields. Before I was concerned about being too
accepting (particularly in regards to version number and type) but I
think it's OK to skip the miscellaneous fields that we don't care
about (WSI was previously in this category) and using the header
length is necessary for this.
>
> The implementation I have gone for is to call process_capwap_proto()
> with a NULL key early on and process_capwap_proto() skips key
> extraction in that case.
I think that's probably OK but if I'm reading it correctly, it doesn't
look the code actually passes in NULL.
> It does also occur to me that it would be possible to get a mix
> of old and new format fragments. But as you're happy to ignore
> key miss-matches between fragments I think mixed formats should also be ok.
That's a good point. I think it's OK though as long as the behavior
is well defined.
>> > 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.
>
> Sorry for not spotting that earlier. I have changed the check around to:
>
> if (unlikely(wsi_len < min_wsi_len + sizeof(struct capwaphdr_wsi_key)
> ||
> !(wsi->flags & CAPWAP_WSI_F_KEY64)))
> return -EINVAL;
I think for future flexibility it's probably a good idea to allow a
WSI even if there is no key since WSI is essentially a generic
extension mechanism.
>> 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.
>
> I think that checking hdr_len against (cwh->begin & CAPWAP_HLEN_MASK) >> 17
> should make this consistent between formats.
>
> if (wbid == CAPWAP_WBID_30) {
> if ((cwh->begin & CAPWAP_F_WSI) && process_capwap_wsi(skb,
> key, &hdr_len))
> goto error;
> cwh = capwap_hdr(skb);
> } else if (wbid != CAPWAP_WBID_2)
> goto error;
>
> if (unlikely((cwh->begin & CAPWAP_HLEN_MASK) >> CAPWAP_HLEN_SHIFT !=
> hdr))
> goto error;
This looks good but I think there is a byte order problem when doing
shifting (before it was just doing exact comparisons so it didn't
matter).
>> >> > @@ -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.
>
> Oops. Undone.
I think we also need to do something similar for hashing.
In addition to the above I spotted a few other problems and did some
testing which uncovered some other things. To avoid bouncing this
around excessively, I took the liberty of making some changes myself.
The new version passes my tests and I think is ready to be committed.
I'll send it as a follow up message, can you guys take a look at it?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
