http2: cannot negotiate ALPN besides http/1.1

[bnoordhuis]

The documentation for 'unknownProtocol' says this:

The 'unknownProtocol' event is emitted when a connecting client fails to negotiate an allowed protocol (i.e. HTTP/2 or HTTP/1.1). The event handler receives the socket for handling. If no listener is registered for this event, the connection is terminated.

The logic seems wrong though. It only passes through nothing (no protocol negotiated) or http/1.1, everything else is ignored:

https://github.com/nodejs/node/blob/11f8024d992c385d3db196ab64678311bfdabd84/lib/internal/http2/core.js#L2614-L2634

Caveat: if the check is loosened, care should be taken not to introduce an information leak.

For an attacker it should not be possible to deduce whether the server has { allowHTTP1: true } and an 'unknownProtocol' listener installed by sending messages with the ALPN proto set to http/1.1 and e.g. hax/13.37, and then comparing the responses he gets back.

[targos]

@nodejs/http2

[BlackYoup]

Hello, I'm interested in fixing this issue if possible but I'd like to be sure of what is expected:

If the client sends a random ALPN like hax/13.37, socket.alpnProtocol will be false and the server shouldn't "downgrade" to an http/1.1 connection if the allowHTTP1 option is true, right? Instead, it should directly try to emit the unknownProtocol or close the connection if there is no such event.

For the information disclosure part, I'm not sure how one would avoid it but maybe I'm missing something.

[bnoordhuis]

@BlackYoup My reading of the code is this:

1. Sending no ALPN protocol or http/1.1 takes the the fallback path - either a downgrade to HTTP/1.1 when {allowHTTP1: true}, or connection termination with a 403 response

2. Sending anything else (h2, hax/13.37, etc.) takes the HTTP/2 path

At the very least, 2. should be more discriminating: allow h2, reject everything else. Untested, but I think it would look something like this:

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index a044039960..8be9ca345d 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -2830,9 +2830,9 @@ function connectionListener(socket) {
   debug('Http2Session server: received a connection');
   const options = this[kOptions] || {};

-  if (socket.alpnProtocol === false || socket.alpnProtocol === 'http/1.1') {
+  if (socket.alpnProtocol !== 'h2') {
     // Fallback to HTTP/1.1
-    if (options.allowHTTP1 === true) {
+    if (options.allowHTTP1 === true && socket.alpnProtocol === 'http/1.1') {
       socket.server[kIncomingMessage] = options.Http1IncomingMessage;
       socket.server[kServerResponse] = options.Http1ServerResponse;
       return httpConnectionListener.call(this, socket);

For the information disclosure part, I'm not sure how one would avoid it but maybe I'm missing something.

The current error message already gives it away and it looks like @addaleax introduced it intentionally in #18986 so I guess there's not much that can be done there. 🤷

[BlackYoup]

Untested, but I think it would look something like this:

Yes, that's what I had in mind too

The current error message already gives it away and it looks like @addaleax introduced it intentionally in #18986 so I guess there's not much that can be done there. shrug

Alright, then I'll just keep it that way.

I'll submit a PR tonight, thanks for the insights :)

[BlackYoup]

After reading the tests, it seems that it is expected behavior to try to downgrade to HTTP1 if the allowHTTP1 option is enabled. I guess that's because some clients might not send an ALPN in the TLS connection because they might be too old.

The test that does not set an ALPN does an HTTP1 request is https://github.com/nodejs/node/blob/5835367df4961bb2d71b0700b430b11f9ad32022/test/parallel/test-http2-https-fallback-http-server-options.js

So maybe that's more a documentation issue around the behavior of allowHTTP1?

[jasnell]

Yeah, this is a feature that never really got the love to finish it all the way through. The check needs to be strict, allowing only h1 or h2 to be specified. We can explicitly add support for additional ALPN values later as appropriate.

[BlackYoup]

@jasnell alright but what about clients that do not set an ALPN and expect the h1 server to respond when the allowHTTP1 option is set? If I understand you correctly and if this check becomes strict, then those clients will get a 403.

[jasnell]

Yeah, I was speaking only in terms of if the ALPN is specified. If it is not, then we have to apply some additional heuristic to determine if it is a valid HTTP/1 request. If allowHTTP1 is specified, and the request is not a valid HTTP/2 initial handshake, then falling back and attempting to process the request as HTTP/1 makes sense.

[BlackYoup]

Then it seems to me that's exactly the behavior we have with the current code. I guess we could add the h2 check somewhere but I don't see how it would make it more strict since the only possible values of socket.alpnProtocol are either false, h2 or http/1.1 from what I understand of the code.

Unless I'm missing something obvious?