RE: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API

[Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>]

> > In this case, the relevance is that the handshake in WireGuard is
> > extremely performance sensitive, in order to fend off DoS. One of the
> > big design gambits in WireGuard is – can we make it 1-RTT to reduce
> > the complexity of the state machine, but keep the crypto efficient
> > enough that this is still safe to do from a DoS perspective. The
> > protocol succeeds at this goal, but in many ways, just by a hair when
> > at scale, and so I’m really quite loathe to decrease handshake
> > performance.
> ...
> > Taken together, we simply can’t skimp on the implementations available
> > on the handshake layer, so we’ll need to add some form of
> > implementation selection, whether it’s the method Zinc uses ([2]), or
> > something else we cook up together.
> >
> 
> So are you saying that the handshake timing constraints in the
> WireGuard protocol are so stringent that we can't run it securely on,
> e.g., an ARM CPU that lacks a NEON unit? Or given that you are not
> providing accelerated implementations of blake2s or Curve25519 for
> arm64, we can't run it securely on arm64 at all?
> 
> Typically, I would prefer to only introduce different versions of the
> same algorithm if there is a clear performance benefit for an actual
> use case.
> 
> Framing this as a security issue rather than a performance issue is
> slightly disingenuous, since people are less likely to challenge it.
> But the security of any VPN protocol worth its salt should not hinge
> on the performance delta between the reference C code and a version
> that was optimized for a particular CPU.
> 
Fully agree with that last statement. Security of a protocol should
*never* depend on the performance of a particular implementation.

I may want to run this on a very constrained embedded system that
would necessarily be very slow, and I would still want that to be
secure. If this is true, it's pretty much a deal-breaker to me ...

Which would be a shame, because I really do like some of the other
things Wireguard does and just the effort of improving VPN in general.

> > Issue 2) Linus’ objection to the async API invasion is more correct
> > than he realizes.
> >
> > I could re-enumerate my objections to the API there, but I think we
> > all get it. It’s horrendous looking. Even the introduction of the
> > ivpad member (what on earth?) in the skb cb made me shutter.
> 
> Your implementation of RFC7539 truncates the nonce to 64-bits, while
> RFC7539 defines a clear purpose for the bits you omit. Since the Zinc
> library is intended to be standalone (and you are proposing its use in
> other places, like big_keys.c), you might want to document your
> justification for doing so in the general case, instead of ridiculing
> the code I needed to write to work around this limitation.
> 
From RFC7539:
"Some protocols may have unique per-invocation inputs that are not 96
 bits in length.  For example, IPsec may specify a 64-bit nonce.  In
 such a case, it is up to the protocol document to define how to
 transform the protocol nonce into a 96-bit nonce, <<for example, by
 concatenating a constant value.>>"

So concatenating zeroes within the protocol is fine (if you can live
with the security consequences) but a generic library function should
of course take all 96 bits as input(!) Actually, the rfc7539esp variant
already takes that part of the nonce from the key, not the IV. This
may be more convenient for use with Wireguard as well? Just force the
trailing nonce portion of the key to zeroes when calling setkey().

> 
> My preference would be to address this by permitting per-request keys
> in the AEAD layer. That way, we can instantiate the transform only
> once, and just invoke it with the appropriate key on the hot path (and
> avoid any per-keypair allocations)
> 
This part I do not really understand. Why would you need to allocate a
new transform if you change the key? Why can't you just call setkey()
on the already allocated transform?

> 
> It all depends on whether we are interested in supporting async
> accelerators or not, and it is clear what my position is on this
> point.
> 
Maybe not for an initial upstream, but it should be a longer-term goal.

> 
> What I *don't* want is to merge WireGuard with its own library based
> crypto now, and extend that later for async accelerators once people
> realize that we really do need that as well.
> 
What's wrong with a step-by-step approach though? i.e. merge it with
library calls now and then gradually work towards the goal of integrating
(a tweaked version of) the Crypto API where that actually makes sense?
Rome wasn't built in one day either ...

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com