set tls.rejectUnauthorized option as true by default (#25)

[ballinette]

cf. comment here: #25 (comment)

the option will be reset to "false" if "NODE_TLS_REJECT_UNAUTHORIZED=0" environment variable is present (here: https://github.com/gajus/global-agent/blob/master/src/classes/Agent.js#L161 )
But we need to be sure it will be set to "true" otherwise.

[gajus]

Setting this setting to true on its own does not make much sense because it will always fail. Unless I am overlooking something?

Are you also configuring ca? If yes, then the behaviour should be that if ca is set, then rejectUnauthorized should default to true.

[ballinette]

Hi
I will test the behaviour if I provide a ca attribute, but in my understanding of NodeJS documentation, this should not be mandatory:
https://nodejs.org/docs/latest-v12.x/api/tls.html#tls_tls_createsecurecontext_options

ca: Optionally override the trusted CA certificates. Default is to trust the well-known CAs curated by Mozilla. Mozilla's CAs are completely replaced when CAs are explicitly specified using this option.

For now, I've tested with a valid but expired certificate on the backend side (and so without any ca speciifed in my application).

• with rejectUnauthorized explicitly set to true => the request is rejected with a certificate has expired error, which is what I expect.
• without explicit value (so should by default have the same behaviour as above) => the request is not rejected.

[ballinette]

Hi again.
I''ve done additional tests to confirm the issue and the fix.

I've run different scenarios

• with the NODE_TLS_REJECT_UNAUTHORIZED environment variable left undefined, or set to 0 or to 1.
• without providing the ca attribute, or with some different values for this attribute.
• and requesting a public url (https://example.org) or a private one signed with a custom certificate.

The code is like following:

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

const globalAgent = require('global-agent');
globalAgent.bootstrap();

/**
* call 'example.org' to test requests to a public endpoint with a well-known public certificate
* then retry with a custom 'my.private.host' hostname to test requests to a private endpoint with a self-signed certificate.
const hostname = '.....';
*/

const options = {
  hostname,
  port: 443,
  path: '/',
  method: 'GET',
};

/**
* 3 steps to be tested:
* - without giving the 'ca' parameter (ie. leave following line commented)
* - with default Certificates Authority from OS (ie. in Ubuntu: /etc/ssl/certs/ca-certificates.crt )
* - with the self-signed certificate used for my.private.host
options.ca = fs.readFileSync('/path/to/ca/cert');
*/

// Call the endpoint and log response headers or error:
const req = https.request(options, (res) => {
  console.log('statusCode:', res.statusCode);
  console.log('headers:', res.headers);
});

req.on('error', (e) => {
  console.error(e);
});
req.end();

Here are the results, with actual version of global-agent, and with my fix:

---

hostname = 'example.org'

(public hostname signed with valid known certificate)

• CA attribute unset (ie. use Mozilla default Certificate authority)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request accepted	accepted	accepted
1	request accepted	accepted	accepted
0	request accepted	accepted	accepted

---

• ca = fs.readFileSync('/etc/ssl/certs/ca-certificates.crt') (ie. system default Certificate authority)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request accepted	accepted	accepted
1	request accepted	accepted	accepted
0	request accepted	accepted	accepted

---

• ca = fs.readFileSync('/my/custom/certificate.crt') (ie. custom Certificate authority - which does not validate example.org certificate)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request rejected	accepted	rejected
1	request rejected	accepted	rejected
0	request accepted	accepted	accepted

---

hostname = 'my.private.host'

(private hostname signed with /my/custom/certificate.crt)

• CA attribute unset (ie. use Mozilla default Certificate authority)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request rejected	accepted	rejected
1	request rejected	accepted	rejected
0	request accepted	accepted	accepted

---

• ca = fs.readFileSync('/etc/ssl/certs/ca-certificates.crt') (ie. system default Certificate authority)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request rejected	accepted	rejected
1	request rejected	accepted	rejected
0	request accepted	accepted	accepted

---

• ca = fs.readFileSync('/my/custom/certificate.crt') (ie. custom Certificate authority - which does validate my.private.host certificate)

NODE_TLS_REJECT_UNAUTHORIZED	Expected Behaviour	Before fix	After fix
unset	request accepted	accepted	accepted
1	request accepted	accepted	accepted
0	request accepted	accepted	accepted

---

tested with node 12 in Ubuntu 20.04

[ballinette]

Note: thinking again about the issue, I see another problem with actual code :
If I set the environment varialbe NODE_TLS_REJECT_UNAUTHORIZED to 0 BUT in the meantime I explicity set rejectUnauthorized request opttion to true, ie. same code as above, with following options:

const options = {
  hostname,
  port: 443,
  path: '/',
  method: 'GET',
  rejectUnauthorized: true
};

the request shoud be rejected if the server's certificate is invalid.

That is the behaviour while sending the request directly without using the global-agent proxy.
But with global-agent, the request is not rejected yet.

We should rather delete lines 156-163 and replace line 149, like that:
https://github.com/ballinette/global-agent/pull/1/files

(NB: note that according to NodeJS official documentation, the only value of the env var that disables the certificate validation is '0')

[GammaPi]

Another test:
hostname = 'www.bing.com'
(public hostname signed with trusted CA(trusted by the system))

CA attribute unset (ie. use Mozilla default Certificate authority)

rejectUnauthorized	Expected Behaviour	Actual Behaviour
unset	request accepted	accepted
true	request accepted	accepted
false	request accepted	accepted

So why set rejectUnauthorized=false in the first place since is not the case?

Test environment:
Ubuntu 18.04

Test code

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

const globalAgent = require('global-agent');
globalAgent.bootstrap();

/**
 * call 'example.org' to test requests to a public endpoint with a well-known public certificate
 * then retry with a custom 'my.private.host' hostname to test requests to a private endpoint with a self-signed certificate.
 const hostname = '.....';
 */
const hostname = 'www.bing.com'

const options = {
  hostname,
  port: 443,
  path: '/',
  method: 'GET',
  rejectUnauthorized: //Change this value to true/false
};

/**
 * 3 steps to be tested:
 * - without giving the 'ca' parameter (ie. leave following line commented)
 * - with default Certificates Authority from OS (ie. in Ubuntu: /etc/ssl/certs/ca-certificates.crt )
 * - with the self-signed certificate used for my.private.host
 options.ca = fs.readFileSync('/path/to/ca/cert');
 */

// Call the endpoint and log response headers or error:
const req = https.request(options, (res) => {
  console.log('statusCode:', res.statusCode);
  console.log('headers:', res.headers);
});

req.on('error', (e) => {
  console.error(e);
});
req.end();

[pimterry]

+1000. As it is this appears to be a critical security vulnerability for everybody using this package right now.

This completely disables TLS for connections beyond the proxy, so anybody (the proxy or anybody beyond the proxy on their network route) could impersonate any server at all.

With this defaulting to false, all HTTPS protections are disabled for all proxied traffic, breaking all the security of TLS.

Note that Node.js itself strongly discourages ever setting NODE_TLS_REJECT_UNAUTHORIZED to 0 for this exact reason:

If value equals '0', certificate validation is disabled for TLS connections. This makes TLS, and HTTPS by extension, insecure. The use of this environment variable is strongly discouraged.

As context, there's two cases here I think:

For the common case of visiting a normal HTTPS site via a non-intercepting proxy (which takes a CONNECT and creates a direct tunnel upstream), this isn't required. The TLS connection will be made directly with the remote server, which will have a normal trusted CA certificate like any other site, and we should verify the connection to that server like any other HTTPS connection (so we don't need to disable rejectUnauthorized).

The case where you might need to change how verification works is for an HTTPS-intercepting proxy. This does happen in some enterprise environments, where the proxy uses a trusted company CA certificate to decrypt, examine, and then proxy all data upstream (to scan for leaking company secrets, scan responses for viruses, this kind of thing).

Even in that case though, nobody should use rejectUnauthorized: false because that disables all HTTPS security, so somebody else on the internal network could intercept and steal/manipulate any of their traffic. Instead, this should be done using the ca argument or NODE_EXTRA_CA_CERTS to explicitly trust only the specific CA certificate that will be used for interception. That's how that would be done without this module too.

In short: nobody should be using rejectUnauthorized: false when using a proxy. Doing so is extremely dangerous. There will be edge cases and its useful for quick hacks, but definitely not by default or for normal code. This should be set to true by default here, to match node's default (although I think this PR isn't quite right: shouldn't it be ?? rather than ||, or some other != undefined check? Otherwise if it's explicitly set to false then it just becomes true again).

I'd suggest adding a test too, to ensure that bad certificates are rejected when using default settings (badssl.com is widely used for testing this).

I've just tested manually, and it seems that this issue still affects the new v3 release. Unless I'm missing something (possible?) this is a very serious vulnerability, and probably needs a formal CVE vulnerability report to be published too, to ensure everybody using this module is notified and updates immediately. Anybody using the affected versions today with the default settings is vulnerable to trivial attacks against all of their HTTPS traffic.

[pimterry]

(I've sent an email to @gajus to discuss some of the sensitive details of this and work out a good solution)

[pimterry]

This is now resolved separately by #48, in addition to another issue, so I think this can be closed.