netdev - Re: IPsec: Why do pfkey_getspi and xfrm_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 24 Sep 2010 20:57:47 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	christophe.gouault@...nd.com
Cc:	netdev@...r.kernel.org
Subject: Re: IPsec: Why do pfkey_getspi and xfrm_alloc_userspi call

From: Christophe Gouault <christophe.gouault@...nd.com>
Date: Mon, 13 Sep 2010 14:31:28 +0200

> I guess the larval state found by xfrm_get_acq_byseq must have the
> same parameters as those provided in the message (mode, reqid, proto,
> daddr, saddr, family). Contrary to what one might think, the call to
> xfrm_get_acq_byseq is more costly than the call to xfrm_find_acq,
> because the later uses a hash table.

Whether any such requirement exists, we certainly have never enforced
something like that.

I tried to look for guidance in some other PFKEYV2 implementations,
but those I looked at (OpenBSD for example) are even more permissive
than we are.

For example, OpenBSD doesn't validate the incoming sequence number at
all as far as making sure it matches the one emitted for the acquire
message.

In fact it's approach to handling this sequence number is completely
different from ours.  It treats it truly as a unique 32-bit cookie
which is used entirely by the user.  When an acquire is emitted, it
does not create an ipsec database entry, instead it waits until the
GETSPI happens (see reserve_spi()).

This lack of consistency for enforcement and validation amongst
implementations makes it really difficult to say what might or might
not be safe to do here.

> Shouldn't we extend the xfrm_get_acq function so that it accepts an
> optional seq parameter? We would replace the first call to
> xfrm_find_acq_byseq by:
> 
>    x = xfrm_find_acq(mode, reqid, proto, xdaddr, xsaddr, 0, family, seq);
> 
> and the if no entry is found, we would call:
> 
>    x = xfrm_find_acq(mode, reqid, proto, xdaddr, xsaddr, 1, family, 0);
> 
> We would take benefit of the hash table, instead of looking up through
> the whole SAD as does xfrm_get_acq_byseq.

Even with all of the above, I do think this suggestion of your's is
reasonable.

But please, this function has too many arguments already, find a way
to do it without adding new ones.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html