bug#31946: 27.0.50; The NSM should warn about more TLS problems

[Eli Zaretskii <eliz@gnu.org>]

> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Sat, 30 Jun 2018 18:28:41 +0100
> Cc: Noam Postavsky <npostavs@gmail.com>, Eli Zaretskii <eliz@gnu.org>, 31946@debbugs.gnu.org
> 
> Here's the patch promised.
> 
> Summary of changes:
> 
> * 9 new checks added, some for securing Emacs builts with older GnuTLS
> versions, some for bringing NSM checks to 2018.
> * Individual checks are now suffixe'd by their cipher suite part. E.g.
> dhe-kx for DHE key exchange, des-cipher for DES cipher, sha1-sig for
> SHA1 signature.
> * Protocal checks now return an error message instead of querying the
> user directly.
> * All protocol checks under the same network-security-level are
> performed. All of the messages are then merged if any, and the user is
> only prompt once after the protocol check for all problems found.
> * nsm-query and nsm-query-user no longer takes extra args to format
> messages. Formatted messages are now provided directly by the
> individual checks.
> * Fix RC4 check where it was previously checking for non-existent RC4
> cipher from GnuTLS. GnuTLS actually calls RC4 ARCFOUR.
> * Removed sha1 check as it is already covered by the intermediate SHA1 checks.
> * DHE check now checks for < 1024 bit prime for 'medium and usage of
> DHE for 'high and above, in addition to prime bit length.

Just looking at this with somewhat naïve eyes of a user who knows very
little about security features, I'm bothered that we add so many
checks to the 'medium' level, only 2 to 'high', and _none_ to
'paranoid'.  Since 'medium' is the lowest level that provides _any_
security features, does having 12 out of 14 checks in 'medium' really
make sense?  Do other browsers offer the same features on the lowest
security level?  Aren't some of the vulnerabilities less frequent
and/or less dangerous, in which case it would make sense to move them
to higher levels?  Or maybe we should introduce an intermediate level
between 'medium' and 'high', and move some of these new checks into
it?

That's my main concern about this and other similar changes.

The next concern is about documentation: IMO such a massive upgrade of
security needs to document the checks, in the source if not in the
Emacs manual.  We must give our users tools to make informed decisions
regarding which security measures are good for them.  This patch comes
just with doc strings, which IMO is not enough: NEWS and the manual
should also be updated.  As for doc strings, see comments below.

>  (defvar network-security-protocol-checks
> -  '((diffie-hellman-prime-bits medium 1024)
> -    (rc4 medium)
> -    (signature-sha1 medium)
> -    (intermediate-sha1 medium)
> -    (3des high)
> +  '((rsa-kx high)
> +    (dhe-kx medium)
> +    (anon-kx medium)
> +    (export-kx medium)
> +    (cbc-cipher high)
> +    (ecdsa-cbc-cipher medium)
> +    (3des-cipher medium)
> +    (des-cipher medium)
> +    (rc4-cipher medium)
> +    (rc2-cipher medium)
> +    (null-cipher medium)
> +    (sha1-sig medium)
> +    (md5-sig medium)
>      (ssl medium))
>    "This variable specifies what TLS connection checks to perform.

Either each test should be documented right here, or the doc string of
this variable should refer the reader to the respective functions,
explaining how to deduce the function name from the test name.

> +(defun nsm-protocol-check--rsa-kx (host port status)
> +  "Check for static RSA key exchange.
> +
> +Static RSA key exchange methods do not offer perfect forward
> +secrecy.
> +
> +Reference:
> +
> +IETF TLSWG (2014). \"[TLS] Confirming Consensus on removing RSA key
> +Transport from TLS 1.3\",
> +`https://www.ietf.org/mail-archive/web/tls/current/msg11621.html'"

A reference is fine, but it alone is not enough: we cannot expect
users to read academic papers just to decide what security they need.
IMO, we should say at least a few words about each test, enough for
the user to understand whether they need this test.  It may be enough
to say just how frequent and/or dangerous is the corresponding
vulnerability; adding the description of a vulnerability with some
objective assessment of its relevance would be a bonus.

> +Recent version of GnuTLS does not enable this key exchange by default,
> +but can be enabled if requested.  This check is mainly provided to
> +secure Emacs built with older version of GnuTLS.

Please state the first version of GnuTLS that disables this by
default: what is "recent" today will become less so in a year.  I'd
like us to give a user who knows what version of GnuTLS she has
installed an effective way of deciding whether this test is needed.

> +RC4 cipher has been prohibited by RFC 7465.
> +
> +Recent version of GnuTLS does not enable this cipher by default, but
> +can be enabled if requested.  This check is mainly provided to secure
> +Emacs built with older version of GnuTLS.

Likewise here (and elsewhere in the patch).

> +The first SHA1 collision was found in 2017[1], as a precaution against
> +events followed the discovery of cheap collisions in MD5, major
> +browsers[2][3][4] have sunsetted the use of SHA1 signatures in
> +certificates.     ^^^^^^^^^^^^^^

Please try to avoid using jargon that may be difficult for non-native
English speakers to understand.  Security issues should not be
obscured by obscure language.

Thanks again for working on this.