search

nodejs / node

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

Validate the SNI host and the HTTP Host header #22389

Closed coolaj86 closed 1 year ago

coolaj86 commented 6 years ago

The Specs

Summary

Essentially these say that the SNI field and the Host field are defined as strings limited to the following characters: a-z0-9.-_, which the addition of : to specify a port in some cases. Obviously there are some other restrictions such as the use of _ and a leading character in front of an "A" label, no numbers in TLDs, restriction on strength length per label between .s, etc.

Attacks

There are at least two classes of attack that are very easy to do, but shouldn't be possible by parsers follow the spec.

SNI - SQL Injection, Timing Attack

https.SNICallback will happily passes invalid SNI bytes, such as

SQL

Robert'); DROP TABLE Student;

So if I could guess that a client was doing some sort of virtual hosting and vulnerable to sql injection, I could easily (albeit blindly) send SQL commands:

openssl s_client -connect test.ppl.family:443 -servername "Robert'); DROP TABLE Students;" -showcerts

Paths

../probing/a/path/or/file

If I know that the server is using fs calls to find certificates to load based on the SNI then I can repeatedly send paths over and over and distinguish slight but consistent variances in the the time it takes to get an error in order to know whether or not certain target files exist on the file system, which may lead to further exploit opportunities.

openssl s_client -connect test.ppl.family:443 -servername "../probing/a/path/or/file" -showcerts

Host Header - SQL Injection, Timing Attack

The host header is susceptible to the same attacks in nearly the same way, excepting that it is more potentially dangerous since the attacker can get back some sort of plain-text data with either a leading error message or the actual target data in many of the affected cases:

curl https://test.ppl.family  -H "Host: Robert'); DROP TABLE Students;"
curl https://test.ppl.family  -H "Host: ../probing/a/path/or/file"

Again, I don't know what use malformed headers would be to a legitimate client or server and, whatever they may be, they're off-spec.

Domain Fronting

A different, but related vulnerability is that the tls servername and Host header can be different, this is known as domain fronting.

For example, In a shared hosting environments the SNI may come in as foo.com and the host header could be set to bar.net. I don't think this introduces any new attack and the risk is relatively low from what I know (WRONG, see below), but it is subversive behavior.

Update: After reading https://portswigger.net/blog/practical-web-cache-poisoning I could definitely see how domain fronting could be used in DoS and DDOS attacks - like causing a reverse proxy to download lots of extremely large files or proxy internal private resources.

Non-Attack Use Case: Knocking

The current behavior does enable "knocking" - sending arbitrary as a side channel for an application-specific protocol. A traditional example of knocking is sending a ping or "magic packet" to a certain port on a server which then causes a firewall rule to be enabled or disabled.

Personally, I think that's a pretty cool like the idea of being able to pass arbitrary bytes in order to perform SNI knocking or Host knocking.

However, this is not supported by the spec and is a very very edge case that would be better hid behind a configuration option - or even require the user to drop down to TCP to do.

I just mention this as a way of acknowledging an off-spec, but valid-ish use case.

Video Demonstration

https://youtu.be/aZgVqPzoZTY?list=PLZaEVINf2Bq_lrS-OOzTUJB4q3HxarlXk

Note: I created the video as a tutorial for Greenlock.js explaining some of the security features and afterwards I realized that this is probably the sort of thing that belongs in core rather than sparsely implemented here and there among frameworks.

Why Not Let "The Community" Deal With It?

Node isn't just for the technical elite anymore. I would imagine that its community has more script kiddies from boot camps than seasoned programmers or security professionals. Even the major frameworks - like express - are not mitigating these attacks (I'm using express in the video demo).

Since these are issues that deal directly with the specs for the related standards, they affect all frameworks and libraries alike. I believe it should be the responsibility of node, which is parsing the data in the first place, to put at least a small measure of protection in place by better adhering to the existing standards.

Proposed Solution

I propose that as node is parsing SNI and Host Headers that it reject, with the most appropriate error code, any requests that come in which arbitrary bytes that cannot be parsed according to the specification.

As a stop-gap solution I'd recommend simply matching case-insensitively on the set of the 39 allowed characters a-z0-9.-_.

I also suggest that there be a flag in the TLS,HTTP, HTTP2, and HTTPS modules when creating a server that allows these arbitrary bytes be passed rather than returning an error just in case someone is doing something off-spec, such as SNI or Host knocking.

coolaj86 commented 6 years ago

Patch Pending Discussion

Also, I'm familiar enough with node's internals that I could probably find the affected areas and patch them. However, I want to have the discussion before I go through the work.

addaleax commented 6 years ago

@nodejs/http-parser @nodejs/http2 @nodejs/security

Trott commented 6 years ago

@nodejs/security-wg

addaleax commented 6 years ago

Re-ping @nodejs/security @nodejs/security-wg

@coolaj86 I think the points you bring up are valid and it makes sense to address them in Node core. My personal feeling would be that you can go ahead and create pull requests for these items, with opt-out switches like you suggested.

There’s some information on contributing in https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md, but we’re also here and on IRC (#node-dev on Freenode) if you need help.

addaleax commented 6 years ago

Oh, actually: Maybe add them with an opt-in switch first, then follow-up PRs to switch the default to opt-out. This would make sure we can introduce these as semver-minor changes that can be backported to older release lines.

vdeturckheim commented 6 years ago

I tend to agree with @addaleax here. @coolaj86 would you open such PR? I'd be happy to help you doing so should you need it.

ChALkeR commented 6 years ago

This is not directly related to SQL injection, as the SQL injection in this example comes from the lack of (automatic?) escaping of the hostname in user code, not from the lack of servername validation. I.e. if the user code and has SQLi vulnerabilities of that kind, it is unlikely that fixing just servername validation on Node.js side would fix those. The real solution is to not build SQL queries by concatenating input. Same for filenames - paths that come from user input should be validated, in user code.

That said, we shouldn't allow values that are not permitted by the spec there. What @addaleax said sounds like a good plan.

coolaj86 commented 6 years ago

@vdeturckheim At the time that I opened the issue I had more time, but it'll be a bit before I can get back around to this to open a PR, but it is on my radar.

@ChALkeR I agree that properly escaping will negate the SQLi vulnerability, but a user could reasonably expect that bytes coming from the TLS module, which is supposed to handle security, would be properly validated ahead-of-time.

I found another complication: An HTTP/2 related RFC spec actually encourages "Domain Fronting" to efficiently reuse existing TLS connections when possible. Current versions of Firefox follow this new standard.

https://www.ietf.org/mail-archive/web/httpbisa/current/msg28781.html

coolaj86 commented 6 years ago

@vdeturckheim Would you happen to know how I could check the altnames on the certificate in the context of an existing tls connection? There's nothing publicly documented about how to get the list of contexts.

Trott commented 5 years ago

Is this being actively worked on? I assume it should not be closed, should it? Should we add a help wanted label?

awwright commented 5 years ago

I don't think this needs any action. There's no evidence this is related to SQL, and I think the title of this issue is misleading and at the very least should be renamed. (There's a way to fix SQL injection attacks, and that's by prohibiting dynamic strings or user input in SQL.) I find it highly dubious that someone might write an application where the only place there's unescaped input is the SNI field, which is typically automated away and is unused by application developers.

If there's a concern to be raised, it's going to be request smuggling, where, for example, a proxy interprets characters in the server_name field differently than an origin server. Perhaps one server stops parsing at a NUL character, and another does not. There's no evidence this can lead to an attack either.

Also, TLS and HTTP do not actually have mandatory dependencies on hostnames. For example, when using HTTP to look up a UUID URI, the Host header will be blank, because there is no // authority component of the URI. Likewise, TLS specifically allows extensions to the server_name field besides hostnames.

If we want to reject SNI hostnames for performance reasons (i.e. so the application doesn't have to do the validation), let's begin by looking at how other implementations handle invalid hostnames in the server_name field.

ChALkeR commented 5 years ago

I don't think this needs any action.

I disagree on that, this issue seems correct that SNI and Host header should probably be verified according to the spec (if they are not yet), and invalid requests should be rejected.

There's no evidence this is related to SQL, and I think the title of this issue is misleading and at the very least should be renamed.

I agree to that, this has nothing to do with SQL for the exact reasons stated above, and the issue title is indeed misleading.

let's begin by looking at how other implementations handle invalid hostnames in the server_name field.

E.g. nginx.

Perhaps the first thing that this issue needs is at least one failing testcase for the expected behaviour, aligned with nginx.

bnoordhuis commented 5 years ago

Is anyone planning to work on this? It's been open for a year without much action, only discussion.

Do I summarize correctly that the ask is to validate the SNI host and the HTTP Host header?

nginx was mentioned. It validates the host name but not until the HTTP phase of the request. The SNI host is treated as an opaque byte array. (So does openssl, by the way.)

For the interested, nginx's validation logic is here:

https://github.com/nginx/nginx/blob/a43974f3f257be632f7b41be6b034d37a3d2bbc3/src/http/ngx_http_request.c#L2059-L2147

coolaj86 commented 5 years ago

Do I summarize correctly that the ask is to validate the SNI host and the HTTP Host header?

Yes.

I maintain Greenlock, the node Let's Encrypt client, and there are a number of web hosts that use it for thousands of sites. Some of them use SQL database plugins to manage the hostnames, which are often retrieved dynamically via SNIcallback.

I do my own checks in Greenlock to ensure that I don't pass unsafe names downstream, but since I know that my library was vulnerable for a year or two, I imagine that other libraries out there exist which are vulnerable.

I see only a very narrow use case for allowing raw bytes (i.e. SNI knocking, which is far from pragmatic).

I see it as inconsistent with the rest of node's HTTP behavior (i.e. lowercasing content header names), and more risky than it's worth.

bnoordhuis commented 5 years ago

@solderjs Since you mentioned further up that you'd be willing to work on a patch, I'd say a PR is welcome. There's probably also the http2 module to consider.

mcollina commented 2 years ago

I don't think we should close it

bnoordhuis commented 1 year ago

It's been almost 5 years with no movement so I'm going to close this. PR still welcome in case anyone wants to work on it.