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

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

[Lars Ingebrigtsen]

This bug report is mostly just to remind myself, but if somebody else
wants to implement it -- go ahead.  :-)

I meant to refactor the protocol checks in nsm.el so that they're more
easily extensible and also allow the user more fine-grained control of
what protocol issues they care about.  Something like

(defvar network-security-tls-problems
        '((low-diffie-hellman-prime-bits medium)
          (rc4 low)
          (dh-small-subgroup high)))

or something, and then a separate function for each of these tests to
avoid an ever-growing huge function with a `cond' in it, and also allow
users to add their own tests.

---

There are also more protocol stuff we should warn about on various
levels.  These should be on `high':


>            "https://3des.badssl.com/"                     ;; fail
>            "https://mozilla-old.badssl.com/"              ;; fail
>            "https://dh-small-subgroup.badssl.com/"        ;; fail
>            "https://dh-composite.badssl.com/"             ;; fail
>            "https://invalid-expected-sct.badssl.com/"     ;; fail, a bit
> concerning

These should be on `medium':

>            "https://dh480.badssl.com/"                    ;; fail
>            "https://dh512.badssl.com/"                    ;; fail



In GNU Emacs 27.0.50 (build 20, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-05-19 built on stories
Repository revision: f4d9fd3dd45f767eca33fbf1beee40da790fa74e
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

I've now done the refactoring bit, and the behavior should hopefully be
identical to pre-rewrite.

The next thing that should be done is to adjust the levels for most of
the checks down to `medium', and then add some more checks from the

https://badssl.com/

list.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> There are also more protocol stuff we should warn about on various
> levels.  These should be on `high':

>>            "https://dh-small-subgroup.badssl.com/"        ;; fail
>>            "https://dh-composite.badssl.com/"             ;; fail

So these ones seem kind of problematic, as alluded to on emacs-devel.
It doesn't look like gnutls has an API to get or check the value of the
DH primes (calc-prime-test bails out when given a 1024 bit prime, so we
definitely need library support for this).

https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00805.html
https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00807.html

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

[Jimmy Yuen Ho Wong]

[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]

`dh-composite` can be mitigated by using the "NORMAL:%PROFILE_MEDIUM"
priority string[1], "NORMAL:%PROFILE_HIGH" [2] will pass all 26 badssl test
while still allowing connection to ELPA/MELPA without even supplying CRL
files (GnuTLS already does OCSP stapling verifcation transparently, and
Emacs is using it already minus surfacing `GNUTLS_CERT_MISSING_OCSP_STATUS`
when it fails). The exact meaning these levels appears to be spread out
among different tables in ENISA's Algorithms, Key Sizes and Parameters
Report - 2013 [3].

As a possible way to avoid confusion, I would suggest we consolidate the 2
different meaning of profiles (NSM and GnuTLS) into GnuTLS's. Instead of
having users to edit an alist like Lars has done in commit 6584bc67, we
could:

1. Append `network-security-level` to `gnutls-algorithm-priority`, i.e.
`network-security-level` will be a list of predefined symbols that will be
mapped to GnuTLS's `%PROFILE_*` strings, and append to it when setting up
`gnutls-boot-parameters`.
2. Forget about letting users decide whether they want to accept
problematic certs or not, no modern browsers does it anymore. Doing network
security checks in 2 different places also introduces impedance mismatch.
Specifically, GnuTLS by default disables a number of cyphers and hashes.
The only way to stop it from generating fatal alerts is to enable
everything GnuTLS has implemented and reinvent all the wheels in LISP (do
you really want to reenable SSL3?). This is insane from both a security and
performance perspective, as we don't have reliable NETSEC resources to
respond to any security issues that we may introduce during the process.
Even if we do, there's a larger problem of Emacs's release process.
3. To solve the problem of letting users fine tune the client's acceptable
cyphersuite, MACs and whatnot for emergencies out of Emacs' release cycles,
let's introduce a bunch of new defcustoms such as `gnutls-cyphersuite`,
`gnutls-key-exchange` etc, see [1] for the table.
4. Normally, the fine tuning defcustoms in 3) will be nil, in which case
`gnutls-algorithm-priority` takes precedence, otherwise they are combined
into a final priority string supplied to `gnutls-boot-parameters`.
5. Merge nsm into the gnutls group. No more distinction between interactive
and non-interactive sessions due to 2).

References:
[1]: https://gnutls.org/manual/html_node/Priority-Strings.html
<https://gnutls.org/manual/html_node/Priority-Strings.html>
[2]: https://gnutls.org/manual/html_node/Selecting-cryptographic-
key-sizes.html#tab_003akey_002dsizes
<https://gnutls.org/manual/html_node/Selecting-cryptographic-key-sizes.html#tab_003akey_002dsizes>
[3]: https://www.enisa.europa.eu/publications/algorithms-key-
sizes-and-parameters-report
<https://www.enisa.europa.eu/publications/algorithms-key-sizes-and-parameters-report>

On Tue, Jun 26, 2018 at 2:23 AM, Noam Postavsky <npostavs@gmail.com> wrote:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
> > There are also more protocol stuff we should warn about on various
> > levels.  These should be on `high':
>
> >>            "https://dh-small-subgroup.badssl.com/"        ;; fail
> >>            "https://dh-composite.badssl.com/"             ;; fail
>
> So these ones seem kind of problematic, as alluded to on emacs-devel.
> It doesn't look like gnutls has an API to get or check the value of the
> DH primes (calc-prime-test bails out when given a 1024 bit prime, so we
> definitely need library support for this).
>
> https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00805.html
> https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00807.html
>
>
>

[-- Attachment #2: Type: text/html, Size: 4875 bytes --]

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

[Jimmy Yuen Ho Wong]

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]

Sorry I was confused in my last reply about modern browsers not allowing
you to accept certs. Chrome just hides that functionality really well, so
forget about my proposal earlier. (one should not reply to emails at 5 in
the morning) Here's my new proposal:

1. Forget about defining what tests belongs in what levels, there should
just be one level which is the default sets of tests, let's call this
coarse grain setting.
2. Fine grain settings should only allow you to **add** to the default list
of checks, so it will be a defcustom of an alist (there's prior art of
this), let's call this `nsm-additional-checks`
3. We can predefine a bunch of check functions that users can add to
`nsm-additional-checks` without having to write their own.
4. For dh-small-subgroup and dh-composite, the only way to check this in
LISP seems to be to supply `:min-prime-bits 2048` to
`gnutls-boot-parameters`. In which case GnuTLS will fail with fatal alert
for both counts. A user will not be able to accept dh-small-subgroup and
dh-composite certs if checks for them are enabled. This is fine, as a user
is not able to accept RC4 certs via NSM now, browsers also do it this way.

On Tue, Jun 26, 2018 at 5:11 AM, Jimmy Yuen Ho Wong <wyuenho@gmail.com>
wrote:

> `dh-composite` can be mitigated by using the "NORMAL:%PROFILE_MEDIUM"
> priority string[1], "NORMAL:%PROFILE_HIGH" [2] will pass all 26 badssl test
> while still allowing connection to ELPA/MELPA without even supplying CRL
> files (GnuTLS already does OCSP stapling verifcation transparently, and
> Emacs is using it already minus surfacing `GNUTLS_CERT_MISSING_OCSP_STATUS`
> when it fails). The exact meaning these levels appears to be spread out
> among different tables in ENISA's Algorithms, Key Sizes and Parameters
> Report - 2013 [3].
>
> As a possible way to avoid confusion, I would suggest we consolidate the 2
> different meaning of profiles (NSM and GnuTLS) into GnuTLS's. Instead of
> having users to edit an alist like Lars has done in commit 6584bc67, we
> could:
>
> 1. Append `network-security-level` to `gnutls-algorithm-priority`, i.e.
> `network-security-level` will be a list of predefined symbols that will be
> mapped to GnuTLS's `%PROFILE_*` strings, and append to it when setting up
> `gnutls-boot-parameters`.
> 2. Forget about letting users decide whether they want to accept
> problematic certs or not, no modern browsers does it anymore. Doing network
> security checks in 2 different places also introduces impedance mismatch.
> Specifically, GnuTLS by default disables a number of cyphers and hashes.
> The only way to stop it from generating fatal alerts is to enable
> everything GnuTLS has implemented and reinvent all the wheels in LISP (do
> you really want to reenable SSL3?). This is insane from both a security and
> performance perspective, as we don't have reliable NETSEC resources to
> respond to any security issues that we may introduce during the process.
> Even if we do, there's a larger problem of Emacs's release process.
> 3. To solve the problem of letting users fine tune the client's acceptable
> cyphersuite, MACs and whatnot for emergencies out of Emacs' release cycles,
> let's introduce a bunch of new defcustoms such as `gnutls-cyphersuite`,
> `gnutls-key-exchange` etc, see [1] for the table.
> 4. Normally, the fine tuning defcustoms in 3) will be nil, in which case
> `gnutls-algorithm-priority` takes precedence, otherwise they are combined
> into a final priority string supplied to `gnutls-boot-parameters`.
> 5. Merge nsm into the gnutls group. No more distinction between
> interactive and non-interactive sessions due to 2).
>
> References:
> [1]: https://gnutls.org/manual/html_node/Priority-Strings.html
> <https://gnutls.org/manual/html_node/Priority-Strings.html>
> [2]: https://gnutls.org/manual/html_node/Selecting-cryptographic-
> key-sizes.html#tab_003akey_002dsizes
> <https://gnutls.org/manual/html_node/Selecting-cryptographic-key-sizes.html#tab_003akey_002dsizes>
> [3]: https://www.enisa.europa.eu/publications/algorithms-key-size
> s-and-parameters-report
>
> <https://www.enisa.europa.eu/publications/algorithms-key-sizes-and-parameters-report>
>
> On Tue, Jun 26, 2018 at 2:23 AM, Noam Postavsky <npostavs@gmail.com>
> wrote:
>
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>> > There are also more protocol stuff we should warn about on various
>> > levels.  These should be on `high':
>>
>> >>            "https://dh-small-subgroup.badssl.com/"        ;; fail
>> >>            "https://dh-composite.badssl.com/"             ;; fail
>>
>> So these ones seem kind of problematic, as alluded to on emacs-devel.
>> It doesn't look like gnutls has an API to get or check the value of the
>> DH primes (calc-prime-test bails out when given a 1024 bit prime, so we
>> definitely need library support for this).
>>
>> https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00805.html
>> https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00807.html
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 6632 bytes --]

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

[Noam Postavsky]

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

> 4. For dh-small-subgroup and dh-composite, the only way to check this in
> LISP seems to be to supply `:min-prime-bits 2048` to
> `gnutls-boot-parameters`.

It only blocks dh-composite, not dh-small-subgroup for me.  And I think
that's just a coincidence: dh-composite.badssl.com site sends a 2047 bit
DH "prime" while dh-small-subgroup.badssl.com sends a 2048 bit DH prime.
But it's certainly possible to send a 2048 bit composite as the "prime"
which would wouldn't be blocked either.  I would guess the 2047 bit
parameter was intended to be 2048, but the top bit just happened to
generate as 0.

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

[Eli Zaretskii]

> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Tue, 26 Jun 2018 07:26:20 +0100
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 31946@debbugs.gnu.org
> 
> 1. Forget about defining what tests belongs in what levels, there should just be one level which is the default
> sets of tests, let's call this coarse grain setting.
> 2. Fine grain settings should only allow you to **add** to the default list of checks, so it will be a defcustom of
> an alist (there's prior art of this), let's call this `nsm-additional-checks`
> 3. We can predefine a bunch of check functions that users can add to `nsm-additional-checks` without having
> to write their own.

FWIW, I don't think this will fly with our users: Emacs users don't
like to be second-guessed, nor be told that "Emacs knows better".  And
even if we do go that way, Emacs is not a black box: people will soon
enough discover what we want to conceal, and will do what they want
regardless.

What we can, and probably should, do is prominently document each test
and warn against removing or weakening those which will expose users
to security vulnerabilities.

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

[Lars Ingebrigtsen]

Hm...  this URL

https://www.usps.com/business/web-tools-apis/welcome.htm

now gives a warning about a SHA1 intermediary certificate, while
Chromium and Firefox seems fine with it, so there may be a bug in the
SHA1 check.  Haven't had time to debug.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  this URL
>
> https://www.usps.com/business/web-tools-apis/welcome.htm
>
> now gives a warning about a SHA1 intermediary certificate, while
> Chromium and Firefox seems fine with it, so there may be a bug in the
> SHA1 check.  Haven't had time to debug.

According to the show certificate info in Firefox, it's the root
certificate which has SHA1.  Firefox shows both the issuer and subject
name as:

    CN = VeriSign Class 3 Public Primary Certification Authority - G5
    OU = "(c) 2006 VeriSign, Inc. - For authorized use only"
    OU = VeriSign Trust Network
    O = "VeriSign, Inc."
    C = US

But in Emacs, I'm getting this from gnutls_x509_crt_get_issuer_dn():

"C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority"

and this from  gnutls_x509_crt_get_dn():

"C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public Primary Certification Authority - G5"

So gnutls is getting this non-matching issuer from somewhere, but it's
unclear to me where.

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> But in Emacs, I'm getting this from gnutls_x509_crt_get_issuer_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority"
>
> and this from  gnutls_x509_crt_get_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006
> VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public
> Primary Certification Authority - G5"

Ah, I see...

> So gnutls is getting this non-matching issuer from somewhere, but it's
> unclear to me where.

Hm...

Oh!  I see that gnutls has gotten several variations on these functions
now.  For instance:

https://www.gnutls.org/reference/gnutls-x509.html#gnutls-x509-crt-get-issuer-dn3

It says:

"When the flag GNUTLS_X509_DN_FLAG_COMPAT is specified, the output
format will match the format output by previous to 3.5.6 versions of
GnuTLS which was not not fully RFC4514-compliant."

Which I would interpret to mean that the dn3 version of these functions
now return the RFC4515-compliant strings.  Perhaps we should call these
newer functions instead of the _dn functions?  I guess more #ifdefs and
configure checks will be needed...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> But in Emacs, I'm getting this from gnutls_x509_crt_get_issuer_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority"
>
> and this from  gnutls_x509_crt_get_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006
> VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public
> Primary Certification Authority - G5"

Ah, I see...

> So gnutls is getting this non-matching issuer from somewhere, but it's
> unclear to me where.

Hm...

Oh!  I see that gnutls has gotten several variations on these functions
now.  For instance:

https://www.gnutls.org/reference/gnutls-x509.html#gnutls-x509-crt-get-issuer-dn3

It says:

"When the flag GNUTLS_X509_DN_FLAG_COMPAT is specified, the output
format will match the format output by previous to 3.5.6 versions of
GnuTLS which was not not fully RFC4514-compliant."

Which I would interpret to mean that the dn3 version of these functions
now return the RFC4515-compliant strings.  Perhaps we should call these
newer functions instead of the _dn functions?  I guess more #ifdefs and
configure checks will be needed...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> But in Emacs, I'm getting this from gnutls_x509_crt_get_issuer_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority"
>
> and this from  gnutls_x509_crt_get_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006
> VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public
> Primary Certification Authority - G5"

Ah, I see...

> So gnutls is getting this non-matching issuer from somewhere, but it's
> unclear to me where.

Hm...

Oh!  I see that gnutls has gotten several variations on these functions
now.  For instance:

https://www.gnutls.org/reference/gnutls-x509.html#gnutls-x509-crt-get-issuer-dn3

It says:

"When the flag GNUTLS_X509_DN_FLAG_COMPAT is specified, the output
format will match the format output by previous to 3.5.6 versions of
GnuTLS which was not not fully RFC4514-compliant."

Which I would interpret to mean that the dn3 version of these functions
now return the RFC4515-compliant strings.  Perhaps we should call these
newer functions instead of the _dn functions?  I guess more #ifdefs and
configure checks will be needed...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Let's see.  Here's the issuer/subjects from the three certificates in
the chain on that site when I use _dn3:

(("CN=Symantec Class 3 Secure Server CA - G4,OU=Symantec Trust Network,O=Symantec Corporation,C=US" . "CN=*.usps.com,OU=Telecommunications Services,O=United States Postal Service,L=Raleigh,ST=North Carolina,C=US")
 ("CN=VeriSign Class 3 Public Primary Certification Authority - G5,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,OU=VeriSign Trust Network,O=VeriSign\\, Inc.,C=US" . "CN=Symantec Class 3 Secure Server CA - G4,OU=Symantec Trust Network,O=Symantec Corporation,C=US")
 ("OU=Class 3 Public Primary Certification Authority,O=VeriSign\\, Inc.,C=US" . "CN=VeriSign Class 3 Public Primary Certification Authority - G5,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,OU=VeriSign Trust Network,O=VeriSign\\, Inc.,C=US"))

And here's when I use _dn:

(("C=US,O=Symantec Corporation,OU=Symantec Trust Network,CN=Symantec Class 3 Secure Server CA - G4" . "C=US,ST=North Carolina,L=Raleigh,O=United States Postal Service,OU=Telecommunications Services,CN=*.usps.com")
 ("C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public Primary Certification Authority - G5" . "C=US,O=Symantec Corporation,OU=Symantec Trust Network,CN=Symantec Class 3 Secure Server CA - G4")
 ("C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority" . "C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public Primary Certification Authority - G5"))

OK, and this is Firefox:

    CN = VeriSign Class 3 Public Primary Certification Authority - G5
    OU = "(c) 2006 VeriSign, Inc. - For authorized use only"
    OU = VeriSign Trust Network
    O = "VeriSign, Inc."
    C = US

Hm.  Actually, aren't these all the same?  Just in different order?  The
_dn3 data seems to be the same as the _dn data, only rejuggled...

Or am I totally misreading?  That's quite likely.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Let's see.  Here's the issuer/subjects from the three certificates in
> the chain on that site when I use _dn3:

>  ("OU=Class 3 Public Primary Certification Authority,O=VeriSign\\, Inc.,C=US" . "CN=VeriSign Class 3 Public Primary Certification Authority - G5,OU=(c) 2006 VeriSign\\, Inc. - For authorized use only,OU=VeriSign Trust Network,O=VeriSign\\, Inc.,C=US"))

> OK, and this is Firefox:
>
>     CN = VeriSign Class 3 Public Primary Certification Authority - G5
>     OU = "(c) 2006 VeriSign, Inc. - For authorized use only"
>     OU = VeriSign Trust Network
>     O = "VeriSign, Inc."
>     C = US
>
> Hm.  Actually, aren't these all the same?  Just in different order?  The
> _dn3 data seems to be the same as the _dn data, only rejuggled...

Yeah, the _dn3 data still misses the CN=... from the issuer and is not
equal the the subject for the root, so it doesn't seem to help this
problem.

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Yeah, the _dn3 data still misses the CN=... from the issuer and is not
> equal the the subject for the root, so it doesn't seem to help this
> problem.

I tried using gnutls-cli, and it saus:

- Certificate[2] info:
 - subject `CN=VeriSign Class 3 Public Primary Certification Authority - G5,OU=(c) 2006 VeriSign\, Inc. - For authorized use only,OU=VeriSign Trust Network,O=VeriSign\, Inc.,C=US', issuer `OU=Class 3 Public Primary Certification Authority,O=VeriSign\, Inc.,C=US', serial 0x250ce8e030612e9f2b89f7054d7cf8fd, RSA key 2048 bits, signed using RSA-SHA1, activated `2006-11-08 00:00:00 UTC', expires `2021-11-07 23:59:59 UTC', key-ID `sha256:25b41b506e4930952823a6eb9f1d31def645ea38a5c6c6a96d71957e384df058'

So, no CN= in the issuer there, either...

And here's openssl s_client:

 2 s:/C=US/O=VeriSign, Inc./OU=VeriSign Trust Network/OU=(c) 2006 VeriSign, Inc. - For authorized use only/CN=VeriSign Class 3 Public Primary Certification Authority - G5
   i:/C=US/O=VeriSign, Inc./OU=Class 3 Public Primary Certification Authority

So there just isn't any CN= in the issuer here?

So we need a new way to determine whether a certificate is an
intermediate certificate.  Unless that really is an intermediate
certificate and the warning is correct.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So we need a new way to determine whether a certificate is an
> intermediate certificate.  Unless that really is an intermediate
> certificate and the warning is correct.  :-)

Going by the certificate serial number, Firefox doesn't show this
certificate at all in the chain.  The first two certificate serial
numbers do match up, but Firefox shows
18:DA:D1:9E:26:7D:E8:BB:4A:21:58:CD:CC:6B:3B:4A for the 3rd one (root),
while in Emacs the 3rd one has
25:0c:e8:e0:30:61:2e:9f:2b:89:f7:05:4d:7c:f8:fd.

I'm even more confused...

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Going by the certificate serial number, Firefox doesn't show this
> certificate at all in the chain.  The first two certificate serial
> numbers do match up, but Firefox shows
> 18:DA:D1:9E:26:7D:E8:BB:4A:21:58:CD:CC:6B:3B:4A for the 3rd one (root),
> while in Emacs the 3rd one has
> 25:0c:e8:e0:30:61:2e:9f:2b:89:f7:05:4d:7c:f8:fd.
>
> I'm even more confused...

Yeah, I didn't get any further here, and I don't know what more to
check...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>
>> Going by the certificate serial number, Firefox doesn't show this
>> certificate at all in the chain.

> Yeah, I didn't get any further here, and I don't know what more to
> check...

gnutls rejects SHA1 certificates by default as of 3.6.0, so I tried with
the latest stable gnutls version (3.6.3), and it does reject
sha1-intermediate.badssl.com while accepting www.usps.com.  Stepping
through with gdb I see the problematic certificate is dropped in
_gnutls_verify_crt_status, the *second* time it's called from
gnutls_x509_trust_list_verify_crt2.  I'm not sure yet how we can use
this from Emacs, but at least it's a direction to look in.

    int
    gnutls_x509_trust_list_verify_crt2(gnutls_x509_trust_list_t list,
    [...]
    {
        [...]
            hash_pjw_bare(cert_list[cert_list_size - 1]->raw_issuer_dn.
            [...]
            _gnutls_verify_crt_status(cert_list, cert_list_size,
                                            list->node[hash].trusted_cas,
                                            list->
                                            node[hash].trusted_ca_size,
                                            flags, purpose, func);
            if (SIGNER_OLD_OR_UNKNOWN(*voutput)
               [...]) {
                    /* if we couldn't find the issuer, try to see if the last
                     * certificate is in the trusted list and try to verify against
                     * (if it is not self signed) */
                        hash_pjw_bare(cert_list[cert_list_size - 1]->raw_dn.
                        [...]
                     _gnutls_debug_log("issuer in verification was not found or insecure; trying against trust list\n");

                    *voutput =
                        _gnutls_verify_crt_status(cert_list, cert_list_size,
                                                list->node[hash].trusted_cas,
                                                list->
                                                node[hash].trusted_ca_size,
                                                flags, purpose, func);

    unsigned int
    _gnutls_verify_crt_status(const gnutls_x509_crt_t * certificate_list,
    [...]
    {
       [...]
        /* We want to shorten the chain by removing the cert that matches
         * one of the certs we trust and all the certs after that i.e. if
         * cert chain is A signed-by B signed-by C signed-by D (signed-by
         * self-signed E but already removed above), and we trust B, remove
         * B, C and D. */
         [...]

        for (; i < clist_size; i++) { [...]
            for (j = 0; j < tcas_size; j++) { [...]
                /* we check for a certificate that may not be identical with the one
                 * sent by the client, but will have the same name and key. That is
                 * because it can happen that a CA certificate is upgraded from intermediate
                 * CA to self-signed CA at some point. */
                if (_gnutls_check_if_same_key
                    (certificate_list[i], trusted_cas[j], i) != 0) {
                    /* explicit time check for trusted CA that we remove from
                     * list. GNUTLS_VERIFY_DISABLE_TRUSTED_TIME_CHECKS
                     */
                    [...]
                    clist_size = i;

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

[Lars Ingebrigtsen]

And this it the patch.  It'd have to be cleaned up and autoconfiged, of
course, if this turns out to be the way to go.  (And who's
responsibility is it to clean up the data in &issuer anyway?  I can't
use gnutls_free, because the datum itself shouldn't be freed...)

diff --git a/src/gnutls.c b/src/gnutls.c
index dfbbecfc87..69b64ae96d 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1031,6 +1031,16 @@ gnutls_certificate_details (gnutls_x509_crt_t cert)
     }
 
   /* Issuer. */
+#if 1
+  {
+    gnutls_datum_t issuer;
+    err = gnutls_x509_crt_get_issuer_dn3 (cert, &issuer, 0);
+    if (err >= GNUTLS_E_SUCCESS)
+      res = nconc2 (res, list2 (intern (":issuer"),
+				make_string ((char *) issuer.data,
+					     issuer.size)));
+  }
+#else
   buf_size = 0;
   err = gnutls_x509_crt_get_issuer_dn (cert, NULL, &buf_size);
   check_memory_full (err);
@@ -1044,6 +1054,7 @@ gnutls_certificate_details (gnutls_x509_crt_t cert)
 				  make_string (dn, buf_size)));
       xfree (dn);
     }
+#endif
 
   /* Validity. */
   {
@@ -1062,6 +1073,16 @@ gnutls_certificate_details (gnutls_x509_crt_t cert)
   }
 
   /* Subject. */
+#if 1
+  {
+    gnutls_datum_t subject;
+    err = gnutls_x509_crt_get_dn3 (cert, &subject, 0);
+    if (err >= GNUTLS_E_SUCCESS)
+      res = nconc2 (res, list2 (intern (":subject"),
+				make_string ((char *) subject.data,
+					     subject.size)));
+  }
+#else
   buf_size = 0;
   err = gnutls_x509_crt_get_dn (cert, NULL, &buf_size);
   check_memory_full (err);
@@ -1075,6 +1096,7 @@ gnutls_certificate_details (gnutls_x509_crt_t cert)
 				  make_string (dn, buf_size)));
       xfree (dn);
     }
+#endif
 
   /* SubjectPublicKeyInfo. */
   {


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> (And who's
> responsibility is it to clean up the data in &issuer anyway?  I can't
> use gnutls_free, because the datum itself shouldn't be freed...)

Aha.  You're supposed to say

        gnutls_free(buf.data);

This is a thing that often frustrates me with C libraries: They usually
don't specify these memory life cycle things...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> But in Emacs, I'm getting this from gnutls_x509_crt_get_issuer_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=Class 3 Public Primary Certification Authority"
>
> and this from  gnutls_x509_crt_get_dn():
>
> "C=US,O=VeriSign\\, Inc.,OU=VeriSign Trust Network,OU=(c) 2006
> VeriSign\\, Inc. - For authorized use only,CN=VeriSign Class 3 Public
> Primary Certification Authority - G5"

Ah, I see...

> So gnutls is getting this non-matching issuer from somewhere, but it's
> unclear to me where.

Hm...

Oh!  I see that gnutls has gotten several variations on these functions
now.  For instance:

https://www.gnutls.org/reference/gnutls-x509.html#gnutls-x509-crt-get-issuer-dn3

It says:

"When the flag GNUTLS_X509_DN_FLAG_COMPAT is specified, the output
format will match the format output by previous to 3.5.6 versions of
GnuTLS which was not not fully RFC4514-compliant."

Which I would interpret to mean that the dn3 version of these functions
now return the RFC4515-compliant strings.  Perhaps we should call these
newer functions instead of the _dn functions?  I guess more #ifdefs and
configure checks will be needed...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>>>            "https://dh-small-subgroup.badssl.com/"        ;; fail
>>>            "https://dh-composite.badssl.com/"             ;; fail
>
> So these ones seem kind of problematic, as alluded to on emacs-devel.
> It doesn't look like gnutls has an API to get or check the value of the
> DH primes (calc-prime-test bails out when given a 1024 bit prime, so we
> definitely need library support for this).

But they may not that urgent, if I understand things correctly.  Current
Firefox, for instance, does not seem to warn about this.

We could get in touch with the gnutls maintainer and ask for his input
and perhaps ask for API endpoints to allow us to check for these things?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 26 Jun 2018 11:27:34 +0200
> Cc: 31946@debbugs.gnu.org, Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> 
> We could get in touch with the gnutls maintainer and ask for his input
> and perhaps ask for API endpoints to allow us to check for these things?

Yes, I think that's the right way for moving forward.

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

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Tue, 26 Jun 2018 11:27:34 +0200
>> Cc: 31946@debbugs.gnu.org, Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>> 
>> We could get in touch with the gnutls maintainer and ask for his input
>> and perhaps ask for API endpoints to allow us to check for these things?
>
> Yes, I think that's the right way for moving forward.

By the way, I've researched this a bit more, it seems like there is no
practical way to detect small subgroups at all, the only solution is to
move to standardized domains (the smallest of which is 2048 bits)
similar to how ECDHE uses standard curves.  This also solves the
composite prime problem, which is likely too expensive to check as well.

https://tools.ietf.org/html/rfc7919:

   Additionally, the DH parameters selected by the server may have a
   known structure that renders them secure against a small subgroup
   attack, but a client receiving an arbitrary p and g has no efficient
   way to verify that the structure of a new group is reasonable for
   use.

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

[Jimmy Yuen Ho Wong]

[-- Attachment #1: Type: text/plain, Size: 3974 bytes --]

On Wed, Jun 27, 2018 at 1:45 AM, Noam Postavsky <npostavs@gmail.com> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: Lars Ingebrigtsen <larsi@gnus.org>
> >> Date: Tue, 26 Jun 2018 11:27:34 +0200
> >> Cc: 31946@debbugs.gnu.org, Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> >>
> >> We could get in touch with the gnutls maintainer and ask for his input
> >> and perhaps ask for API endpoints to allow us to check for these things?
> >
> > Yes, I think that's the right way for moving forward.
>
> By the way, I've researched this a bit more, it seems like there is no
> practical way to detect small subgroups at all, the only solution is to
> move to standardized domains (the smallest of which is 2048 bits)
> similar to how ECDHE uses standard curves.  This also solves the
> composite prime problem, which is likely too expensive to check as well.
>
> https://tools.ietf.org/html/rfc7919:
>
>    Additionally, the DH parameters selected by the server may have a
>    known structure that renders them secure against a small subgroup
>    attack, but a client receiving an arbitrary p and g has no efficient
>    way to verify that the structure of a new group is reasonable for
>    use.
>

According to Dorey et all [1], the RFC you've linked is one of the 4
strategies she
proposed, and the the most feasible in 2016 but still computationally
expensive.

Another strategy involves disabling DHE, which she didn't really like as
that will be
TLS 1.3's only fallback, but Safari[2], Chrome[3] have since removed them,
and Firefox
[4] is on the verge, so I think this option is also viable for Emacs. Just
need to pick out the
appripriate DHE ciphers and add a - for everyone of them in the constructed
priority string
in `gnutls-boot`.

As to why these browsers are still "failing" the dh-small-subgroup and
dh-composite tests,
the negotiated ciphersuite on both Chrome and Firefox is
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, so the effects of small subgroup
attack is basically mitigated (if I understand the papers correctly).

`(setq gnutls-log-level 999)` in Emacs currently doesn't tell me what KX
algo was used
, it just tells me AES-256-GCM was negotiated as a cipher. However,

`$ gnutls-cli -V --x509cafile /opt/local/etc/openssl/cert.pem -p 443
dh-small-subgroup.badssl.com`

gives me

- Status: The certificate is trusted.
- Description: (TLS1.2)-(DHE-RSA-2048)-(AES-128-GCM)
- Session ID: ...
- Ephemeral Diffie-Hellman parameters
 - Using prime: 2048 bits
 - Secret key: 507 bits
 - Peer's public key: 2048 bits
 - PKCS#3 format:

-----BEGIN DH PARAMETERS-----
...
-----END DH PARAMETERS-----

- Version: TLS1.2
- Key Exchange: DHE-RSA
- Server Signature: RSA-SHA512
- Cipher: AES-128-GCM
- MAC: AEAD
- Compression: NULL
- Options: safe renegotiation,
- Channel binding 'tls-unique': ...
- Handshake was completed

This is just plain... weird. It's TLS_DHE_RSA_AES_128_GCM_SHA512 with 2048
bit prime, and it's nowhere to be found in browsers' ciphersuites.
Although, TLS_DHE_RSA_AES_128_GCM_SHA256 does still exist on browsers,
and does offer perfect forward secrecy according to SSLLabs[5], but it's
fairly low in
their preferences.

So.... this look o...kay if you cut out the signature algorithm? Someone
should
doublecheck this.

Alternatively, we can wait for widespread adaption of TLS 1.3 (Cloudflare
predicts
50% of TLS connection will be 1.3 by the end of this year[6]), time fixes
everything...

Tidbit: The GnuTLS basically ignored a group of Adobe researchers when they
reported to them GnuTLS was susceptible to the small group attack[7]...

[1]: https://eprint.iacr.org/2016/999.pdf
[2]: https://groups.google.com/a/chromium.org/forum/#!topic/
blink-dev/AAdv838-koo
[3]: https://www.chromestatus.com/feature/5128908798164992
[4]: https://bugzilla.mozilla.org/show_bug.cgi?id=1227519
[5]: https://www.ssllabs.com/ssltest/viewMyClient.html
[6]: https://blog.cloudflare.com/our-predictions-for-2018/
[7]: https://eprint.iacr.org/2016/995.pdf

[-- Attachment #2: Type: text/html, Size: 6520 bytes --]

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

[Lars Ingebrigtsen]

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

> `(setq gnutls-log-level 999)` in Emacs currently doesn't tell me what
> KX algo was used , it just tells me AES-256-GCM was negotiated as a
> cipher. However,

To get the connection parameters, say something like:

(gnutls-peer-status (open-network-stream "foo" nil "dh-composite.badssl.com" "https" :tls-parameters (cons 'gnutls-x509pki (gnutls-boot-parameters :hostname "dh-composite.badssl.com"))))

=>

(:certificates ((:version 3 :serial-number "01:f2:02:03:1d:fd:a9:8e:fd:ff:0f:72:be:51:06:0d" :issuer "C=US,O=DigiCert Inc,CN=DigiCert SHA2 Secure Server CA" :valid-from "2017-03-18" :valid-to "2020-03-25" :subject "C=US,ST=California,L=Walnut Creek,O=Lucas Garron,CN=*.badssl.com" :public-key-algorithm "RSA" :certificate-security-level "Medium" :signature-algorithm "RSA-SHA256" :public-key-id "sha1:79:65:df:c9:3c:6a:e6:fe:83:81:ec:48:22:16:ec:44:ef:47:28:2a" :certificate-id "sha1:ca:53:08:74:6c:1e:06:44:d6:3a:f6:1b:f5:81:c7:2a:f9:0c:70:95") (:version 3 :serial-number "01:fd:a3:eb:6e:ca:75:c8:88:43:8b:72:4b:cf:bc:91" :issuer "C=US,O=DigiCert Inc,OU=www.digicert.com,CN=DigiCert Global Root CA" :valid-from "2013-03-08" :valid-to "2023-03-08" :subject "C=US,O=DigiCert Inc,CN=DigiCert SHA2 Secure Server CA" :public-key-algorithm "RSA" :certificate-security-level "Medium" :signature-algorithm "RSA-SHA256" :public-key-id "sha1:51:bc:4f:77:17:08:cf:e5:09:dd:e9:ea:a5:54:8e:91:c0:67:78:53" :certificate-id "sha1:1f:b8:6b:11:68:ec:74:31:54:06:2e:8c:9c:c5:b1:71:a4:b7:cc:b4")) :certificate (:version 3 :serial-number "01:f2:02:03:1d:fd:a9:8e:fd:ff:0f:72:be:51:06:0d" :issuer "C=US,O=DigiCert Inc,CN=DigiCert SHA2 Secure Server CA" :valid-from "2017-03-18" :valid-to "2020-03-25" :subject "C=US,ST=California,L=Walnut Creek,O=Lucas Garron,CN=*.badssl.com" :public-key-algorithm "RSA" :certificate-security-level "Medium" :signature-algorithm "RSA-SHA256" :public-key-id "sha1:79:65:df:c9:3c:6a:e6:fe:83:81:ec:48:22:16:ec:44:ef:47:28:2a" :certificate-id "sha1:ca:53:08:74:6c:1e:06:44:d6:3a:f6:1b:f5:81:c7:2a:f9:0c:70:95") :diffie-hellman-prime-bits 2047 :key-exchange "DHE-RSA" :protocol "TLS1.2" :cipher "AES-128-GCM" :mac "AEAD")


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> To get the connection parameters, say something like:
>
> (gnutls-peer-status (open-network-stream "foo" nil
> "dh-composite.badssl.com" "https" :tls-parameters (cons
> 'gnutls-x509pki (gnutls-boot-parameters :hostname
> "dh-composite.badssl.com"))))

Speaking of which -- it's quite a mouthful to say:

(open-network-stream
 "foo" nil "dh-composite.badssl.com" "https"
 :tls-parameters (cons 'gnutls-x509pki (gnutls-boot-parameters
                                        :hostname "dh-composite.badssl.com")))

I've been meaning to add a :tls keyword to `open-network-stream' that
would make

(open-network-stream "foo" nil "dh-composite.badssl.com" "https" :tls t)

a short way to write the above.  I.e., the default TLS parameters (which
is what you need in 99.9% of the cases) would be used if you just say
:tls t.

Does that sound OK to you, Eli?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 31946@debbugs.gnu.org,  Noam Postavsky <npostavs@gmail.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 27 Jun 2018 14:20:16 +0200
> 
> Speaking of which -- it's quite a mouthful to say:
> 
> (open-network-stream
>  "foo" nil "dh-composite.badssl.com" "https"
>  :tls-parameters (cons 'gnutls-x509pki (gnutls-boot-parameters
>                                         :hostname "dh-composite.badssl.com")))
> 
> I've been meaning to add a :tls keyword to `open-network-stream' that
> would make
> 
> (open-network-stream "foo" nil "dh-composite.badssl.com" "https" :tls t)
> 
> a short way to write the above.  I.e., the default TLS parameters (which
> is what you need in 99.9% of the cases) would be used if you just say
> :tls t.
> 
> Does that sound OK to you, Eli?

Sounds good, but does it really require a new property?  Why not a
special value of the existing :tls-parameters?  For example:

  (open-network-stream "foo" nil "dh-composite.badssl.com" "https"
                       :tls-parameters 'tls-defaults)

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

[Jimmy Yuen Ho Wong]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

I've been reading a bit more on recent cipher and key exchange negotiation
changes, it appears that the reason 3des "fail" on modern browsers is the
same reason they "fail" dh-small-subgroup and dh-composite. They are not
actually failing if the negotiated KX algo is ECDHE.

As a good measure, I think we should also offer in the high profile, checks
for RSA KX and CBC mode ciphers. They are all marked as weak by modern
browsers. There are apparently enterprise middlewares that decrypt RSA KX
for monitoring. CBC is weak and should also be checked in the high profile
because BEAST and POODLE (high because of compatibiltiy).

On Wed, Jun 27, 2018 at 4:16 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: 31946@debbugs.gnu.org,  Noam Postavsky <npostavs@gmail.com>, Eli
> Zaretskii <eliz@gnu.org>
> > Date: Wed, 27 Jun 2018 14:20:16 +0200
> >
> > Speaking of which -- it's quite a mouthful to say:
> >
> > (open-network-stream
> >  "foo" nil "dh-composite.badssl.com" "https"
> >  :tls-parameters (cons 'gnutls-x509pki (gnutls-boot-parameters
> >                                         :hostname "
> dh-composite.badssl.com")))
> >
> > I've been meaning to add a :tls keyword to `open-network-stream' that
> > would make
> >
> > (open-network-stream "foo" nil "dh-composite.badssl.com" "https" :tls t)
> >
> > a short way to write the above.  I.e., the default TLS parameters (which
> > is what you need in 99.9% of the cases) would be used if you just say
> > :tls t.
> >
> > Does that sound OK to you, Eli?
>
> Sounds good, but does it really require a new property?  Why not a
> special value of the existing :tls-parameters?  For example:
>
>   (open-network-stream "foo" nil "dh-composite.badssl.com" "https"
>                        :tls-parameters 'tls-defaults)
>

[-- Attachment #2: Type: text/html, Size: 3004 bytes --]

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> (open-network-stream "foo" nil "dh-composite.badssl.com" "https" :tls t)
>> 
>> a short way to write the above.  I.e., the default TLS parameters (which
>> is what you need in 99.9% of the cases) would be used if you just say
>> :tls t.
>> 
>> Does that sound OK to you, Eli?
>
> Sounds good, but does it really require a new property?  Why not a
> special value of the existing :tls-parameters?  For example:
>
>   (open-network-stream "foo" nil "dh-composite.badssl.com" "https"
>                        :tls-parameters 'tls-defaults)

Yes, that would work, too.

But I think (considering the semantics and discoverability of the
interface), that a separate :tls parameter would be nice.  If the user
wonders "how to I make this connection encrypted?  Ah, :tls t".  It
seems like a logical interface.

For instance in constructions like

(open-network-stream ... :tls (and foo bar))

it seems more idiomatic. 

While :tls-parameters 'tls-defaults is something that seems less natural
to me.  :tls t :tls-parameters ... for the extremely, extremely few that
need to specify the parameters explicitly doesn't seem like a huge
imposition, either...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Noam Postavsky]

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

> Tidbit: The GnuTLS basically ignored a group of Adobe researchers when they
> reported to them GnuTLS was susceptible to the small group
> attack[7]...

> [7]: https://eprint.iacr.org/2016/995.pdf

I guess the report is here (the reporter, Luke Valenta, is the first
author of the paper): https://gitlab.com/gnutls/gnutls/issues/104

The paper just says "didn't patch", but looking in the details of the
report, Luke says:

    From a client's perspective, the TLS protocol limitation does
    prevent "q" from being specified. However, since a server knows the
    value of "q", it should be perform proper subgroup validation checks
    as a precaution against small subgroup attacks[...]

    I agree that since the server does not reuse ephemeral DH keys, it
    is not currently vulnerable to a small subgroup attack.

So, the client side can't be patched, and the server side doesn't really
need to be patched (just leave the "reuse ephemeral key" option turned
off).

Furthermore, it seems gnutls has added support for standardized primes,
so that pretty much resolves the issue as much as it can be:

https://gitlab.com/gnutls/gnutls/merge_requests/437

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

[Jimmy Yuen Ho Wong]

>
> So, the client side can't be patched, and the server side doesn't really
> need to be patched (just leave the "reuse ephemeral key" option turned
> off).
>

He's talking about GnuTLS servers, nobody uses GnuTLS on the
server-side, also GnuTLS' implementation of RFC 7919 doesn't seem to
be in 3.5.x branch.

A bit more update from my research, the authoritative list of browser
capabilities is actually https://www.ssllabs.com/ssltest/clients.html
. We can see that only IE and Opera still support DHE_RSA KX, and
these browsers don't matter as game changers.

Unless we plan to require GnuTLS 3.6+, we'll definitely need to warn
in the 'medium level. This is a super simple check.

> Furthermore, it seems gnutls has added support for standardized primes,
> so that pretty much resolves the issue as much as it can be:
>

WRT RFC 7919, IMO, it is dead on arrival, even when it is approved.
With DHE based ciphers removed in browsers, the only real options are
TLS 1.2/1.3 with ECDHE KX, and fall back to TLS 1.2 with RSA KX.

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

[Lars Ingebrigtsen]

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

> A bit more update from my research, the authoritative list of browser
> capabilities is actually https://www.ssllabs.com/ssltest/clients.html
> . We can see that only IE and Opera still support DHE_RSA KX, and
> these browsers don't matter as game changers.

(For those who don't read Encryption Acronym Soup, that means
"Diffie-Hellman (Ephemeral) key exchange.)

I can't see that that web page mentions Diffie-Hellman at all?

And Firefox in Debian Stable certainly supports Diffie-Hellman.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Jimmy Yuen Ho Wong]

> I can't see that that web page mentions Diffie-Hellman at all?
>

Click on the individual browsers.

> And Firefox in Debian Stable certainly supports Diffie-Hellman.
>

Firefox on Debian is Firefox 52 ESR, it's 9 versions behind current.
Firefox 60 ESR is in the experimental section.

SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
DHE_RSA in the UA capabilities page[1], but client test[2] still shows
it is supported, so does Chrome and Safari. I don't understand what's
going on there. Could that list in in client test be static? Or that
browsers still advertise their support for DHE_RSA when in fact they
don't? Might have to get on a server and log out the TLS handshake to
see what's actually going on...

[1]: https://www.ssllabs.com/ssltest/clients.html
[2]: https://www.ssllabs.com/ssltest/viewMyClient.html

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

[Lars Ingebrigtsen]

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

>> I can't see that that web page mentions Diffie-Hellman at all?
>>
>
> Click on the individual browsers.

I see.

> SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
> DHE_RSA in the UA capabilities page[1], but client test[2] still shows
> it is supported, so does Chrome and Safari. I don't understand what's
> going on there. Could that list in in client test be static? Or that
> browsers still advertise their support for DHE_RSA when in fact they
> don't? Might have to get on a server and log out the TLS handshake to
> see what's actually going on...
>
> [1]: https://www.ssllabs.com/ssltest/clients.html
> [2]: https://www.ssllabs.com/ssltest/viewMyClient.html

My 

Chromium	66.0.3359.117 (Developer Build) built on Debian 9.4,
running on Debian 9.4 (64-bit)

on the viewMyClient reports not supporting DHE-RSA.

Confusing.  :-)

I tried finding a web site that says how many sites do not support ECDHE
as key exchange, and only found something from 2014 that says that was
60%...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Jimmy Yuen Ho Wong]

The Telemetry data[1] from Mozilla in bug report 1227519[2] suggests
DHE usage is very low for HTTP. No data for any other protocol.

I just used Wireshark on Chrome and Firefox on macOS, they all seem to
advertise DH and DHE cipher suites in Client Hello for TLS 1.2, they
even advertise CBC mode ciphers too. While I'm not sure about Firefox,
surely Chrome has removed DHE_SHA KX and CBC modes according to
ChromeStatus[3]?


[1]: https://tlscanary.mozilla.org/runs/2018-01-25-01-21-44/
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1227519
[3]: https://www.chromestatus.com/features#tls

On Thu, Jun 28, 2018 at 6:01 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>
>>> I can't see that that web page mentions Diffie-Hellman at all?
>>>
>>
>> Click on the individual browsers.
>
> I see.
>
>> SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
>> DHE_RSA in the UA capabilities page[1], but client test[2] still shows
>> it is supported, so does Chrome and Safari. I don't understand what's
>> going on there. Could that list in in client test be static? Or that
>> browsers still advertise their support for DHE_RSA when in fact they
>> don't? Might have to get on a server and log out the TLS handshake to
>> see what's actually going on...
>>
>> [1]: https://www.ssllabs.com/ssltest/clients.html
>> [2]: https://www.ssllabs.com/ssltest/viewMyClient.html
>
> My
>
> Chromium        66.0.3359.117 (Developer Build) built on Debian 9.4,
> running on Debian 9.4 (64-bit)
>
> on the viewMyClient reports not supporting DHE-RSA.
>
> Confusing.  :-)
>
> I tried finding a web site that says how many sites do not support ECDHE
> as key exchange, and only found something from 2014 that says that was
> 60%...
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

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

[Jimmy Yuen Ho Wong]

Update:

I just tried with this node.js script:

#! /usr/bin/env node

const https = require('https');
const fs = require('fs');

const options = {
  key: fs.readFileSync('ryans-key.pem'),
  cert: fs.readFileSync('ryans-cert.pem'),
  ciphers: "DHE-RSA-AES128-GCM-SHA256"
};

https.createServer(options, (req, res) => {
  res.writeHead(200);
  res.end('hello world\n');
}).listen(8000);

Chrome gave me a ERR_SSL_VERSION_OR_CIPHER_MISMATCH, Firefox gave me a
SSL_ERROR_NO_CYPHER_OVERLAP, Safari just told me it can't establish a
connection.

Conversely, changing DHE to ECDHE will at least show me the warning
screen for a self-signed cert, so I'm fairly certain now that these 3
browsers have removed DHE KX.


Heads up:

I'm adding about 10 new checks into NSM ATM. Will send a early preview
patch here later today for feedback.


On Thu, Jun 28, 2018 at 7:15 PM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
> The Telemetry data[1] from Mozilla in bug report 1227519[2] suggests
> DHE usage is very low for HTTP. No data for any other protocol.
>
> I just used Wireshark on Chrome and Firefox on macOS, they all seem to
> advertise DH and DHE cipher suites in Client Hello for TLS 1.2, they
> even advertise CBC mode ciphers too. While I'm not sure about Firefox,
> surely Chrome has removed DHE_SHA KX and CBC modes according to
> ChromeStatus[3]?
>
>
> [1]: https://tlscanary.mozilla.org/runs/2018-01-25-01-21-44/
> [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1227519
> [3]: https://www.chromestatus.com/features#tls
>
> On Thu, Jun 28, 2018 at 6:01 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>>
>>>> I can't see that that web page mentions Diffie-Hellman at all?
>>>>
>>>
>>> Click on the individual browsers.
>>
>> I see.
>>
>>> SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
>>> DHE_RSA in the UA capabilities page[1], but client test[2] still shows
>>> it is supported, so does Chrome and Safari. I don't understand what's
>>> going on there. Could that list in in client test be static? Or that
>>> browsers still advertise their support for DHE_RSA when in fact they
>>> don't? Might have to get on a server and log out the TLS handshake to
>>> see what's actually going on...
>>>
>>> [1]: https://www.ssllabs.com/ssltest/clients.html
>>> [2]: https://www.ssllabs.com/ssltest/viewMyClient.html
>>
>> My
>>
>> Chromium        66.0.3359.117 (Developer Build) built on Debian 9.4,
>> running on Debian 9.4 (64-bit)
>>
>> on the viewMyClient reports not supporting DHE-RSA.
>>
>> Confusing.  :-)
>>
>> I tried finding a web site that says how many sites do not support ECDHE
>> as key exchange, and only found something from 2014 that says that was
>> 60%...
>>
>> --
>> (domestic pets only, the antidote for overdose, milk.)
>>    bloggy blog: http://lars.ingebrigtsen.no

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

[Jimmy Yuen Ho Wong]

Correction:

This is the actualy cipher string I used
"DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA256".

All 3 browsers fail to connect.

On Fri, Jun 29, 2018 at 6:21 AM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
> Update:
>
> I just tried with this node.js script:
>
> #! /usr/bin/env node
>
> const https = require('https');
> const fs = require('fs');
>
> const options = {
>   key: fs.readFileSync('ryans-key.pem'),
>   cert: fs.readFileSync('ryans-cert.pem'),
>   ciphers: "DHE-RSA-AES128-GCM-SHA256"
> };
>
> https.createServer(options, (req, res) => {
>   res.writeHead(200);
>   res.end('hello world\n');
> }).listen(8000);
>
> Chrome gave me a ERR_SSL_VERSION_OR_CIPHER_MISMATCH, Firefox gave me a
> SSL_ERROR_NO_CYPHER_OVERLAP, Safari just told me it can't establish a
> connection.
>
> Conversely, changing DHE to ECDHE will at least show me the warning
> screen for a self-signed cert, so I'm fairly certain now that these 3
> browsers have removed DHE KX.
>
>
> Heads up:
>
> I'm adding about 10 new checks into NSM ATM. Will send a early preview
> patch here later today for feedback.
>
>
> On Thu, Jun 28, 2018 at 7:15 PM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
>> The Telemetry data[1] from Mozilla in bug report 1227519[2] suggests
>> DHE usage is very low for HTTP. No data for any other protocol.
>>
>> I just used Wireshark on Chrome and Firefox on macOS, they all seem to
>> advertise DH and DHE cipher suites in Client Hello for TLS 1.2, they
>> even advertise CBC mode ciphers too. While I'm not sure about Firefox,
>> surely Chrome has removed DHE_SHA KX and CBC modes according to
>> ChromeStatus[3]?
>>
>>
>> [1]: https://tlscanary.mozilla.org/runs/2018-01-25-01-21-44/
>> [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1227519
>> [3]: https://www.chromestatus.com/features#tls
>>
>> On Thu, Jun 28, 2018 at 6:01 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>>> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>>>
>>>>> I can't see that that web page mentions Diffie-Hellman at all?
>>>>>
>>>>
>>>> Click on the individual browsers.
>>>
>>> I see.
>>>
>>>> SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
>>>> DHE_RSA in the UA capabilities page[1], but client test[2] still shows
>>>> it is supported, so does Chrome and Safari. I don't understand what's
>>>> going on there. Could that list in in client test be static? Or that
>>>> browsers still advertise their support for DHE_RSA when in fact they
>>>> don't? Might have to get on a server and log out the TLS handshake to
>>>> see what's actually going on...
>>>>
>>>> [1]: https://www.ssllabs.com/ssltest/clients.html
>>>> [2]: https://www.ssllabs.com/ssltest/viewMyClient.html
>>>
>>> My
>>>
>>> Chromium        66.0.3359.117 (Developer Build) built on Debian 9.4,
>>> running on Debian 9.4 (64-bit)
>>>
>>> on the viewMyClient reports not supporting DHE-RSA.
>>>
>>> Confusing.  :-)
>>>
>>> I tried finding a web site that says how many sites do not support ECDHE
>>> as key exchange, and only found something from 2014 that says that was
>>> 60%...
>>>
>>> --
>>> (domestic pets only, the antidote for overdose, milk.)
>>>    bloggy blog: http://lars.ingebrigtsen.no

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

[Jimmy Yuen Ho Wong]

[-- Attachment #1: Type: text/plain, Size: 5307 bytes --]

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.

Possible improvements:

* Check for TLS compression. I haven't looked into how to do this with
GnuTLS' API yet, but we should probably do this because CRIME and
BREACH.
* Check for TLS 1.0 usage and protocol downgrades? Today happens to be
the deadline to migrate to TLS 1.1+ for PCI DSS compliance, but the
reason seems to be POODLE, BEAST, CRIME, BREACH and Heartbleed and
downgrade to SSL. Mitigations for some of these are already in
existing checks, others may be covered by a compression check. Maybe
we should be paranoid about this? Also, I'm not sure how to check for
protocol downgrades with GnuTLS' API.
* Write some unit tests....

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.

Jimmy

On Fri, Jun 29, 2018 at 6:25 AM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
> Correction:
>
> This is the actualy cipher string I used
> "DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA256".
>
> All 3 browsers fail to connect.
>
> On Fri, Jun 29, 2018 at 6:21 AM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
>> Update:
>>
>> I just tried with this node.js script:
>>
>> #! /usr/bin/env node
>>
>> const https = require('https');
>> const fs = require('fs');
>>
>> const options = {
>>   key: fs.readFileSync('ryans-key.pem'),
>>   cert: fs.readFileSync('ryans-cert.pem'),
>>   ciphers: "DHE-RSA-AES128-GCM-SHA256"
>> };
>>
>> https.createServer(options, (req, res) => {
>>   res.writeHead(200);
>>   res.end('hello world\n');
>> }).listen(8000);
>>
>> Chrome gave me a ERR_SSL_VERSION_OR_CIPHER_MISMATCH, Firefox gave me a
>> SSL_ERROR_NO_CYPHER_OVERLAP, Safari just told me it can't establish a
>> connection.
>>
>> Conversely, changing DHE to ECDHE will at least show me the warning
>> screen for a self-signed cert, so I'm fairly certain now that these 3
>> browsers have removed DHE KX.
>>
>>
>> Heads up:
>>
>> I'm adding about 10 new checks into NSM ATM. Will send a early preview
>> patch here later today for feedback.
>>
>>
>> On Thu, Jun 28, 2018 at 7:15 PM, Jimmy Yuen Ho Wong <wyuenho@gmail.com> wrote:
>>> The Telemetry data[1] from Mozilla in bug report 1227519[2] suggests
>>> DHE usage is very low for HTTP. No data for any other protocol.
>>>
>>> I just used Wireshark on Chrome and Firefox on macOS, they all seem to
>>> advertise DH and DHE cipher suites in Client Hello for TLS 1.2, they
>>> even advertise CBC mode ciphers too. While I'm not sure about Firefox,
>>> surely Chrome has removed DHE_SHA KX and CBC modes according to
>>> ChromeStatus[3]?
>>>
>>>
>>> [1]: https://tlscanary.mozilla.org/runs/2018-01-25-01-21-44/
>>> [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1227519
>>> [3]: https://www.chromestatus.com/features#tls
>>>
>>> On Thu, Jun 28, 2018 at 6:01 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>>>> Jimmy Yuen Ho Wong <wyuenho@gmail.com> writes:
>>>>
>>>>>> I can't see that that web page mentions Diffie-Hellman at all?
>>>>>>
>>>>>
>>>>> Click on the individual browsers.
>>>>
>>>> I see.
>>>>
>>>>> SSLLabs only reports that Firefox 59 / Win 7 has dropped support for
>>>>> DHE_RSA in the UA capabilities page[1], but client test[2] still shows
>>>>> it is supported, so does Chrome and Safari. I don't understand what's
>>>>> going on there. Could that list in in client test be static? Or that
>>>>> browsers still advertise their support for DHE_RSA when in fact they
>>>>> don't? Might have to get on a server and log out the TLS handshake to
>>>>> see what's actually going on...
>>>>>
>>>>> [1]: https://www.ssllabs.com/ssltest/clients.html
>>>>> [2]: https://www.ssllabs.com/ssltest/viewMyClient.html
>>>>
>>>> My
>>>>
>>>> Chromium        66.0.3359.117 (Developer Build) built on Debian 9.4,
>>>> running on Debian 9.4 (64-bit)
>>>>
>>>> on the viewMyClient reports not supporting DHE-RSA.
>>>>
>>>> Confusing.  :-)
>>>>
>>>> I tried finding a web site that says how many sites do not support ECDHE
>>>> as key exchange, and only found something from 2014 that says that was
>>>> 60%...
>>>>
>>>> --
>>>> (domestic pets only, the antidote for overdose, milk.)
>>>>    bloggy blog: http://lars.ingebrigtsen.no

[-- Attachment #2: 0001-Modern-protocol-checks-for-NSM.patch --]
[-- Type: application/octet-stream, Size: 26086 bytes --]

From 09ddb4892879e93025b0d6f92979b9f8be350c62 Mon Sep 17 00:00:00 2001
From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
Date: Sat, 30 Jun 2018 17:28:47 +0100
Subject: [PATCH] Modern protocol checks for NSM

* lisp/net/nsm.el
(nsm-check-certificate, nsm-fingerprint-ok-p,
nsm-check-plain-connection): Pre-format query messages before passing
to nsm-query.
(nsm-query): No longer takes extra arguments for formatting
messages.
(nsm-query-user): No longer take extra arguments for formatting
messages. 'what' parameter can now be a list of conditions.
(nsm-check-protocol): Prompts user for all problems
found instead of just the first one.
(network-security-protocol-checks): Add rsx-kx, anon-kx,
export-kx, cbc-cipher, ecdsa-cbc-cipher, des-cipher, rc2-cipher,
null-cipher, md5-sig checks.
(nsm-protocol-check--diffie-hellman-prime-bits): Rename to
nsm-protocol-check--dhe-kx. Checks for prime bits < 1024 for 'medium
network-security-level and usage of DHE KX for 'high in addition to
prime bits. Shorten error messages.
(nsm-protocol-check--rc4): Rename to
nsm-protocol-check--rc4-cipher. Fix bug where it was previously
checking for non-existent cipher name RC4 in GnuTLS instead of
ARCFOUR. Shorten error message.
(nsm-protocol-check--signature-sha1): Removed.
(nsm-protocol-check--intermediate-sha1): Rename to
nsm-protocol-check--sha1-sig. Shorten error message.

(Bug#31946)
---
 lisp/net/nsm.el | 511 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 404 insertions(+), 107 deletions(-)

diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index 0653cfbb1a..df67fe9cf6 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -169,24 +169,33 @@ nsm-check-certificate
 	(if (and (not (nsm-warnings-ok-p status settings))
 		 (not (nsm-query
 		       host port status 'conditions
-		       "The TLS connection to %s:%s is insecure for the following reason%s:\n\n%s"
-		       host port
-		       (if (> (length warnings) 1)
-			   "s" "")
-		       (mapconcat #'gnutls-peer-status-warning-describe
-                                  warnings
-                                  "\n"))))
+                       (format-message
+		        "The TLS connection to %s:%s is insecure for the following reason%s:\n\n%s"
+		        host port
+		        (if (> (length warnings) 1)
+			    "s" "")
+		        (mapconcat #'gnutls-peer-status-warning-describe
+                                   warnings
+                                   "\n")))))
 	    (progn
 	      (delete-process process)
 	      nil)
 	  process))))))
 
 (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.
 It's an alist where the first element is the name of the check,
@@ -198,87 +207,370 @@ network-security-protocol-checks
 HOST PORT STATUS OPTIONAL-PARAMETER.")
 
 (defun nsm-check-protocol (process host port status settings)
-  (cl-loop for check in network-security-protocol-checks
-           for type = (intern (format ":%s" (car check)) obarray)
-           while process
-           ;; Skip the check if the user has already said that this
-           ;; host is OK for this type of "error".
-           when (and (not (memq type (plist-get settings :conditions)))
-                     (>= (nsm-level network-security-level)
-                         (nsm-level (cadr check))))
-           do (let ((result
-                     (funcall (intern (format "nsm-protocol-check--%s"
-                                              (car check))
-                                      obarray)
-                              host port status (nth 2 check))))
-                (unless result
-                  (delete-process process)
-                  (setq process nil))))
+  (let ((results
+         (cl-remove-if-not
+          #'cdr
+          (cl-loop for check in network-security-protocol-checks
+                   for type = (intern (format ":%s" (car check)) obarray)
+                   ;; Skip the check if the user has already said that this
+                   ;; host is OK for this type of "error".
+                   when (and (not (memq type (plist-get settings :conditions)))
+                             (>= (nsm-level network-security-level)
+                                 (nsm-level (cadr check))))
+                   collect (cons
+                            type
+                            (funcall (intern (format "nsm-protocol-check--%s"
+                                                     (car check))
+                                             obarray)
+                                     host port status))))))
+    (when (and results
+               (not (nsm-query host port status
+                               (sort (map-keys results) #'string<)
+                               (format-message
+		                "The TLS connection to %s:%s is insecure for the following reason%s:\n\n%s"
+		                host port
+		                (if (> (length results) 1)
+			            "s" "")
+		                (string-join (map-values results) "\n")))))
+      (delete-process process)
+      (setq process nil)))
   ;; If a test failed we return nil, otherwise the process object.
   process)
 
-(defun nsm--encryption (status)
-  (format "%s-%s-%s"
-          (plist-get status :key-exchange)
-	  (plist-get status :cipher)
-	  (plist-get status :mac)))
-
-(defun nsm-protocol-check--diffie-hellman-prime-bits (host port status bits)
-  (let ((prime-bits (plist-get status :diffie-hellman-prime-bits)))
-    (or (not prime-bits)
-        (>= prime-bits bits)
-	(nsm-query
-	 host port status :diffie-hellman-prime-bits
-	 "The Diffie-Hellman prime bits (%s) used for this connection to %s:%s is less than what is considered safe (%s)."
-	 prime-bits host port bits))))
-
-(defun nsm-protocol-check--3des (host port status _)
-  (or (not (string-match "\\b3DES\\b" (plist-get status :cipher)))
-      (nsm-query
-       host port status :rc4
-       "The connection to %s:%s uses the 3DES cipher (%s), which is believed to be unsafe."
-       host port (plist-get status :cipher))))
-
-(defun nsm-protocol-check--rc4 (host port status _)
-  (or (not (string-match "\\bRC4\\b" (nsm--encryption status)))
-      (nsm-query
-       host port status :rc4
-       "The connection to %s:%s uses the RC4 algorithm (%s), which is believed to be unsafe."
-       host port (nsm--encryption status))))
-
-(defun nsm-protocol-check--signature-sha1 (host port status _)
-  (let ((signature-algorithm
-         (plist-get (plist-get status :certificate) :signature-algorithm)))
-    (or (not (string-match "\\bSHA1\\b" signature-algorithm))
-        (nsm-query
-         host port status :signature-sha1
-         "The certificate used to verify the connection to %s:%s uses the SHA1 algorithm (%s), which is believed to be unsafe."
-         host port signature-algorithm))))
-
-(defun nsm-protocol-check--intermediate-sha1 (host port status _)
-  ;; Skip the first certificate, because that's the host certificate.
-  (cl-loop for certificate in (cdr (plist-get status :certificates))
+\f
+
+;; Key exchange checks
+
+(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'"
+  (let ((kx (plist-get status :key-exchange)))
+    (and (string-match "^\\bRSA\\b" kx)
+         (format-message
+          "RSA key exchange method (%s) that does not offer perfect forward secrecy"
+          kx))))
+
+(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.
+
+In 2014, the discovery of Logjam[1] had proven non-elliptic-curve
+Diffie-Hellman key exchange with < 1024 prime bit length to be
+unsafe. In subsequent years, it was discovered that there were
+rampant use of small subgroup prime or composite number for DHE
+by many servers, and thus allowed themselves to be vulnerable to
+backdoors[2]. Given the difficulty in validating DHE parameters,
+major browser vendors had started to remove DHE since
+2016[3]. Emacs stops short of banning DHE and terminating
+connection, but prompts the user instead.
+
+References:
+
+[1]: Adrian et al (2014). \"Imperfect Forward Secrecy: How
+Diffie-Hellman Fails in Practice\", `https://weakdh.org/'
+
+[2]: Dorey, Fong, and Essex (2016). \"Indiscreet Logs: Persistent
+Diffie-Hellman Backdoors in TLS.\",
+`https://eprint.iacr.org/2016/999.pdf'
+
+[3]: Chrome Platform Status (2017). \"Remove DHE-based ciphers\",
+`https://www.chromestatus.com/feature/5128908798164992'"
+  (let* ((kx (plist-get status :key-exchange))
+         (dhe-p (string-match "^\\bDHE\\b" kx))
+         (prime-bits (plist-get status :diffie-hellman-prime-bits))
+         msg)
+    (when dhe-p
+      (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)"
+                     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"
+                      kx))))
+      msg)))
+
+(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'"
+  (let ((kx (plist-get status :key-exchange)))
+    (and (string-match "\\bANON\\b" kx)
+         (format-message
+          "anonymous key exchange method (%s) can be unsafe"
+          kx))))
+
+(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,
+but can be enabled if requested.  This check is mainly provided to
+secure Emacs built with older version of GnuTLS.
+
+Reference:
+
+Schneier, Bruce (1996). Applied Cryptography (Second ed.). John Wiley
+& Sons. ISBN 0-471-11709-9."
+  (let ((kx (plist-get status :key-exchange)))
+    (and (string-match "\\bEXPORT\\b" kx)
+         (format-message
+          "EXPORT level key exchange method (%s) is insecure"
+          kx))))
+
+;; 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].
+
+Reference:
+
+Sullivan (Feb 2016). \"Padding oracles and the decline of CBC-mode
+cipher suites\",
+`https://blog.cloudflare.com/padding-oracles-and-the-decline-of-cbc-mode-ciphersuites/'"
+  (let ((cipher (plist-get status :cipher)))
+    (and (string-match "\\bCBC\\b" cipher)
+         (format-message
+          "CBC mode cipher (%s) can be insecure"
+          cipher))))
+
+(defun nsm-protocol-check--ecdsa-cbc-cipher (host port status)
+  "Check for CBC mode cipher usage under ECDSA key exchange.
+
+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].
+
+Due to still widespread use of CBC mode ciphers by servers, this
+function only checks for CBC mode cipher usage in combination with
+ECDSA key exchange, which is virtually non-existent[2].
+
+References:
+
+[1]: Sullivan (Feb 2016). \"Padding oracles and the decline of
+CBC-mode cipher suites\",
+`https://blog.cloudflare.com/padding-oracles-and-the-decline-of-cbc-mode-ciphersuites/'
+
+[2]: Chrome Platform Status (2017). \"Remove CBC-mode ECDSA ciphers in
+TLS\", `https://www.chromestatus.com/feature/5740978103123968'"
+  (let ((kx (plist-get status :key-exchange))
+        (cipher (plist-get status :cipher)))
+    (and (string-match "\\bECDSA\\b" kx)
+         (string-match "\\bCBC\\b" cipher)
+         (format-message
+          "CBC mode cipher (%s) can be insecure"
+          cipher))))
+
+(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
+of security.
+
+Reference:
+
+Barker, Elaine (Jan 2016). \"NIST Special Publication 800-57:
+Recommendation for Key Management Part 1: General\" (4 ed.),
+`https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r4.pdf'
+"
+  (let ((cipher (plist-get status :cipher)))
+    (and (string-match "\\b3DES\\b" cipher)
+         (format-message
+          "3DES cipher (%s) is insecure"
+          cipher))))
+
+(defun nsm-protocol-check--des-cipher (host port status)
+  "Check for DES ciphers.
+
+Using dedicated hardware costing less than $10k, a DES key used to
+encrypt a message can be brute-forced in a matter of days.  RFC
+5469[1] suggests TLS libraries should not enable this algorithm.
+
+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.
+
+Reference:
+
+[1]: P. Eronen, Ed (2009). \"DES and IDEA Cipher Suites for Transport
+Layer Security (TLS)\", `https://tools.ietf.org/html/rfc5469'"
+  (let ((cipher (plist-get status :cipher)))
+    (and (string-match "\\bDES\\b" cipher)
+         (format-message
+          "DES cipher (%s) is insecure"
+          cipher))))
+
+(defun nsm-protocol-check--rc4-cipher (host port status)
+  "Check for RC4 ciphers.
+
+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.
+
+Reference:
+
+Popov A (Feb 2015). \"Prohibiting RC4 Cipher Suites\",
+`https://tools.ietf.org/html/rfc7465'"
+  (let ((cipher (plist-get status :cipher)))
+    (and (string-match "\\bARCFOUR\\b" cipher)
+         (format-message
+          "RC4 cipher (%s) is insecure"
+          cipher))))
+
+(defun nsm-protocol-check--rc2-cipher (host port status)
+  "Check for RC2 ciphers.
+
+\"RC2 can be broken with one related-key query and about 2^34 chosen
+plaintexts\", which is a matter of seconds on 2018 hardware.
+
+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.
+
+Reference:
+
+Kelsey, Schneier, Wagner (1997). \"Related-Key Cryptanalysis of 3-WAY,
+Biham-DES,CAST, DES-X, NewDES, RC2, and TEA\",
+`https://www.schneier.com/academic/paperfiles/paper-relatedkey.pdf'"
+  (let ((cipher (plist-get status :cipher)))
+    (and (string-match "\\bRC2\\b" cipher)
+         (format-message
+          "RC2 cipher (%s) is insecure"
+          cipher))))
+
+(defun nsm-protocol-check--null-cipher (host port status)
+  "Check for NULL cipher suites.
+
+This function checks for NULL key exchange, cipher and message
+authentication code key derivation function.  As the name suggests, a
+NULL assigned for any of the above disables an integral part of the
+security properties that make up the TLS protocol."
+  (let ((suite (format "%s-%s-%s"
+                       (plist-get status :key-exchange)
+	               (plist-get status :cipher)
+	               (plist-get status :mac))))
+    (and (string-match "\\bNULL\\b" suite)
+         (format-message
+          "NULL cipher suite (%s) violates authenticity, integrity, or confidentiality guarantees"
+          suite))))
+
+;; Signature checks
+
+(defun nsm-protocol-check--sha1-sig (host port status)
+  "Check for SHA1 signatures on certificates.
+
+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.
+
+References:
+
+[1]: Stevens M, Karpman P et al (2017). \"The first collision for full
+SHA-1\", `https://shattered.io/static/shattered.pdf'
+[2]: Chromium Security Education TLS/SSL. \"Deprecated and Removed
+Features (SHA-1 Certificate Signatures)\",
+`https://www.chromium.org/Home/chromium-security/education/tls\#TOC-SHA-1-Certificate-Signatures'
+[3]: Jones J.C (2017). \"The end of SHA-1 on the Public Web\",
+`https://blog.mozilla.org/security/2017/02/23/the-end-of-sha-1-on-the-public-web/'
+[4]: Apple Support (2017). \"Move to SHA-256 signed certificates to
+avoid connection failures\",
+`https://support.apple.com/en-gb/HT207459'
+[5]: Microsoft Security Advisory 4010323 (2017). \"Deprecation of
+SHA-1 for SSL/TLS Certificates in Microsoft Edge and Internet Explorer
+11\",
+`https://docs.microsoft.com/en-us/security-updates/securityadvisories/2017/4010323'"
+  (cl-loop for certificate in (plist-get status :certificates)
            for algo = (plist-get certificate :signature-algorithm)
-           ;; Don't check root certificates -- SHA1 isn't dangerous
-           ;; there.
-           when (and (not (equal (plist-get certificate :issuer)
+           ;; Don't check root certificates -- root is always trusted.
+           if (and (not (equal (plist-get certificate :issuer)
                                  (plist-get certificate :subject)))
-                     (string-match "\\bSHA1\\b" algo)
-                     (not (nsm-query
-                           host port status :signature-sha1
-                           "An intermediate certificate used to verify the connection to %s:%s uses the SHA1 algorithm (%s), which is believed to be unsafe."
-                           host port algo)))
-           do (cl-return nil)
-           finally (cl-return t)))
-
-(defun nsm-protocol-check--ssl (host port status _)
+                     (string-match "\\bSHA1\\b" algo))
+           return (format-message
+                   "SHA1 signature (%s) is prone to collisions"
+                   algo)
+           end))
+
+(defun nsm-protocol-check--md5-sig (host port status)
+  "Check for MD5 signatures on certificates.
+
+In 2008, a group of researchers were able to forge an intermediate CA
+certificate that appeared to be legitimate when checked by MD5[1].
+RFC 6151 has recommended against the usage of MD5 for digital
+signatures, which includes TLS certificate signatures.
+
+References:
+
+[1]: Sotirov A, Stevens M et al (2008). \"MD5 considered harmful today
+- Creating a rogue CA certificate\",
+`http://www.win.tue.nl/hashclash/rogue-ca/'
+[2]: Turner S, Chen L (2011). \"Updated Security Considerations for
+the MD5 Message-Digest and the HMAC-MD5 Algorithms\",
+`https://tools.ietf.org/html/rfc6151'"
+  (cl-loop for certificate in (plist-get status :certificates)
+           for algo = (plist-get certificate :signature-algorithm)
+           ;; Don't check root certificates -- root is always trusted.
+           if (and (not (equal (plist-get certificate :issuer)
+                               (plist-get certificate :subject)))
+                   (string-match "\\bMD51\\b" algo))
+           return (format-message
+                   "MD5 signature (%s) is very prone to collisions"
+                   algo)
+           end))
+
+;; Protocol level checks
+
+(defun nsm-protocol-check--ssl (host port status)
+  "Check for SSL protocol version.
+
+This function guard against usage of SSL3.0, which has been deprecated
+by RFC7568.
+
+Reference:
+
+IETF (2015). \"Deprecating Secure Sockets Layer Version 3.0\",
+`https://tools.ietf.org/html/rfc7568'"
   (let ((protocol (plist-get status :protocol)))
-    (or (not protocol)
-	(not (string-match "SSL" protocol))
-	(nsm-query
-	 host port status :ssl
-	 "The connection to %s:%s uses the %s protocol, which is believed to be unsafe."
-	 host port protocol))))
+    (and protocol
+         (string-match "SSL" protocol)
+         (format-message
+	  " %s protocol is deprecated"
+	  protocol))))
+
+\f
 
 (defun nsm-fingerprint (status)
   (plist-get (plist-get status :certificate) :public-key-id))
@@ -293,10 +585,11 @@ nsm-fingerprint-ok-p
 	      (setq did-query
 		    (nsm-query
 		     host port status 'fingerprint
-		     "The fingerprint for the connection to %s:%s has changed from %s to %s"
-		     host port
-		     (plist-get settings :fingerprint)
-		     (nsm-fingerprint status)))))
+                     (format-message
+		      "The fingerprint for the connection to %s:%s has changed from %s to %s"
+		      host port
+		      (plist-get settings :fingerprint)
+		      (nsm-fingerprint status))))))
 	;; Not OK.
 	nil
       (when did-query
@@ -308,9 +601,10 @@ nsm-fingerprint-ok-p
 (defun nsm-new-fingerprint-ok-p (host port status)
   (nsm-query
    host port status 'fingerprint
-   "The fingerprint for the connection to %s:%s is new: %s"
-   host port
-   (nsm-fingerprint status)))
+   (format-message
+    "The fingerprint for the connection to %s:%s is new: %s"
+    host port
+    (nsm-fingerprint status))))
 
 (defun nsm-check-plain-connection (process host port settings warn-unencrypted)
   ;; If this connection used to be TLS, but is now plain, then it's
@@ -322,22 +616,24 @@ nsm-check-plain-connection
 	 (not
 	  (nsm-query
 	   host port nil 'conditions
-	   "The connection to %s:%s used to be an encrypted connection, but is now unencrypted.  This might mean that there's a man-in-the-middle tapping this connection."
-	   host port)))
+           (format-message
+	    "The connection to %s:%s used to be an encrypted connection, but is now unencrypted.  This might mean that there's a man-in-the-middle tapping this connection."
+	    host port))))
     (delete-process process)
     nil)
    ((and warn-unencrypted
 	 (not (memq :unencrypted (plist-get settings :conditions)))
 	 (not (nsm-query
 	       host port nil 'conditions
-	       "The connection to %s:%s is unencrypted."
-	       host port)))
+               (format-message
+	        "The connection to %s:%s is unencrypted."
+	        host port))))
     (delete-process process)
     nil)
    (t
     process)))
 
-(defun nsm-query (host port status what message &rest args)
+(defun nsm-query (host port status what message)
   ;; If there is no user to answer queries, then say `no' to everything.
   (if (or noninteractive
 	  nsm-noninteractive)
@@ -346,7 +642,7 @@ nsm-query
 	   (condition-case nil
                (intern
                 (car (split-string
-                      (nsm-query-user message args
+                      (nsm-query-user message
                                       (nsm-format-certificate status))))
                 obarray)
 	     ;; Make sure we manage to close the process if the user hits
@@ -364,7 +660,7 @@ nsm-query
 	(nsm-save-host host port status what response)
 	t))))
 
-(defun nsm-query-user (message args cert)
+(defun nsm-query-user (message cert)
   (let ((buffer (get-buffer-create "*Network Security Manager*")))
     (save-window-excursion
       ;; First format the certificate and warnings.
@@ -374,7 +670,7 @@ nsm-query-user
           (when (> (length cert) 0)
             (insert cert "\n"))
           (let ((start (point)))
-            (insert (apply #'format-message message args))
+            (insert message)
             (goto-char start)
             ;; Fill the first line of the message, which usually
             ;; contains lots of explanatory text.
@@ -391,32 +687,33 @@ nsm-query-user
 
 (defun nsm-save-host (host port status what permanency)
   (let* ((id (nsm-id host port))
+         (what (if (listp what) what (list what)))
 	 (saved
 	  (list :id id
 		:fingerprint (or (nsm-fingerprint status)
 				 ;; Plain connection.
 				 :none))))
-    (when (or (eq what 'conditions)
+    (when (or (memq 'conditions what)
 	      nsm-save-host-names)
       (nconc saved (list :host (format "%s:%s" host port))))
     ;; We either want to save/update the fingerprint or the conditions
     ;; of the certificate/unencrypted connection.
     (cond
-     ((eq what 'conditions)
+     ((memq 'conditions what)
       (cond
        ((not status)
 	(nconc saved '(:conditions (:unencrypted))))
        ((plist-get status :warnings)
 	(nconc saved
 	       (list :conditions (plist-get status :warnings))))))
-     ((not (eq what 'fingerprint))
+     ((not (memq 'fingerprint what))
       ;; Store additional protocol settings.
       (let ((settings (nsm-host-settings id)))
 	(when settings
 	  (setq saved settings))
 	(if (plist-get saved :conditions)
-	    (nconc (plist-get saved :conditions) (list what))
-	  (nconc saved (list :conditions (list what)))))))
+	    (nconc (plist-get saved :conditions) what)
+	  (nconc saved (list :conditions what))))))
     (if (eq permanency 'always)
 	(progn
 	  (nsm-remove-temporary-setting id)
-- 
2.18.0

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

[Noam Postavsky]

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

[Jimmy Yuen Ho Wong]

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)
>

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

[Noam Postavsky]

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

> 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),

That seems okay to me.  If the public key stays the same, that indicates
it's signed by the same private key holder that was trusted last time.
What did you expect the "actual fingerprint" to be, and how would using
it help?

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

[Eli Zaretskii]

> 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.

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

[Lars Ingebrigtsen]

The netsec branch has now been merged into the trunk and I'm closing
this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Andy Moreton]

On Mon 26 Aug 2019, Lars Ingebrigtsen wrote:

> The netsec branch has now been merged into the trunk and I'm closing
> this bug report.

After this merge, the network tests fail on 64 bit windows (MSYS2), and
using any network functionality results in crashes.

From emacs --no-desktop under gdb, running list-packages pops upo the
NSM "Certificate information" window and asks "Continue comnecting?".
Any of the asnwer for this question result in a crash.

warning: HEAP[emacs.exe]: 
warning: Invalid address specified to RtlFreeHeap( 0000000002720000, 0000000006037150 )

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
(gdb) #0  0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#1  0x00007ff919941467 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#2  0x00007ff919985290 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#3  0x00007ff9199356b8 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#4  0x00007ff9198c07a0 in ntdll!RtlGetCurrentServiceSessionId () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#5  0x00007ff9198bfba1 in ntdll!RtlFreeHeap () from C:\WINDOWS\SYSTEM32\ntdll.dll
No symbol table info available.
#6  0x000000040029e7aa in free_after_dump (ptr=0x6037150) at C:/emacs/git/emacs/master/src/w32heap.c:504
No locals.
#7  0x000000040018b7f4 in xfree (block=0x6037150) at C:/emacs/git/emacs/master/src/alloc.c:748
No locals.
#8  0x0000000400227bb0 in Fgnutls_format_certificate (cert=XIL(0xd97ad24)) at C:/emacs/git/emacs/master/src/gnutls.c:1616
        err = 0x0
        crt = 0x62247a0
        crt_data = {
          data = 0xd97fd28 "-----BEGIN CERTIFICATE-----\nMIIFUTCCBDmgAwIBAgISBOm+yiBCrLLuPB59/zwFk1v7MA0GCSqGSIb3DQEBCwUA\nMEoxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MSMwIQYDVQQD\nExpMZXQncyBFbmNyeXB0IEF1dGhvcml0eSBYMzAeFw"..., 
          size = 0x76f
        }
        out = {
          data = 0x6037150 "X.509 Certificate Information:\n\tVersion: 3\n\tSerial Number (hex): 04e9beca2042acb2ee3c1e7dff3c05935bfb\n\tIssuer: CN=Let's Encrypt Authority X3,O=Let's Encrypt,C=US\n\tValidity:\n\t\tNot Before: Wed Aug 07 20"..., 
          size = 0x101f
        }
        out_buf = 0x8968130 "X.509 Certificate Information:\n\tVersion: 3\n\tSerial Number (hex): 04e9beca2042acb2ee3c1e7dff3c05935bfb\n\tIssuer: CN=Let's Encrypt Authority X3,O=Let's Encrypt,C=US\n\tValidity:\n\t\tNot Before: Wed Aug 07 20"...
        result = XIL(0x5b20003)
#9  0x00000004001c2091 in funcall_subr (subr=0x4006667c0 <Sgnutls_format_certificate>, numargs=0x1, args=0xbfba20) at C:/emacs/git/emacs/master/src/eval.c:2876
        internal_argbuf = {XIL(0xbfb930), XIL(0x4001042d1), XIL(0xbfb940), make_fixnum(4295234243), XIL(0x4006667c0), XIL(0x10), XIL(0), XIL(0)}
        internal_args = 0xbfba20
[...snipped...]

Lisp Backtrace:
"gnutls-format-certificate" (0xbfba20)
"nsm-query-user" (0xbfc0a0)
"nsm-query" (0xbfc5b0)
"nsm-check-tls-connection" (0xbfcbc0)
"nsm-verify-connection" (0xbfd0c8)
"read-event" (0xbfdd78)
"sit-for" (0xbfe298)
"execute-extended-command" (0xbfea00)
"funcall-interactively" (0xbfe9f8)
"call-interactively" (0xbfeec0)
"command-execute" (0xbff3f8)

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

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 26 Aug 2019 14:13:18 +0100
> 
> On Mon 26 Aug 2019, Lars Ingebrigtsen wrote:
> 
> > The netsec branch has now been merged into the trunk and I'm closing
> > this bug report.
> 
> After this merge, the network tests fail on 64 bit windows (MSYS2), and
> using any network functionality results in crashes.
> 
> >From emacs --no-desktop under gdb, running list-packages pops upo the
> NSM "Certificate information" window and asks "Continue comnecting?".
> Any of the asnwer for this question result in a crash.
> 
> warning: HEAP[emacs.exe]: 
> warning: Invalid address specified to RtlFreeHeap( 0000000002720000, 0000000006037150 )
> 
> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
> 0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
> (gdb) #0  0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #1  0x00007ff919941467 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #2  0x00007ff919985290 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #3  0x00007ff9199356b8 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #4  0x00007ff9198c07a0 in ntdll!RtlGetCurrentServiceSessionId () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #5  0x00007ff9198bfba1 in ntdll!RtlFreeHeap () from C:\WINDOWS\SYSTEM32\ntdll.dll
> No symbol table info available.
> #6  0x000000040029e7aa in free_after_dump (ptr=0x6037150) at C:/emacs/git/emacs/master/src/w32heap.c:504
> No locals.
> #7  0x000000040018b7f4 in xfree (block=0x6037150) at C:/emacs/git/emacs/master/src/alloc.c:748
> No locals.
> #8  0x0000000400227bb0 in Fgnutls_format_certificate (cert=XIL(0xd97ad24)) at C:/emacs/git/emacs/master/src/gnutls.c:1616

That's because any memory allocated by GnuTLS cannot be freed by Emacs
via 'free' or 'xfree', it needs to be freed by calling 'gnutls_free'
(the GnuTLS manual clearly says that, btw).  That crashes, because on
Windows we use our own implementation of 'free', which uses a
different heap.

Please try the latest master, I tried to fix that.  Also, please tell
which tests specifically crashed/failed for you, I'd like to run them
here.

Thanks.

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

[Andy Moreton]

On Mon 26 Aug 2019, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 26 Aug 2019 14:13:18 +0100
>> 
>> On Mon 26 Aug 2019, Lars Ingebrigtsen wrote:
>> 
>> > The netsec branch has now been merged into the trunk and I'm closing
>> > this bug report.
>> 
>> After this merge, the network tests fail on 64 bit windows (MSYS2), and
>> using any network functionality results in crashes.
>> 
>> >From emacs --no-desktop under gdb, running list-packages pops upo the
>> NSM "Certificate information" window and asks "Continue comnecting?".
>> Any of the asnwer for this question result in a crash.
>> 
>> warning: HEAP[emacs.exe]: 
>> warning: Invalid address specified to RtlFreeHeap( 0000000002720000, 0000000006037150 )
>> 
>> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
>> 0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> (gdb) #0  0x00007ff9199863b7 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #1  0x00007ff919941467 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #2  0x00007ff919985290 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #3  0x00007ff9199356b8 in ntdll!memset () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #4  0x00007ff9198c07a0 in ntdll!RtlGetCurrentServiceSessionId () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #5  0x00007ff9198bfba1 in ntdll!RtlFreeHeap () from C:\WINDOWS\SYSTEM32\ntdll.dll
>> No symbol table info available.
>> #6  0x000000040029e7aa in free_after_dump (ptr=0x6037150) at C:/emacs/git/emacs/master/src/w32heap.c:504
>> No locals.
>> #7  0x000000040018b7f4 in xfree (block=0x6037150) at C:/emacs/git/emacs/master/src/alloc.c:748
>> No locals.
>> #8  0x0000000400227bb0 in Fgnutls_format_certificate (cert=XIL(0xd97ad24)) at C:/emacs/git/emacs/master/src/gnutls.c:1616
>
> That's because any memory allocated by GnuTLS cannot be freed by Emacs
> via 'free' or 'xfree', it needs to be freed by calling 'gnutls_free'
> (the GnuTLS manual clearly says that, btw).  That crashes, because on
> Windows we use our own implementation of 'free', which uses a
> different heap.

Agreed, but master built from commit 1071a4f still crashes in the same
way:

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00000000051d6248 in ?? () from C:\msys64\mingw64\bin\libgnutls-30.dll
(gdb) bt
#0  0x00000000051d6248 in ?? () from C:\msys64\mingw64\bin\libgnutls-30.dll
#1  0x0000000400227be4 in Fgnutls_format_certificate (cert=XIL(0xd8e36e4)) at C:/emacs/git/emacs/master/src/gnutls.c:1619
#2  0x00000004001c2091 in funcall_subr (subr=0x4006667c0 <Sgnutls_format_certificate>, numargs=0x1, args=0xbfba20) at C:/emacs/git/emacs/master/src/eval.c:2876
[snipped]

Lisp Backtrace:
"gnutls-format-certificate" (0xbfba20)
"nsm-query-user" (0xbfc0a0)
"nsm-query" (0xbfc5b0)
"nsm-check-tls-connection" (0xbfcbc0)
"nsm-verify-connection" (0xbfd0c8)
"read-event" (0xbfdd78)
"sit-for" (0xbfe298)
"execute-extended-command" (0xbfea00)
"funcall-interactively" (0xbfe9f8)
"call-interactively" (0xbfeec0)
"command-execute" (0xbff3f8)

(gdb) f 1
#1  0x0000000400227be4 in Fgnutls_format_certificate (cert=XIL(0xd8e36e4)) at C:/emacs/git/emacs/master/src/gnutls.c:1619
1619      gnutls_free (out.data);

(gdb) info locals
err = 0x0
crt = 0x61947a0
crt_data = {
  data = 0xd8e8588 "-----BEGIN CERTIFICATE-----\nMIIFUTCCBDmgAwIBAgISBOm+yiBCrLLuPB59/zwFk1v7MA0GCSqGSIb3DQEBCwUA\nMEoxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MSMwIQYDVQQD\nExpMZXQncyBFbmNyeXB0IEF1dGhvcml0eSBYMzAeFw"...,
  size = 0x76f
}
out = {
  data = 0x60fc3f0 "X.509 Certificate Information:\n\tVersion: 3\n\tSerial Number (hex): 04e9beca2042acb2ee3c1e7dff3c05935bfb\n\tIssuer: CN=Let's Encrypt Authority X3,O=Let's Encrypt,C=US\n\tValidity:\n\t\tNot Before: Wed Aug 07 20"...,
  size = 0x101f
}
out_buf = 0x9ae8690 "X.509 Certificate Information:\n\tVersion: 3\n\tSerial Number (hex): 04e9beca2042acb2ee3c1e7dff3c05935bfb\n\tIssuer: CN=Let's Encrypt Authority X3,O=Let's Encrypt,C=US\n\tValidity:\n\t\tNot Before: Wed Aug 07 20"...
result = XIL(0x5a90003)


In Fgnutls_format_certificate, should out_buf be freed after calling
build_string ? Removing both xfree(out.data) and xfree(out_buf) from the
end of that function does give me a running emacs.

    AndyM

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

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 26 Aug 2019 18:45:14 +0100
> 
> > That's because any memory allocated by GnuTLS cannot be freed by Emacs
> > via 'free' or 'xfree', it needs to be freed by calling 'gnutls_free'
> > (the GnuTLS manual clearly says that, btw).  That crashes, because on
> > Windows we use our own implementation of 'free', which uses a
> > different heap.
> 
> Agreed, but master built from commit 1071a4f still crashes in the same
> way:

Can you show a recipe?

> In Fgnutls_format_certificate, should out_buf be freed after calling
> build_string ?

I see no problem with that, since build_string copies the data.

> Removing both xfree(out.data) and xfree(out_buf) from the end of
> that function does give me a running emacs.

It may run, but it leaks memory.

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

[Andy Moreton]

On Mon 26 Aug 2019, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 26 Aug 2019 18:45:14 +0100
>> 
>> > That's because any memory allocated by GnuTLS cannot be freed by Emacs
>> > via 'free' or 'xfree', it needs to be freed by calling 'gnutls_free'
>> > (the GnuTLS manual clearly says that, btw).  That crashes, because on
>> > Windows we use our own implementation of 'free', which uses a
>> > different heap.
>> 
>> Agreed, but master built from commit 1071a4f still crashes in the same
>> way:
>
> Can you show a recipe?

1) Delete ~/.emacs.d/network-security.data (to ensure NSM will query the user).
2) Run "emacs -Q"
3) "M-x list-packages", and at the NSM prompt, answer 'a' or 's' or 'd'.

>> In Fgnutls_format_certificate, should out_buf be freed after calling
>> build_string ?
>
> I see no problem with that, since build_string copies the data.
>
>> Removing both xfree(out.data) and xfree(out_buf) from the end of
>> that function does give me a running emacs.
>
> It may run, but it leaks memory.

Agreed - it is a data point to explore the problem rather than a
solution.

    AndyM

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

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 26 Aug 2019 19:19:56 +0100
> 
> On Mon 26 Aug 2019, Eli Zaretskii wrote:
> 
> >> Agreed, but master built from commit 1071a4f still crashes in the same
> >> way:
> >
> > Can you show a recipe?
> 
> 1) Delete ~/.emacs.d/network-security.data (to ensure NSM will query the user).
> 2) Run "emacs -Q"
> 3) "M-x list-packages", and at the NSM prompt, answer 'a' or 's' or 'd'.

Looks like something is wrong with how I'm using gnutls_free when it
comes from the DLL.  I need to look into that.

> >> Removing both xfree(out.data) and xfree(out_buf) from the end of
> >> that function does give me a running emacs.
> >
> > It may run, but it leaks memory.
> 
> Agreed - it is a data point to explore the problem rather than a
> solution.

The xmalloc is actually not needed at all, because we can create the
Lisp string directly.  But that's not the problem here, the problem
is the call to gnutls_free.

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

[Andy Moreton]

On Mon 26 Aug 2019, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 26 Aug 2019 19:19:56 +0100
>> 
>> On Mon 26 Aug 2019, Eli Zaretskii wrote:
>> 
>> >> Agreed, but master built from commit 1071a4f still crashes in the same
>> >> way:
>> >
>> > Can you show a recipe?
>> 
>> 1) Delete ~/.emacs.d/network-security.data (to ensure NSM will query the user).
>> 2) Run "emacs -Q"
>> 3) "M-x list-packages", and at the NSM prompt, answer 'a' or 's' or 'd'.
>
> Looks like something is wrong with how I'm using gnutls_free when it
> comes from the DLL.  I need to look into that.

Found it I think. gnutls_free is a pointer of type gnutls_free_function,
so the value returned from GetProcAddress is a data pointer, not a
function address. Something like the following patch appears to work
i.e. no longer crashes with the recipe above.

If this is the right fix then we will need to add e.g. DEF_DLL_DATA and
LOAD_DLL_DATA macros for runtime import of data (and an equivalent to
get_proc_addr to use the right types for runtime data imports).

The patch also has "#undef gnutls_free" because gnutls.h also defines
gnutls_free as a macro, if GNUTLS_INTERNAL_BUILD is defined. For some
reason the MSYS2 maintainers add a patch to their build to define
GNUTLS_INTERNAL_BUILD unconditionally: see 0001-add-missing-define.patch
at <https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-gnutls>.


diff --git a/src/gnutls.c b/src/gnutls.c
index 67d1fb9552..c4bce62869 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -140,6 +140,7 @@ DEF_DLL_FN (void, gnutls_dh_set_prime_bits,
 DEF_DLL_FN (int, gnutls_dh_get_prime_bits, (gnutls_session_t));
 DEF_DLL_FN (int, gnutls_error_is_fatal, (int));
 DEF_DLL_FN (int, gnutls_global_init, (void));
+#undef gnutls_free /* Also defined as a macro in gnutls.h */
 DEF_DLL_FN (void, gnutls_free, (void *));
 DEF_DLL_FN (void, gnutls_global_set_log_function, (gnutls_log_func));
 #  ifdef HAVE_GNUTLS3
@@ -1616,7 +1617,7 @@ DEFUN ("gnutls-format-certificate", Fgnutls_format_certificate,
   memset (out_buf, 0, (out.size + 1) * sizeof (char));
   memcpy (out_buf, out.data, out.size);
 
-  gnutls_free (out.data);
+  (*((gnutls_free_function *)gnutls_free)) (out.data);
   gnutls_x509_crt_deinit (crt);
 
   Lisp_Object result = build_string (out_buf);

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

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 26 Aug 2019 23:56:06 +0100
> 
> > Looks like something is wrong with how I'm using gnutls_free when it
> > comes from the DLL.  I need to look into that.
> 
> Found it I think. gnutls_free is a pointer of type gnutls_free_function,
> so the value returned from GetProcAddress is a data pointer, not a
> function address. Something like the following patch appears to work
> i.e. no longer crashes with the recipe above.

Yes, thanks.  I was struggling with the same thing, but evidently
there's a limit to what I can do after 11PM at my age ;-)

> If this is the right fix then we will need to add e.g. DEF_DLL_DATA and
> LOAD_DLL_DATA macros for runtime import of data (and an equivalent to
> get_proc_addr to use the right types for runtime data imports).

I think there's no need to invent harness for a single user, we can
simply do that by hand.  Please try the patch below, it's against the
current master (it also removes the unnecessary intermediate buffer
allocation while at that).

> The patch also has "#undef gnutls_free" because gnutls.h also defines
> gnutls_free as a macro, if GNUTLS_INTERNAL_BUILD is defined. For some
> reason the MSYS2 maintainers add a patch to their build to define
> GNUTLS_INTERNAL_BUILD unconditionally: see 0001-add-missing-define.patch
> at <https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-gnutls>.

I guess they want for some reason not to have the exported functions
decorated with __declspec(dllimport).  I have no idea why, maybe for
reasons of function visibility?  It's worth asking them, I think,
because this kind of kludge can be a nuisance.

diff --git a/src/gnutls.c b/src/gnutls.c
index 67d1fb9..fb75eb9 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -140,7 +140,6 @@ DEF_DLL_FN (void, gnutls_dh_set_prime_bits,
 DEF_DLL_FN (int, gnutls_dh_get_prime_bits, (gnutls_session_t));
 DEF_DLL_FN (int, gnutls_error_is_fatal, (int));
 DEF_DLL_FN (int, gnutls_global_init, (void));
-DEF_DLL_FN (void, gnutls_free, (void *));
 DEF_DLL_FN (void, gnutls_global_set_log_function, (gnutls_log_func));
 #  ifdef HAVE_GNUTLS3
 DEF_DLL_FN (void, gnutls_global_set_audit_log_function, (gnutls_audit_log_func));
@@ -291,6 +290,7 @@ DEF_DLL_FN (const char *, gnutls_ext_get_name, (unsigned int));
 #   endif
 #  endif	 /* HAVE_GNUTLS3 */
 
+static gnutls_free_function *gnutls_free_func;
 
 static bool
 init_gnutls_functions (void)
@@ -327,7 +327,6 @@ init_gnutls_functions (void)
   LOAD_DLL_FN (library, gnutls_dh_get_prime_bits);
   LOAD_DLL_FN (library, gnutls_error_is_fatal);
   LOAD_DLL_FN (library, gnutls_global_init);
-  LOAD_DLL_FN (library, gnutls_free);
   LOAD_DLL_FN (library, gnutls_global_set_log_function);
 #  ifdef HAVE_GNUTLS3
   LOAD_DLL_FN (library, gnutls_global_set_audit_log_function);
@@ -430,6 +429,13 @@ init_gnutls_functions (void)
 #   endif
 #  endif	 /* HAVE_GNUTLS3 */
 
+  /* gnutls_free is a variable inside GnuTLS, whose value is the
+     "free" function.  So it needs special handling.  */
+  gnutls_free_func = (gnutls_free_function *) GetProcAddress (library,
+							      "gnutls_free");
+  if (!gnutls_free_func)
+    return false;
+
   max_log_level = clip_to_bounds (INT_MIN, global_gnutls_log_level, INT_MAX);
   {
     Lisp_Object name = CAR_SAFE (Fget (Qgnutls, QCloaded_from));
@@ -465,7 +471,6 @@ init_gnutls_functions (void)
 #  define gnutls_global_init fn_gnutls_global_init
 #  define gnutls_global_set_audit_log_function fn_gnutls_global_set_audit_log_function
 #  define gnutls_global_set_log_function fn_gnutls_global_set_log_function
-#  define gnutls_free fn_gnutls_free
 #  define gnutls_global_set_log_level fn_gnutls_global_set_log_level
 #  define gnutls_handshake fn_gnutls_handshake
 #  define gnutls_init fn_gnutls_init
@@ -562,6 +567,11 @@ init_gnutls_functions (void)
 #   endif
 #  endif	 /* HAVE_GNUTLS3 */
 
+/* gnutls_free is a data pointer to a variable which holds a pointer
+   to the function.  We use #undef because MinGW64 defines gnutls_free
+   as a macro as well in the GnuTLS headers.  */
+#  undef gnutls_free
+#  define gnutls_free (*gnutls_free_func)
 
 /* This wrapper is called from fns.c, which doesn't know about the
    LOAD_DLL_FN stuff above.  */
@@ -1612,16 +1622,10 @@ DEFUN ("gnutls-format-certificate", Fgnutls_format_certificate,
 	     emacs_gnutls_strerror (err));
     }
 
-  char *out_buf = xmalloc ((out.size + 1) * sizeof (char));
-  memset (out_buf, 0, (out.size + 1) * sizeof (char));
-  memcpy (out_buf, out.data, out.size);
-
+  Lisp_Object result = make_string_from_bytes (out.data, out.size, out.size);
   gnutls_free (out.data);
   gnutls_x509_crt_deinit (crt);
 
-  Lisp_Object result = build_string (out_buf);
-  xfree (out_buf);
-
   return result;
 }
 

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

[Andy Moreton]

On 27/08/2019 8:23 am, Eli Zaretskii wrote:
>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 26 Aug 2019 23:56:06 +0100
>> 
>> > Looks like something is wrong with how I'm using gnutls_free when it
>> > comes from the DLL.  I need to look into that.
>> 
>> Found it I think. gnutls_free is a pointer of type gnutls_free_function,
>> so the value returned from GetProcAddress is a data pointer, not a
>> function address. Something like the following patch appears to work
>> i.e. no longer crashes with the recipe above.
> 
> Yes, thanks.  I was struggling with the same thing, but evidently
> there's a limit to what I can do after 11PM at my age ;-)
> 
>> If this is the right fix then we will need to add e.g. DEF_DLL_DATA and
>> LOAD_DLL_DATA macros for runtime import of data (and an equivalent to
>> get_proc_addr to use the right types for runtime data imports).
> 
> I think there's no need to invent harness for a single user, we can
> simply do that by hand.  Please try the patch below, it's against the
> current master (it also removes the unnecessary intermediate buffer
> allocation while at that).

Your patch works (tested with package-list and gnus) - please install.

Thanks,

    AndyM

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

[Eli Zaretskii]

> Cc: 31946@debbugs.gnu.org
> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 27 Aug 2019 16:25:58 +0100
> 
> Your patch works (tested with package-list and gnus) - please install.

Done.  Thanks for helping and for testing the patches.

bug#31946: 27.0.50; GnuTLS still crashes on MinGW

[Markus Weber]

Just a side node: Commit f3f09189 did not prevent emacs from crashing
here, i.e. when refreshing packages with (package-list-packages).

Backtrace:
000000040019b974
00000004000b1af6
00000004000c9796
00000004000ca447
00000004001f265a
00007ffdb38a8040
00007ffdb5461197
00007ffdb542a221
00007ffdb545fe06
00000000052a6240

Many thanks!

In GNU Emacs 27.0.50 (build 1, x86_64-w64-mingw32)
 of 2019-08-27 built on MC-T530
Repository revision: d9e4d52a10a198b06333eb6523561cccc1a078c1
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0.18362
System Description: Microsoft Windows 10 Pro (v10.0.1903.18362.295)

bug#31946: 27.0.50; GnuTLS still crashes on MinGW

[Eli Zaretskii]

> From: Markus Weber <parlando@intergga.ch>
> Date: Tue, 27 Aug 2019 10:01:42 +0200
> 
> Just a side node: Commit f3f09189 did not prevent emacs from crashing
> here, i.e. when refreshing packages with (package-list-packages).

This bug is not completely solved yet, stay tuned.

bug#31946: 27.0.50; GnuTLS still crashes on MinGW

[Eli Zaretskii]

> Date: Tue, 27 Aug 2019 18:00:33 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 31946@debbugs.gnu.org
> 
> > From: Markus Weber <parlando@intergga.ch>
> > Date: Tue, 27 Aug 2019 10:01:42 +0200
> > 
> > Just a side node: Commit f3f09189 did not prevent emacs from crashing
> > here, i.e. when refreshing packages with (package-list-packages).
> 
> This bug is not completely solved yet, stay tuned.

Please try again now, should be fixed.

bug#31946: 27.0.50; GnuTLS still crashes on MinGW

[mw]

Is fixed. No issue discovered.

On 8/27/2019 5:50 PM, Eli Zaretskii wrote:
> Please try again now, should be fixed. 

bug#31946: 27.0.50; GnuTLS still crashes on MinGW

[Eli Zaretskii]

> Cc: 31946@debbugs.gnu.org
> From: mw <parlando@intergga.ch>
> Date: Thu, 29 Aug 2019 10:05:58 +0200
> 
> Is fixed. No issue discovered.

Thanks for testing.