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

[Jimmy Yuen Ho Wong <wyuenho@gmail.com>]

Thanks. I'll update the patch later.

I'm having a headache about what to do with the prompts now.
Currently, if a cert didn't verify, there's a prompt. If a cert did
verify but was "changed", I get another prompt. But "change" is
determined by a public key ID hash instead of the actual fingerprint
(but `nsm-fingerprint` treat the public key ID as the fingerprint),
and the prompt happens *before* cipher suite checks, in which you may
get another prompt.

I think I need to do another pass at this to get the check ordering
right so I can get just one prompt for all problems found.

Basically, I'm trying to break the certificate check into multiple
checks (I'll do that for DHE too) and figure out what to do with the
fingerprint. Let me know if I'm going way off track here...



On Sat, Jun 30, 2018 at 9:30 PM, Noam Postavsky <npostavs@gmail.com> wrote:
> 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)
>