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.
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.
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.
> > 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;
> >> > +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.
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;
>
> >> > @@ -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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
