all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Noam Postavsky <npostavs@gmail.com>
To: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
Cc: Lars Ingebrigtsen <larsi@gnus.org>, 31946@debbugs.gnu.org
Subject: bug#31946: 27.0.50; The NSM should warn about more TLS problems
Date: Sat, 30 Jun 2018 16:30:40 -0400	[thread overview]
Message-ID: <87lgawm2z3.fsf@gmail.com> (raw)
In-Reply-To: <CAKDRQS4LWyfDB8Ao3ECNzPt6LWEHSa9_FaK_yhPtuMLgVXbdQA@mail.gmail.com> (Jimmy Yuen Ho Wong's message of "Sat, 30 Jun 2018 18:28:41 +0100")

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)






  reply	other threads:[~2018-06-30 20:30 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-23 10:38 bug#31946: 27.0.50; The NSM should warn about more TLS problems Lars Ingebrigtsen
2018-06-24 13:43 ` Lars Ingebrigtsen
2018-06-26  1:23 ` Noam Postavsky
2018-06-26  4:11   ` Jimmy Yuen Ho Wong
2018-06-26  6:26     ` Jimmy Yuen Ho Wong
2018-06-26 12:44       ` Noam Postavsky
2018-06-26 14:38       ` Eli Zaretskii
2018-07-03 15:08       ` Lars Ingebrigtsen
2018-07-04  1:34         ` Noam Postavsky
2018-07-08 14:20           ` Lars Ingebrigtsen
2018-07-08 14:21           ` Lars Ingebrigtsen
2018-07-08 14:22           ` Lars Ingebrigtsen
2018-07-08 18:34             ` Lars Ingebrigtsen
2018-07-08 18:50               ` Noam Postavsky
2018-07-08 20:01                 ` Lars Ingebrigtsen
2018-07-10  2:04                   ` Noam Postavsky
2018-07-22 11:05                     ` Lars Ingebrigtsen
2018-07-29 14:45                       ` Noam Postavsky
2018-07-08 18:36             ` Lars Ingebrigtsen
2018-07-08 19:20               ` Lars Ingebrigtsen
2018-07-08 14:23           ` Lars Ingebrigtsen
2018-06-26  9:27   ` Lars Ingebrigtsen
2018-06-26 14:42     ` Eli Zaretskii
2018-06-27  0:45       ` Noam Postavsky
2018-06-27  5:09         ` Jimmy Yuen Ho Wong
2018-06-27 12:07           ` Lars Ingebrigtsen
2018-06-27 12:20             ` Lars Ingebrigtsen
2018-06-27 15:16               ` Eli Zaretskii
2018-06-27 16:40                 ` Jimmy Yuen Ho Wong
2018-06-28 16:10                 ` Lars Ingebrigtsen
2018-06-28  0:14           ` Noam Postavsky
2018-06-28 15:58             ` Jimmy Yuen Ho Wong
2018-06-28 16:15               ` Lars Ingebrigtsen
2018-06-28 16:42                 ` Jimmy Yuen Ho Wong
2018-06-28 17:01                   ` Lars Ingebrigtsen
2018-06-28 18:15                     ` Jimmy Yuen Ho Wong
2018-06-29  5:21                       ` Jimmy Yuen Ho Wong
2018-06-29  5:25                         ` Jimmy Yuen Ho Wong
2018-06-30 17:28                           ` Jimmy Yuen Ho Wong
2018-06-30 20:30                             ` Noam Postavsky [this message]
2018-06-30 23:15                               ` Jimmy Yuen Ho Wong
2018-06-30 23:35                                 ` Noam Postavsky
2018-07-01 15:01                             ` Eli Zaretskii
2019-08-26  4:34 ` Lars Ingebrigtsen
2019-08-26 13:13   ` Andy Moreton
2019-08-26 16:07     ` Eli Zaretskii
2019-08-26 17:45       ` Andy Moreton
2019-08-26 18:12         ` Eli Zaretskii
2019-08-26 18:19           ` Andy Moreton
2019-08-26 19:36             ` Eli Zaretskii
2019-08-26 22:56               ` Andy Moreton
2019-08-27  7:23                 ` Eli Zaretskii
2019-08-27 15:25                   ` Andy Moreton
2019-08-27 15:50                     ` Eli Zaretskii
2019-08-27  8:01 ` bug#31946: 27.0.50; GnuTLS still crashes on MinGW Markus Weber
2019-08-27 15:00   ` Eli Zaretskii
2019-08-27 15:50     ` Eli Zaretskii
2019-08-29  8:05       ` mw
2019-08-29 12:20         ` Eli Zaretskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lgawm2z3.fsf@gmail.com \
    --to=npostavs@gmail.com \
    --cc=31946@debbugs.gnu.org \
    --cc=larsi@gnus.org \
    --cc=wyuenho@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.