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

Ability to check TLS contexts, servernames, altnames #24095

Closed
coolaj86 opened this issue Nov 5, 2018 · 9 comments
Closed

Ability to check TLS contexts, servernames, altnames #24095

coolaj86 opened this issue Nov 5, 2018 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@coolaj86
Copy link
Contributor

coolaj86 commented Nov 5, 2018

We have server.addContext(hostname, context), but we don't have a server.getContext(hostname).

Why?

Now, that might seem rather silly, but let me explain why it would be important to have this:

There's a type of spoofing known as Domain Fronting where the attacker creates a TLS session with one SNI, but once connected they send a different HTTP Host header (more info on a bug I created including this at #22389).

There's an RFC that specifically prohibits this behavior, which many cloud platforms follow, but node does not currently follow.

However, there's a catch. Another RFC in relation to HTTP/2 contradicts the former and suggests reusing the same TLS connection for a different servername when the certificate in the session includes the new servername (more info at https://www.ietf.org/mail-archive/web/httpbisa/current/msg28781.html).

Current versions of Firefox actually follow the second spec presently.

I want to block domain fronting, but not to the exclusion of allowing Firefox to reuse tls sessions.

Currently I'm checking that the Host header matches the tlsSocket.servername (when it exists), but in order to support Firefox I need to know the altnames (SAN) as well, but I haven't been able to figure out a way to get that information from the socket.

I'm looking for another workaround, but this seems like something that node could reasonably expose in a future version to make such checks easier.

@sam-github
Copy link
Contributor

re: server.getContext(hostname)... You would like the default SNICallback exposed? You can find it (totally unsupported) here:

server.on("secureConnection", (c) => {
  console.log('on connection');
  console.log('', c._tlsOptions.SNICallback);

It is the inverse of addContext(). I could arrange for it to be exposed if that is what you need.

However... I'm confused by your later discussion of the SubjectAlternativeNames... those aren't in the SecureContext. That makes it sound like you want the X.509 cert that was chosen to present to the client. That cert might be in the SecureContext, but you wouldn't know which one it was, and its not there in a parsed form that would be useful to you (I think).

If what you need is to know the cert that was actually selected by OpenSSL as matching the negotiated cipher suite, then that would be a different kind of API, not the inverse of addContext() you first proposed.

@coolaj86
Copy link
Contributor Author

coolaj86 commented Nov 8, 2018

Re default SNICallback: No, it's easy enough to keep track of that, but I don't have a clear use case for doing so.

Re SubjectAlternativeNames: Yes, I need the information found on the certificate(s) associated with a domain. The more technically correct answer would be that I should get the certificate that is associated with the current session. For all practical purposes, just knowing that in the venn diagram of certificates in the current session and certificates associated with the current tlsSocket.servername has overlap.

The goal

  • prevent domain fronting
  • without blocking Firefox (and future browsers) when reusing a single session for multiple servernames on the same certificate

Example

  • cert subject is example.com
  • cert altnames include example.com, whatever.com
  • A user starts at google.com
    • they find a result at whatever.com
    • they then click a link on that page that takes them to example.com
    • they exit the session by clicking a link on that page that takes them to foobar.net, which is also hosted on the same server but not listed in the certificate

Good

In firefox the TLS session is reused for the transition between whatever.com and example.com, but a new session is started for foobar.net. The tlsSocket.servername is incongruent with the example.com HTTP Host header. This is desirable behavior for efficiency. Even though foobar.net is on the same server, the session isn't reused because it's not listed on the certificate, for security.

Bad

If using a malicious client we don't want it to be allowed that the client establishes a connection at whatever.com and then uses the host header of foobar.net or internal.whatever.com. This could lead to subversive access (students on campus using a proxy to access blocked sites), unauthorized access (intranet/local sites or admin APIs exposed), cache poisoning (caching content for one domain at another), etc. In this case, if unchecked, the application proxy (perhaps node) thinks that the connection is for one site, getting the attacker through the firewall, and then the web server which has other resources is accessed and loaded.

Hence detecting which Subject/Altnames are inclusive/exclusive is desirable.

Current Workaround

Within SNICallback I'm doing the classic O(n^2) loop to map each domain to all names listed on its cert (and the same for each name on the cert) each time a new cert is loaded. It will yield false positives (different certs with some similar names extend each other's circles of trust), but not false negatives (firefox will never be blocked). I consider this Secure Enough for Now™, assuming the process isn't running for years on end (after certs have expired there's not an easy way to remove previous allowed names).

@sam-github
Copy link
Contributor

cert subject is example.com
cert altnames include whatever.com

People do this? The SAN is supposed to be an alternative to the subject that allows specifying normal internet names (emails, dns, etc), since the actual subject is a path in a non-existent OSI global directory tree, and didn't allow DNS names... except the CN= field is (ab)used to be a DNS name. I hadn't heard of people using both simultaneously, and putting DIFFERENT names in the two. What is the expected result of that?

@coolaj86
Copy link
Contributor Author

coolaj86 commented Nov 8, 2018

Many sites have done this over the years (ostensibly due to the expense of wildcards in comparison to listing multiple domains on a single cert). In particular virtual hosting sites (similar to but not necessarily including heroku and wix). I've personally witnessed it.

Let's Encrypt has rate limits, so they encourage putting up to 100 names on a single certificate: https://letsencrypt.org/docs/rate-limits/

However, perhaps the example I gave was too extreme. Keep everything else the same in what I said before, but change it to this:

  • cert subject is example.com
  • cert altnames include example.com, www.example.com, api.example.com, examplestaticcontent.com, sso.examplesecure.com

The exact same problem problem emerges: If Firefox reuses the session established with www.example.com when connecting to api.example.com, and they are both listed on the cert, I'd like to have a node-native way to distinguish that from Domain Fronting (they are not both listed on the cert).

@sam-github
Copy link
Contributor

@coolaj86 is #24261 what you were looking for?

@sam-github sam-github added the feature request Issues that request new features to be added to Node.js. label Nov 8, 2018
@coolaj86
Copy link
Contributor Author

coolaj86 commented Nov 8, 2018

Oooh! That looks very nice. It's not clear to me from the example the altnames are represented there, but it looks like they might be and I've got a way to figure that it if not.

While you're in that code... could you possibly also expose the function that parses the PEM as well?

Thanks a bunch! Do you have a "buy me a coffee" button somewhere?

@sam-github
Copy link
Contributor

could you possibly also expose the function that parses the PEM as well?

I'd like to, but that would take more thought as to API. It might even require a new X.509 object type. https://www.npmjs.com/package/asn1.js may be your best bet for that ATM.

@sam-github
Copy link
Contributor

SANs are supported, though as @bnoordhuis points out, there are no docs for the object fields. Sorry.

int nids[] = { NID_subject_alt_name, NID_info_access };

sam-github added a commit to sam-github/node that referenced this issue Nov 14, 2018
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: nodejs#24095
BridgeAR pushed a commit that referenced this issue Nov 14, 2018
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: #24095

PR-URL: #24261
Fixes: #24095
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this issue Nov 15, 2018
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: nodejs#24095

PR-URL: nodejs#24261
Fixes: nodejs#24095
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
BridgeAR pushed a commit that referenced this issue Nov 15, 2018
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: #24095

PR-URL: #24261
Fixes: #24095
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@coolaj86
Copy link
Contributor Author

Again, thank you.

sam-github added a commit to sam-github/node that referenced this issue Apr 29, 2019
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: nodejs#24095

PR-URL: nodejs#24261
Fixes: nodejs#24095
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants