crypto: disable ssl compression at build time #6582

bnoordhuis · 2016-05-04T22:29:50Z

R=@nodejs/crypto

CI: https://ci.nodejs.org/job/node-test-pull-request/2508/

SSL compression was first disabled at runtime in March 2011 in commit e83c695 ("Disable compression with OpenSSL.") for performance reasons and was later shown to be vulnerable to information leakage (CRIME.) Let's stop compiling it in altogether. This commit removes a broken CHECK from src/node_crypto.cc; broken because sk_SSL_COMP_num() returns -1 for a NULL stack, not 0. As a result, node.js would abort when linked to an OPENSSL_NO_COMP build of openssl. PR-URL: nodejs#6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

SSL_CIPHER and SSL_METHOD are always const with the version of openssl that we support, no need to check OPENSSL_VERSION_NUMBER first. PR-URL: nodejs#6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

strcasecmp() is not used in src/node_http_parser.cc so there is no need to include its header file. PR-URL: nodejs#6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

strcasecmp() is affected by the current locale as configured through e.g. the LC_ALL environment variable and the setlocale() libc function. It can result in unpredictable results across systems so replace it with a function that isn't susceptible to that. PR-URL: nodejs#6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PURIFY makes OpenSSL zero out some buffers. It also stops RAND_bytes() from using the existing contents of the destination buffer as a source of entropy, which according to some papers, is a possible attack vector for reducing the overall entropy. PR-URL: nodejs#6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

indutny · 2016-05-04T23:08:51Z

A bit diverse PR, but LGTM

jasnell · 2016-05-04T23:23:03Z

Yeah, there's a few different things going on in here but nothing that stands out as a concern. LGTM if CI is green. Disabling ssl compression is +++

addaleax · 2016-05-05T01:09:56Z

LGTM

Fishrock123 · 2016-05-05T14:11:06Z

@bnoordhuis this isn't stuff that needs to land in v6.1.0, is it?

bnoordhuis · 2016-05-06T10:53:40Z

@Fishrock123 b261178...f6940df could be back-ported, they're arguably bug fixes. Maybe I should've filed a separate PR for those but in my head they were all connected.

Fishrock123 · 2016-05-06T14:38:07Z

I'll just pull them into next weeks release, it's fine.

SSL compression was first disabled at runtime in March 2011 in commit e83c695 ("Disable compression with OpenSSL.") for performance reasons and was later shown to be vulnerable to information leakage (CRIME.) Let's stop compiling it in altogether. This commit removes a broken CHECK from src/node_crypto.cc; broken because sk_SSL_COMP_num() returns -1 for a NULL stack, not 0. As a result, node.js would abort when linked to an OPENSSL_NO_COMP build of openssl. PR-URL: #6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

SSL_CIPHER and SSL_METHOD are always const with the version of openssl that we support, no need to check OPENSSL_VERSION_NUMBER first. PR-URL: #6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

strcasecmp() is not used in src/node_http_parser.cc so there is no need to include its header file. PR-URL: #6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

strcasecmp() is affected by the current locale as configured through e.g. the LC_ALL environment variable and the setlocale() libc function. It can result in unpredictable results across systems so replace it with a function that isn't susceptible to that. PR-URL: #6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PURIFY makes OpenSSL zero out some buffers. It also stops RAND_bytes() from using the existing contents of the destination buffer as a source of entropy, which according to some papers, is a possible attack vector for reducing the overall entropy. PR-URL: #6582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ahshah · 2016-05-18T05:24:18Z

I'm curious to know why the PURIFY flag is set for nodejs openssl?

indutny · 2016-05-18T05:29:15Z

@ahshah have you seen the comment here ?

ahshah · 2016-05-18T05:46:02Z

I did- but after your note, I'm unsure what the comment is trying to say.
Is it claiming that an attack vector is created when entropy is reduced (by not using the destination buffer as a source for entropy)?
Or is it claiming that an attack vector is created by using the destination buffer as a source of entropy (because an attack could manipulate the source of entropy)

indutny · 2016-05-18T05:51:28Z

@ahshah latter 😉

MylesBorins · 2016-06-02T00:22:05Z

@bnoordhuis LTS?

bnoordhuis · 2016-06-02T08:36:25Z

@thealphanerd This PR could be back-ported in its entirety although technically, dropping SSL compression from the build is a change that could stop some native add-ons from loading.

I've never seen such add-ons though, and if they do exist, they're insecure. You could argue it's better if they stop working altogether.

Commits b261178...f6940df are safe, at any rate.

MylesBorins · 2016-07-11T20:15:02Z

@bnoordhuis b261178...f6940df is not landing vleanly on v4.x would you be willing to do a manual backport?

bnoordhuis · 2016-07-11T21:10:48Z

#7660