[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#31946: 27.0.50; The NSM should warn about more TLS problems
From: |
Noam Postavsky |
Subject: |
bug#31946: 27.0.50; The NSM should warn about more TLS problems |
Date: |
Sat, 30 Jun 2018 16:30:40 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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)
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, (continued)
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Lars Ingebrigtsen, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Noam Postavsky, 2018/06/27
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Lars Ingebrigtsen, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Lars Ingebrigtsen, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/28
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/29
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/29
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/30
- bug#31946: 27.0.50; The NSM should warn about more TLS problems,
Noam Postavsky <=
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Jimmy Yuen Ho Wong, 2018/06/30
- bug#31946: 27.0.50; The NSM should warn about more TLS problems, Noam Postavsky, 2018/06/30