nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.68k stars 29.1k forks source link

TLS secure context not honored after upgrade from v9.11.1 or v8.10.0 to v10 #20833

Closed alaindemour closed 6 years ago

alaindemour commented 6 years ago
tls.connect({
    host: 'catalogue.swanngalleries.com',
    port: 443,
    ciphers: 'DES-CBC3-SHA',
    secureProtocol: "TLSv1_method"
},  () => {
    console.log('SUCCESS')
}).on('error', function (err) {
    console.log('ERROR')
})

The code above attempts to connects to a web site that is very out of date for its TLS version (but "real world"), it works on v9.11.1 or v8.10.0, but fail on all v10 version v10.0.0 v10.1.0. Providing the context as a separate object does not help , it fails in the same way.

ryzokuken commented 6 years ago

/cc @nodejs/crypto

ryzokuken commented 6 years ago

I can confirm that this prints out "SUCCESS" in Node 8 while printing out "ERROR" in Node 10

ryzokuken commented 6 years ago

Couldn't find anything relevant in the "changes" block at https://nodejs.org/api/tls.html#tls_tls_connect_options_callback, so probably no semver-majors

ryzokuken commented 6 years ago

Okay, so removing the ciphers: 'DES-CBC3-SHA' part breaks it on Node 8 at well, maybe Node 10 isn't respecting this particular value?

Example:

const tls = require('tls');

const ctx = tls.createSecureContext({
  secureProtocol: "TLSv1_method",
})

tls.connect({
  host: 'catalogue.swanngalleries.com',
  port: 443,
  secureContext: ctx,
}, () => {
  console.log('SUCCESS')
}).on('error', function (err) {
  console.log('ERROR')
});
apapirovski commented 6 years ago

We upgraded OpenSSL and deprecated (/removed) a bunch of stuff in crypto for 10.0.0, seems like the most likely culprit.

ryzokuken commented 6 years ago

@apapirovski exactly. Looking at https://www.openssl.org/docs/man1.1.0/apps/ciphers.html#CIPHER-LIST-FORMAT, DES-CBC3-SHA isn't the best cipher to use anyway, so they're probably out of luck.

ryzokuken commented 6 years ago

Okay, a few observations:

  1. This cipher was part of the SSL 3.0 suite, published in 1996. (That's old)
  2. There's a known vulnerability in SSL v3 ciphers, particularly affecting block ciphers, so CBC should be affected.

In 2014, SSL 3.0 was found to be vulnerable to the POODLE attack that affects all block ciphers in SSL; and RC4, the only non-block cipher supported by SSL 3.0, is also feasibly broken as used in SSL 3.0.

  1. SSL 3.0 was prohibited from being used in 2015 by RFC 7568 (https://tools.ietf.org/html/rfc7568) that said, this prohibition might not apply to the cipher suite.

SSL 3.0 was also later prohibited in June 2015 by RFC 7568

  1. CBC is vulnerable to a padding oracle attack, which might not be relevant in this particular case, but worth considering: https://en.wikipedia.org/wiki/Padding_oracle_attack#Padding_oracle_attack_on_CBC_encryption
alaindemour commented 6 years ago

This site(which I am not affiliated with) and that I just gave as an example is clearly not doing a great job at keeping an up-to-date TLS profile, but as a client a node v10 program should be able to connect to it as version 8 or 9 could. All the browsers I tried can also do it i.e. chrome/firefox/safari etc ...

alaindemour commented 6 years ago

What I mean is that for a server, it seems reasonable to tighten up node security, but as a client it would seem reasonable to be able to connect to any site that a mainstream browser can connect to.

ryzokuken commented 6 years ago

But again, if clients don't deprecate old/crufty ciphers, servers will keep using them. They won't have any incentive to upgrade.

ryzokuken commented 6 years ago

@apapirovski Maybe you meant https://github.com/nodejs/node/pull/19794?

apapirovski commented 6 years ago

I think DES might no longer be supported in OpenSSL 1.1.0 based on some research. That said I'll let @nodejs/crypto speak to whether that's what's going on here.

ryzokuken commented 6 years ago

@apapirovski I believe DES-CBC3-SHA is not a DES cipher but a 3DES cipher instead.

ryzokuken commented 6 years ago

@alaindemour okay, I haven't spent a lot of time researching this, but the more I read, the more I think you shouldn't use DES-CBC3-SHA. It is very insecure, from what I can see.

  1. It's vulnerable to the SSL POODLE attack as I mentioned earlier. Read: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8730

  2. It's also vulnerable to a birthday attack called "SWEET32", which is kinda alarming, and you probably shouldn't use it at this point. Read: https://sweet32.info/ and https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-2183 and https://www.openssl.org/blog/blog/2016/08/24/sweet32/

It's probably affected by even more attacks, for a more detailed summary, check this: https://www.gracefulsecurity.com/tls-ssl-vulnerabilities/

bnoordhuis commented 6 years ago

I think DES might no longer be supported in OpenSSL 1.1.0 based on some research.

That's correct and, since this is not something we're going back on, I'll close out the issue. @ryzokuken amply documented why you shouldn't be using it anymore.

alaindemour commented 6 years ago

The overall approach (forcing deprecation of the TLS options way before common browsers do) means one cannot wrote a web crawler using node.js any more, a web crawler has to connect to whatever site is out there on the internet regardless of how badly configured there are (if they are used by human beings), it is not a "choice" of the developer to use this cipher or that cipher one has to use whatever the site actually mandate. This is essentially saying "web crawlers are not a good use case for node". Is it a tenable position?

bnoordhuis commented 6 years ago

Node.js aims to be secure by default. Nothing and no one is stopping you from building Node.js from source in a configuration that enables SSLv3 or insecure ciphers again, but it's not something we do - or will do - in release builds.

ryzokuken commented 6 years ago

Is it a tenable position?

@alaindemour it's not, because:

  1. Browsers could keep supporting <blink> for all we know, I don't think following "major browsers" or any browsers, in that regard is a priority for the project.

  2. If a website uses a cipher that was, and I quote, "prohibited from being used" over 3 years ago by the IETF itself, has vulnerabilities that are easily exploitable in ways known to the general public, I don't think we should support it TBH.

I don't think it's our problem if they haven't updated their website even after so many years of the deprecation. I mean, there's a reason things are deprecated, right? I don't think it would have taken them a lot of time to upgrade, and if a team cares so little about the security of their website and users, then they kinda bring this upon themselves, don't you think? This is straight out negligence on their part.

We definitely won't hamper the security of thousands of projects running Node.js and affect our security release cycle because of a single website that fails to upgrade to modern, secure ciphers.

rvagg commented 6 years ago

I'll second @bnoordhuis here: we're moving forward with secure-by-default, we now have OpenSSL 1.1.0 and will soon have 1.1.1 in Node 10+. We've been fairly aggressive in deprecating and removing broken cipher and protocol support from default builds. But you still have the option of building Node yourself to make it insecure-by-default if that's your use case. It's just not something we can afford to do for the broader user-base which expect a higher standard from Node.js. Moving forward I think we're going to see a pretty strong push toward AEAD ciphers thanks to the big jump in restrictions in TLS 1.3 (and therefore HTTP/2). 1.1.1 will give us TLS 1.3 and I imagine there to be a strong encouragement across the board to migrate for servers given that clients (browsers) are moving quickly on this.

(@ryzokuken thanks for your comment on the YouTube feed for the TSC meeting, I hadn't seen this discussion prior, I hope this answers your question)

alaindemour commented 6 years ago

I actually rewrote all my client code using Chrome Headless which seems to behave correctly when you pass the right TLS options. I understand that the community is probably stretched and does not want to go back on a major code surgery that probably involved tons of work, and this is not a mainstream use case for node, but it seems that this is really a product management decision happening as unintended consequences . Essentially this is saying one cannot and should not rely on node.js to write real world CLIENT applications , it puts node on a trajectory to become a niche platform only good enough to write certain classes of http servers. Note that this is something you can do reliably in just about any platform (JVM ecosystem, Golang, .NET ecosystem, C etc.) but not in node any more. It also probably means that to be consistent with this "point of view", there may be some other security related options in the node.js HTTPS client that should be depracted as they cannot be guaranteed to work in the future (i.e. for instance the TLS connection option "rejectUnauthorized: false" that let TLS connection happen with a bad certificate, which is also super useful because many real world sites have problematic certificates, but clearly will produce "unsafe" code

ryzokuken commented 6 years ago

If I understand you correctly (correct me if I don't), you're warranting for us to make core a tad less secure just because there's a "real world site" that doesn't conform to modern standards. I'm sorry to say that that would likely not happen. Yes, we're spread too thin, but I don't believe it would get reverted even if we weren't, because getting less secure moving forwards is counterintuitive and something we don't plan on doing.

Keep in mind, there will always be code in the wild that would work with outdated ciphers and other sorts of crazy stuff. Supporting them is not our job. It's not our fault that someone else doesn't care about their users' privacy, and tbh their users should probably use a different provider.

Care tell me what you think about https://www.blog.google/topics/safety-security/say-yes-https-chrome-secures-web-one-site-time/? Chrome will soon (if it doesn't already) block any non HTTPS websites, isn't that a welcome step according to you?

apapirovski commented 6 years ago

Node 8 is going to be actively supported until April 2019 and doesn't go EOL until after Oct 2019. If the site in question doesn't upgrade by then, I'm not sure that's something Node is obligated to support. There's nothing wrong with using Node 8 if this is functionality you need.

ryzokuken commented 6 years ago

Yeah, more importantly, that too. It's not that we've dropped support for Node 8, so you can still run that insecure code in Node 8 if you can and it'll work until April 2019. That said, I still won't suggest doing that, though.

alaindemour commented 6 years ago

my application is a crawler, do you think the google crawler refuses to crawl sites because they are poorly constructed or use obsolete tech? no, they find a way to crawl any site if it have legit/relevant data (like the one I gave as example). But as I said I moved all crawler code to Chrome Headless which works very well for this use case, so all is good.

ryzokuken commented 6 years ago

@alaindemour Great! I'm glad you were able to find a replacement.