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

[Noam Postavsky <npostavs@gmail.com>]

Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:

> I've manually tested this patch a bit, but please give this patch a
> look and see if I've missed anything. I need all the feedbacks I can
> get for this.

Overall, I'd say this looks pretty good.  Some (mostly minor) comments
on the details below.

> * lisp/net/nsm.el
> (nsm-check-certificate, nsm-fingerprint-ok-p,
> nsm-check-plain-connection): Pre-format query messages before passing

It should be formatted as

(nsm-check-certificate, nsm-fingerprint-ok-p)
(nsm-check-plain-connection): Pre-format query messages before passing

> (nsm-protocol-check--diffie-hellman-prime-bits): Rename to
> nsm-protocol-check--dhe-kx. Checks for prime bits < 1024 for 'medium
                             ^

Periods should be double spaced, this applies in docstrings as well.

> nsm-protocol-check--rc4-cipher. Fix bug where it was previously
> checking for non-existent cipher name RC4 in GnuTLS instead of
> ARCFOUR.

Yikes, that's a good catch.

>  (defvar network-security-protocol-checks
> +  '((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))

> @@ -198,87 +207,370 @@ network-security-protocol-checks
>  HOST PORT STATUS OPTIONAL-PARAMETER.")
>  
>  (defun nsm-check-protocol (process host port status settings)
> +  (let ((results
> +         (cl-remove-if-not
> +          #'cdr
> +          (cl-loop for check in network-security-protocol-checks

This cl-remove-if-not over a cl-loop collect seems a bit awkward.  How
about

(cl-loop for (name level . _) in network-security-protocol-checks
         for type = (intern (format ":%s" name))
         ;; Skip the check if the user has already said that this
         ;; host is OK for this type of "error".
         for result =
         (and (not (memq type (plist-get settings :conditions)))
              (>= (nsm-level network-security-level)
                  (nsm-level level))
              (funcall (intern (format "nsm-protocol-check--%s" name))
                       host port status))
         when result
         collect (cons type result))

> +(defun nsm-protocol-check--dhe-kx (host port status)
> +  "Check for finite field ephemeral Diffie-Hellman key exchange.
> +
> +If `network-security-level' is 'medium, and a DHE key exchange
> +method was used, this function queries the user if the prime bit
> +length is < 1024.
> +
> +If `network-security-level' is 'high or above, and a DHE key
> +exchange method was used, this function queries the user even if
> +the prime bit length is >= 1024.

It's kind of inconvenient that this function hardcodes the security
levels; it also makes reading the current settings more difficult (e.g.,
when I saw (dhe-kx medium) at first, I thought you were going to warn
about DHE on level medium).  Can we do better here?  Maybe split in two?
(By the way, the network-security-level values in docstrings should be
formatted as `medium' and `high', not single quoted.)

> +In 2014, the discovery of Logjam[1] had proven non-elliptic-curve
> +Diffie-Hellman key exchange with < 1024 prime bit length to be
> +unsafe.

I'd actually say, DH smaller than 1024 bits was known to be unsafe
before that, the logjam attack allows a man-in-the-middle to downgrade
what would have been a >= 1024 bit connection to "export" grade (e.g.,
512 bits).

> +      (if (and (>= (nsm-level network-security-level) (nsm-level 'medium))
> +               (< prime-bits 1024))
> +          (setq msg (format-message
> +                     "Diffie-Hellman prime bits (%s) too low (%s)"

I would phrase this as

"Diffie-Hellman prime bits (%d) lower than `gnutls-min-prime-bits' (%d)"

> +                     prime-bits gnutls-min-prime-bits)))
> +      (if (>= (nsm-level network-security-level) (nsm-level 'high))
> +          (setq msg (concat
> +                     msg
> +                     (format-message
> +                      "non-elliptic-curve ephemeral Diffie-Hellman key exchange method (%s) maybe using an unsafe prime"

I would phrase this as

"non-standardized Diffie-Hellman parameters cannot be validated"

(this covers the non-elliptic-curveness as well; the reason elliptic
curves are safe is that they're standardized and pre-validated.)

And you're missing a space between the messages, in the case where you
hit both of them.

> +(defun nsm-protocol-check--anon-kx (host port status)
> +  "Check for anonymous key exchange.
> +
> +Anonymouse key exchange exposes the connection to MITM attacks.
> +
> +Reference:
> +
> +GnuTLS authors (2018). \"GnuTLS Manual 4.3.3 Anonymous
> +authentication\",
> +`https://www.gnutls.org/manual/gnutls.html\#Anonymous-authentication'"
                                             ^
                                             typo?

> +(defun nsm-protocol-check--export-kx (host port status)
> +  "Check for EXPORT key exchange.
> +
> +EXPORT cipher suites are a family of 40-bit effective security
> +algorithms legally exportable by the United States in the early 90s.
> +They can be broken in seconds on 2018 hardware.
> +
> +Recent version of GnuTLS does not enable this key exchange by default,

This should be "Recent versions of GnuTLS do not..."

> +but can be enabled if requested.  This check is mainly provided to
      ^
      it

> +;; Cipher checks
> +
> +(defun nsm-protocol-check--cbc-cipher (host port status)
> +  "Check for CBC mode ciphers.
> +
> +CBC mode cipher in TLS versions earlier than 1.3 are problematic
> +because of MAC-then-encrypt. This construction is vulnerable to
> +padding oracle attacks[1].

I think the TLS version reference should be dropped, unless TLS 1.3 uses
CBC with encrypt-then-MAC?  I understood it just deprecates CBC
altogether.

> +(defun nsm-protocol-check--3des-cipher (host port status)
> +  "Check for 3DES ciphers.
> +
> +3DES is considered a weak cipher by NIST as it only has 80 bits

Is it possible to distinguish between 3DES 2-key and 3DES 3-key? (the
latter giving 112 bit security, which is still a bit low, but probably
acceptable for medium level)