From e6b35f4a86e788659d8ba2dada815492480a1382 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 May 2016 20:15:53 +0200 Subject: [PATCH 1/5] crypto: disable ssl compression at build time SSL compression was first disabled at runtime in March 2011 in commit e83c6959 ("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: https://github.com/nodejs/node/pull/6582 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- deps/openssl/openssl.gypi | 7 +++---- src/node_crypto.cc | 11 ++--------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/deps/openssl/openssl.gypi b/deps/openssl/openssl.gypi index 63286a1a64138c..73aff917d792f6 100644 --- a/deps/openssl/openssl.gypi +++ b/deps/openssl/openssl.gypi @@ -214,10 +214,6 @@ 'openssl/crypto/cms/cms_pwri.c', 'openssl/crypto/cms/cms_sd.c', 'openssl/crypto/cms/cms_smime.c', - 'openssl/crypto/comp/c_rle.c', - 'openssl/crypto/comp/c_zlib.c', - 'openssl/crypto/comp/comp_err.c', - 'openssl/crypto/comp/comp_lib.c', 'openssl/crypto/conf/conf_api.c', 'openssl/crypto/conf/conf_def.c', 'openssl/crypto/conf/conf_err.c', @@ -1252,6 +1248,9 @@ 'PURIFY', '_REENTRANT', + # Compression is not used and considered insecure (CRIME.) + 'OPENSSL_NO_COMP', + # SSLv3 is susceptible to downgrade attacks (POODLE.) 'OPENSSL_NO_SSL3', diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a143576227b80f..d46e8a42a9143d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5721,15 +5721,8 @@ void InitCryptoOnce() { // Turn off compression. Saves memory and protects against CRIME attacks. -#if !defined(OPENSSL_NO_COMP) -#if OPENSSL_VERSION_NUMBER < 0x00908000L - STACK_OF(SSL_COMP)* comp_methods = SSL_COMP_get_compression_method(); -#else - STACK_OF(SSL_COMP)* comp_methods = SSL_COMP_get_compression_methods(); -#endif - sk_SSL_COMP_zero(comp_methods); - CHECK_EQ(sk_SSL_COMP_num(comp_methods), 0); -#endif + // No-op with OPENSSL_NO_COMP builds of OpenSSL. + sk_SSL_COMP_zero(SSL_COMP_get_compression_methods()); #ifndef OPENSSL_NO_ENGINE ERR_load_ENGINE_strings(); From b261178a5204ca38183af9fe6c32720e46d84b51 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 May 2016 20:35:24 +0200 Subject: [PATCH 2/5] src: remove pre-openssl 1.0 legacy code 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: https://github.com/nodejs/node/pull/6582 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- src/node_crypto.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d46e8a42a9143d..d50671aa760e2b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -28,12 +28,6 @@ #define strcasecmp _stricmp #endif -#if OPENSSL_VERSION_NUMBER >= 0x10000000L -#define OPENSSL_CONST const -#else -#define OPENSSL_CONST -#endif - #define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val, prefix) \ do { \ if (!Buffer::HasInstance(val) && !val->IsString()) { \ @@ -351,7 +345,7 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { SecureContext* sc = Unwrap(args.Holder()); Environment* env = sc->env(); - OPENSSL_CONST SSL_METHOD *method = SSLv23_method(); + const SSL_METHOD* method = SSLv23_method(); if (args.Length() == 1 && args[0]->IsString()) { const node::Utf8Value sslmethod(env->isolate(), args[0]); @@ -1969,7 +1963,7 @@ void SSLWrap::GetCurrentCipher(const FunctionCallbackInfo& args) { Base* w = Unwrap(args.Holder()); Environment* env = w->ssl_env(); - OPENSSL_CONST SSL_CIPHER* c = SSL_get_current_cipher(w->ssl_); + const SSL_CIPHER* c = SSL_get_current_cipher(w->ssl_); if (c == nullptr) return; From 6db772d648d2f2a79a4426e6b06fe1de298cd80c Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 May 2016 20:41:42 +0200 Subject: [PATCH 3/5] src: remove unused #include statement strcasecmp() is not used in src/node_http_parser.cc so there is no need to include its header file. PR-URL: https://github.com/nodejs/node/pull/6582 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- src/node_http_parser.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index d1a130d7eead04..4a0dd580a40af6 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -15,12 +15,6 @@ #include // free() #include // strdup() -#if defined(_MSC_VER) -#define strcasecmp _stricmp -#else -#include // strcasecmp() -#endif - // This is a binding to http_parser (https://github.com/joyent/http-parser) // The goal is to decouple sockets from parsing for more javascript-level // agility. A Buffer is read from a socket and passed to parser.execute(). From f6940dfa4627301bbd1a24530ac3f4fe3fe8494c Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 May 2016 20:56:04 +0200 Subject: [PATCH 4/5] src: don't use locale-sensitive strcasecmp() 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: https://github.com/nodejs/node/pull/6582 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- src/node.cc | 23 +++++++++++------------ src/node_crypto.cc | 6 +----- src/util-inl.h | 12 ++++++++++++ src/util.h | 6 ++++++ test/cctest/util.cc | 18 ++++++++++++++++++ 5 files changed, 48 insertions(+), 17 deletions(-) diff --git a/src/node.cc b/src/node.cc index eb8805b2195ad6..0d85ce77c96cd3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -69,7 +69,6 @@ #if defined(_MSC_VER) #include #include -#define strcasecmp _stricmp #define getpid GetCurrentProcessId #define umask _umask typedef int mode_t; @@ -1381,27 +1380,27 @@ enum encoding ParseEncoding(const char* encoding, break; } - if (strcasecmp(encoding, "utf8") == 0) { + if (StringEqualNoCase(encoding, "utf8")) { return UTF8; - } else if (strcasecmp(encoding, "utf-8") == 0) { + } else if (StringEqualNoCase(encoding, "utf-8")) { return UTF8; - } else if (strcasecmp(encoding, "ascii") == 0) { + } else if (StringEqualNoCase(encoding, "ascii")) { return ASCII; - } else if (strcasecmp(encoding, "base64") == 0) { + } else if (StringEqualNoCase(encoding, "base64")) { return BASE64; - } else if (strcasecmp(encoding, "ucs2") == 0) { + } else if (StringEqualNoCase(encoding, "ucs2")) { return UCS2; - } else if (strcasecmp(encoding, "ucs-2") == 0) { + } else if (StringEqualNoCase(encoding, "ucs-2")) { return UCS2; - } else if (strcasecmp(encoding, "utf16le") == 0) { + } else if (StringEqualNoCase(encoding, "utf16le")) { return UCS2; - } else if (strcasecmp(encoding, "utf-16le") == 0) { + } else if (StringEqualNoCase(encoding, "utf-16le")) { return UCS2; - } else if (strcasecmp(encoding, "binary") == 0) { + } else if (StringEqualNoCase(encoding, "binary")) { return BINARY; - } else if (strcasecmp(encoding, "buffer") == 0) { + } else if (StringEqualNoCase(encoding, "buffer")) { return BUFFER; - } else if (strcasecmp(encoding, "hex") == 0) { + } else if (StringEqualNoCase(encoding, "hex")) { return HEX; } else { return default_encoding; diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d50671aa760e2b..71799d0bfa3ef3 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -24,10 +24,6 @@ #include #include -#if defined(_MSC_VER) -#define strcasecmp _stricmp -#endif - #define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val, prefix) \ do { \ if (!Buffer::HasInstance(val) && !val->IsString()) { \ @@ -4446,7 +4442,7 @@ void DiffieHellman::DiffieHellmanGroup( for (size_t i = 0; i < arraysize(modp_groups); ++i) { const modp_group* it = modp_groups + i; - if (strcasecmp(*group_name, it->name) != 0) + if (!StringEqualNoCase(*group_name, it->name)) continue; initialized = diffieHellman->Init(it->prime, diff --git a/src/util-inl.h b/src/util-inl.h index 7ff6e51937bde7..355dfcdbee3257 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -203,7 +203,19 @@ void SwapBytes(uint16_t* dst, const uint16_t* src, size_t buflen) { dst[i] = (src[i] << 8) | (src[i] >> 8); } +char ToLower(char c) { + return c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c; +} +bool StringEqualNoCase(const char* a, const char* b) { + do { + if (*a == '\0') + return *b == '\0'; + if (*b == '\0') + return *a == '\0'; + } while (ToLower(*a++) == ToLower(*b++)); + return false; +} } // namespace node diff --git a/src/util.h b/src/util.h index 1f9f81cfd38e16..dce6c343b1b443 100644 --- a/src/util.h +++ b/src/util.h @@ -178,6 +178,12 @@ inline TypeName* Unwrap(v8::Local object); inline void SwapBytes(uint16_t* dst, const uint16_t* src, size_t buflen); +// tolower() is locale-sensitive. Use ToLower() instead. +inline char ToLower(char c); + +// strcasecmp() is locale-sensitive. Use StringEqualNoCase() instead. +inline bool StringEqualNoCase(const char* a, const char* b); + // Allocates an array of member type T. For up to kStackStorageSize items, // the stack is used, otherwise malloc(). template diff --git a/test/cctest/util.cc b/test/cctest/util.cc index fe966d9b34073c..37133aca562b72 100644 --- a/test/cctest/util.cc +++ b/test/cctest/util.cc @@ -56,3 +56,21 @@ TEST(UtilTest, ListHead) { EXPECT_TRUE(list.IsEmpty()); EXPECT_FALSE(list.begin() != list.end()); } + +TEST(UtilTest, StringEqualNoCase) { + using node::StringEqualNoCase; + EXPECT_FALSE(StringEqualNoCase("a", "b")); + EXPECT_TRUE(StringEqualNoCase("", "")); + EXPECT_TRUE(StringEqualNoCase("equal", "equal")); + EXPECT_TRUE(StringEqualNoCase("equal", "EQUAL")); + EXPECT_TRUE(StringEqualNoCase("EQUAL", "EQUAL")); + EXPECT_FALSE(StringEqualNoCase("equal", "equals")); + EXPECT_FALSE(StringEqualNoCase("equals", "equal")); +} + +TEST(UtilTest, ToLower) { + using node::ToLower; + EXPECT_EQ('0', ToLower('0')); + EXPECT_EQ('a', ToLower('a')); + EXPECT_EQ('a', ToLower('A')); +} From a4f94b427170bdef44a4c4ba45ff36664fab6597 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 May 2016 21:27:18 +0200 Subject: [PATCH 5/5] deps: update comment about PURIFY define 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: https://github.com/nodejs/node/pull/6582 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- deps/openssl/openssl.gypi | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/deps/openssl/openssl.gypi b/deps/openssl/openssl.gypi index 73aff917d792f6..3620e45c410746 100644 --- a/deps/openssl/openssl.gypi +++ b/deps/openssl/openssl.gypi @@ -1244,10 +1244,14 @@ 'openssl/include', ], 'openssl_default_defines_all': [ - # No clue what these are for. - 'PURIFY', '_REENTRANT', + # 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. + 'PURIFY', + # Compression is not used and considered insecure (CRIME.) 'OPENSSL_NO_COMP',