Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update secureOptions parameter for https server #570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

inkz
Copy link

@inkz inkz commented Sep 25, 2020

Hello,

I’m a security researcher at r2c. We work with industry experts to write code checks for bugs in open source.

Problem:
I found that the https server don't disallow TLS v1 as it intended to do

secureOptions: constants.SSL_OP_NO_SSLv3 || constants.SSL_OP_NO_TLSv1,

secureOptions: constants.SSL_OP_NO_SSLv3 || constants.SSL_OP_NO_TLSv1,

TLS v1 is deprecated due to POODLE, man in the middle attacks, and other vulnerabilities.

Fix:
This is happens because TLS stack (OpenSSL) requires that the options are combined with bitwise OR while logical OR is used

// this is how it should be
secureOptions: constants.SSL_OP_NO_SSLv3 | constants.SSL_OP_NO_TLSv1

// this is how it is used now
secureOptions: constants.SSL_OP_NO_SSLv3 || constants.SSL_OP_NO_TLSv1

in this case only constants.SSL_OP_NO_SSLv3 is passed to secureOptions allowing connections with TLS v1

https://stackoverflow.com/questions/40434934/how-to-disable-the-ssl-3-0-and-tls-1-0-in-nodejs

We have a tool called Semgrep you can use for your project that continuously detects problems like this one. Semgrep is also available as a GitHub Action to make it easy to set up. The check that identified this bug is available in Semgrep by using https://semgrep.dev/p/colleend.insecure-transport-nodejs

Thanks, and I hope this helps! Let me know if you have any questions.

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2020

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants