[PATCH] DTLS: Add ECDHE-RSA-AES256-SHA384 as a v1.2 cipher suite

[Daniel Lenski]

On Fri, Jul 31, 2020 at 4:00 PM Jason Gunthorpe <jgg at ziepe.ca> wrote:
>
> On Fri, Jul 31, 2020 at 02:41:46PM -0700, Daniel Lenski wrote:
> > On Fri, Jul 31, 2020 at 2:19 PM Nikos Mavrogiannopoulos
> > <n.mavrogiannopoulos at gmail.com> wrote:
> > >
> > > On Thu, Jul 30, 2020 at 10:00 PM Jason Gunthorpe <jgg at ziepe.ca> wrote:
> > > >
> > > > If GCM is not available on the VPN server this is a reasonable fallback.
> > > >
> > > > Severs will not auto-fallback to older TLS if the X-DTLS12-CipherSuite is
> > > > sent, so the existing non-GCM modes with the old TLS do not negotiate.
> > >
> > > In terms of security that's super ugly. All these CBC ciphersuites are
> > > problematic in TLS1.2 due to lucky13 attacks; TLS1.3 dropped all of
> > > them. It is simply too hard to make them secure and that's why they
> > > are disabled by default in openconnect. Not probably helpful solution,
> > > but you may want to refer your IT to good advise at:
> > > https://bettercrypto.org/
> >
> > I believe that the only situation where this would actually *lower*
> > the security of a connection would be the case where a server supports
> > *both* GCM and CBC for DTLS 1.2 ciphers, but (for some insane reason)
> > chooses the CBC cipher when offered both options.
> >
> > Do I have that right?
>
> I think so.
>
> It is already the case that openconnect will accept < TLS1.2 ciphers
> including CBC versions, so I'm not sure how adding them to the TLS1.2
> list makes anything less secure?

Right. To be clear, your patch is modifying *both* …

1) the TLS ciphers list (to allow SHA384 as MAC; IMO this should have
already been included alongside +SHA256 in
https://gitlab.com/openconnect/openconnect/-/commit/5a3f242e7f778836f1645fb6479953e369a8f81e)
2) the DTLS v1.2 ciphers list (to allow one specific cipher suite
there: ECDHE-RSA-AES256-SHA384)

We already have a way for the user to override (1) for corner cases
like yours. In https://gitlab.com/openconnect/openconnect/-/merge_requests/71,
I added the `--gnutls-priority` option specifically to be used for
cases like yours, so recompilation isn't necessary.

However, we *don't* have a good way to override (2) dynamically, at
least not for Cisco servers, because the DTLS cipher "negotiation"
(scare quotes intentional) happens in such a weird non-standard way.

> Arguably openconnect could do with a --secure-crypto option that used
> only good stuff to prevent any possible downgrade attack.

We already have --pfs (which requires that only forward secrecy
ciphersuites be used, e.g. KX != RSA) and I have a merge request out
there (https://gitlab.com/openconnect/openconnect/-/merge_requests/114)
to add --allow-insecure-crypto, which is for
realllllllllllllllllllllly old realllllllllllllllllllllly insecure
stuff like 3DES which some super ancient but still functional
unpatched Cisco devices require.

> Anyhow, the other work around is to use the --dtls-ciphers option to
> switch into < 1.2 mode which still has the lucky13 problem and a whole
> bunch of other issues too.
>
> IT says that Cisco told them to disable GCM as it has some bug. It
> will come back eventually when they get a fix. In the mean
> time having a VPN would be nice, and the Windows client negotiates
> this suite as the choice next in line.

Seems… questionable, but there are lots of questionable decisions made
by VPN administrators which we can't really fix and shouldn't make
unnecessarily difficult for end-users IMO.

I'm in favor of merging this patch since I don't see any cases where
it silently *degrades* security. (One other thing I'd note is that
this DTLS cipher should *already* be supported *anyway* if you build
OpenConnect with OpenSSL instead of GnuTLS.)

Dan