From 77def91bf9a2abd6a19514b57e3dc08f3e3f0fc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Sat, 13 Aug 2022 09:25:23 +0200
Subject: [PATCH] tls,http2: send fatal alert on ALPN mismatch

To comply with RFC 7301, make TLS servers send a fatal alert during the
TLS handshake if both the client and the server are configured to use
ALPN and if the server does not support any of the protocols advertised
by the client.

This affects HTTP/2 servers. Until now, applications could intercept the
'unknownProtocol' event when the client either did not advertise any
protocols or if the list of protocols advertised by the client did not
include HTTP/2 (or HTTP/1.1 if allowHTTP1 was true). With this change,
only the first case can be handled, and the 'unknownProtocol' event will
not be emitted in the second case because the TLS handshake fails and no
secure connection is established.

PR-URL: https://github.com/nodejs/node/pull/44031
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 doc/api/http2.md                              | 14 +++++
 doc/api/tls.md                                |  9 +++-
 lib/internal/http2/core.js                    |  2 +-
 src/crypto/crypto_tls.cc                      | 14 ++---
 test/parallel/test-http2-https-fallback.js    | 16 ++++--
 .../test-http2-server-unknown-protocol.js     | 20 ++++++--
 test/parallel/test-tls-alpn-server-client.js  | 51 +++++++++++++++++--
 7 files changed, 107 insertions(+), 19 deletions(-)

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 28057a58a45724..1d87b5127d9839 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -2275,6 +2275,11 @@ a given number of milliseconds set using `http2secureServer.setTimeout()`.
 
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/44031
+    description: This event will only be emitted if the client did not transmit
+                 an ALPN extension during the TLS handshake.
 -->
 
 * `socket` {stream.Duplex}
@@ -2284,6 +2289,15 @@ 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. A timeout may be specified using the
 `'unknownProtocolTimeout'` option passed to [`http2.createSecureServer()`][].
+
+In earlier versions of Node.js, this event would be emitted if `allowHTTP1` is
+`false` and, during the TLS handshake, the client either does not send an ALPN
+extension or sends an ALPN extension that does not include HTTP/2 (`h2`). Newer
+versions of Node.js only emit this event if `allowHTTP1` is `false` and the
+client does not send an ALPN extension. If the client sends an ALPN extension
+that does not include HTTP/2 (or HTTP/1.1 if `allowHTTP1` is `true`), the TLS
+handshake will fail and no secure connection will be established.
+
 See the [Compatibility API][].
 
 #### `server.close([callback])`
diff --git a/doc/api/tls.md b/doc/api/tls.md
index 1d4db99a11bf22..3477a90faa6e37 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -683,8 +683,8 @@ is set to describe how authorization failed. Depending on the settings
 of the TLS server, unauthorized connections may still be accepted.
 
 The `tlsSocket.alpnProtocol` property is a string that contains the selected
-ALPN protocol. When ALPN has no selected protocol, `tlsSocket.alpnProtocol`
-equals `false`.
+ALPN protocol. When ALPN has no selected protocol because the client or the
+server did not send an ALPN extension, `tlsSocket.alpnProtocol` equals `false`.
 
 The `tlsSocket.servername` property is a string containing the server name
 requested via SNI.
@@ -2012,6 +2012,11 @@ where `secureSocket` has the same API as `pair.cleartext`.
 <!-- YAML
 added: v0.3.2
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/44031
+    description: If `ALPNProtocols` is set, incoming connections that send an
+                 ALPN extension with no supported protocols are terminated with
+                 a fatal `no_application_protocol` alert.
   - version: v12.3.0
     pr-url: https://github.com/nodejs/node/pull/27665
     description: The `options` parameter now supports `net.createServer()`
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index c6915e903e9d01..456ea27cfebf79 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -3055,7 +3055,7 @@ function connectionListener(socket) {
       // going on in a format that they *might* understand.
       socket.end('HTTP/1.0 403 Forbidden\r\n' +
                  'Content-Type: text/plain\r\n\r\n' +
-                 'Unknown ALPN Protocol, expected `h2` to be available.\n' +
+                 'Missing ALPN Protocol, expected `h2` to be available.\n' +
                  'If this is a HTTP request: The server was not ' +
                  'configured with the `allowHTTP1` option or a ' +
                  'listener for the `unknownProtocol` event.\n');
diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc
index 849b21f28dfda8..1135448b1e2941 100644
--- a/src/crypto/crypto_tls.cc
+++ b/src/crypto/crypto_tls.cc
@@ -246,13 +246,13 @@ int SelectALPNCallback(
       in,
       inlen);
 
-  // According to 3.2. Protocol Selection of RFC7301, fatal
-  // no_application_protocol alert shall be sent but OpenSSL 1.0.2 does not
-  // support it yet. See
-  // https://rt.openssl.org/Ticket/Display.html?id=3463&user=guest&pass=guest
-  return status == OPENSSL_NPN_NEGOTIATED
-      ? SSL_TLSEXT_ERR_OK
-      : SSL_TLSEXT_ERR_NOACK;
+  // Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol
+  // match was found. This would neither cause a fatal alert nor would it result
+  // in a useful ALPN response as part of the Server Hello message.
+  // We now return SSL_TLSEXT_ERR_ALERT_FATAL in that case as per Section 3.2
+  // of RFC 7301, which causes a fatal no_application_protocol alert.
+  return status == OPENSSL_NPN_NEGOTIATED ? SSL_TLSEXT_ERR_OK
+                                          : SSL_TLSEXT_ERR_ALERT_FATAL;
 }
 
 int TLSExtStatusCallback(SSL* s, void* arg) {
diff --git a/test/parallel/test-http2-https-fallback.js b/test/parallel/test-http2-https-fallback.js
index 16879b05f12c8d..24283b775d76d5 100644
--- a/test/parallel/test-http2-https-fallback.js
+++ b/test/parallel/test-http2-https-fallback.js
@@ -126,7 +126,7 @@ function onSession(session, next) {
     const { port } = server.address();
     const origin = `https://localhost:${port}`;
 
-    const cleanup = countdown(3, () => server.close());
+    const cleanup = countdown(4, () => server.close());
 
     // HTTP/2 client
     connect(
@@ -149,12 +149,22 @@ function onSession(session, next) {
 
     function testWrongALPN() {
       // Incompatible ALPN TLS client
-      let text = '';
       tls(Object.assign({ port, ALPNProtocols: ['fake'] }, clientOptions))
+        .on('error', common.mustCall((err) => {
+          strictEqual(err.code, 'ECONNRESET');
+          cleanup();
+          testNoALPN();
+        }));
+    }
+
+    function testNoALPN() {
+      // TLS client does not send an ALPN extension
+      let text = '';
+      tls(Object.assign({ port }, clientOptions))
         .setEncoding('utf8')
         .on('data', (chunk) => text += chunk)
         .on('end', common.mustCall(() => {
-          ok(/Unknown ALPN Protocol, expected `h2` to be available/.test(text));
+          ok(/Missing ALPN Protocol, expected `h2` to be available/.test(text));
           cleanup();
         }));
     }
diff --git a/test/parallel/test-http2-server-unknown-protocol.js b/test/parallel/test-http2-server-unknown-protocol.js
index 639bbe4f2d6ab4..b00ccc1b338e47 100644
--- a/test/parallel/test-http2-server-unknown-protocol.js
+++ b/test/parallel/test-http2-server-unknown-protocol.js
@@ -8,6 +8,7 @@ const fixtures = require('../common/fixtures');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 
+const assert = require('assert');
 const h2 = require('http2');
 const tls = require('tls');
 
@@ -18,16 +19,29 @@ const server = h2.createSecureServer({
   allowHalfOpen: true
 });
 
-server.on('connection', (socket) => {
+server.on('secureConnection', common.mustCall((socket) => {
   socket.on('close', common.mustCall(() => {
     server.close();
   }));
-});
+}));
 
 server.listen(0, function() {
+  // If the client does not send an ALPN connection, and the server has not been
+  // configured with allowHTTP1, then the server should destroy the socket
+  // after unknownProtocolTimeout.
   tls.connect({
     port: server.address().port,
     rejectUnauthorized: false,
-    ALPNProtocols: ['bogus']
   });
+
+  // If the client sends an ALPN extension that does not contain 'h2', the
+  // server should send a fatal alert to the client before a secure connection
+  // is established at all.
+  tls.connect({
+    port: server.address().port,
+    rejectUnauthorized: false,
+    ALPNProtocols: ['bogus']
+  }).on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ECONNRESET');
+  }));
 });
diff --git a/test/parallel/test-tls-alpn-server-client.js b/test/parallel/test-tls-alpn-server-client.js
index 3da24c67fbe522..2961c0ed1d4af4 100644
--- a/test/parallel/test-tls-alpn-server-client.js
+++ b/test/parallel/test-tls-alpn-server-client.js
@@ -5,6 +5,7 @@ if (!common.hasCrypto)
   common.skip('missing crypto');
 
 const assert = require('assert');
+const { spawn } = require('child_process');
 const tls = require('tls');
 const fixtures = require('../common/fixtures');
 
@@ -68,7 +69,7 @@ function Test1() {
   }, {
     ALPNProtocols: ['c', 'b', 'e'],
   }, {
-    ALPNProtocols: ['first-priority-unsupported', 'x', 'y'],
+    ALPNProtocols: ['x', 'y', 'c'],
   }];
 
   runTest(clientsOptions, serverOptions, function(results) {
@@ -82,8 +83,8 @@ function Test1() {
                    client: { ALPN: 'b' } });
     // Nothing is selected by ALPN
     checkResults(results[2],
-                 { server: { ALPN: false },
-                   client: { ALPN: false } });
+                 { server: { ALPN: 'c' },
+                   client: { ALPN: 'c' } });
     // execute next test
     Test2();
   });
@@ -161,6 +162,50 @@ function Test4() {
                  { server: { ALPN: false },
                    client: { ALPN: false } });
   });
+
+  TestFatalAlert();
+}
+
+function TestFatalAlert() {
+  const server = tls.createServer({
+    ALPNProtocols: ['foo'],
+    key: loadPEM('agent2-key'),
+    cert: loadPEM('agent2-cert')
+  }, common.mustNotCall());
+
+  server.listen(0, serverIP, common.mustCall(() => {
+    const { port } = server.address();
+
+    // The Node.js client will just report ECONNRESET because the connection
+    // is severed before the TLS handshake completes.
+    tls.connect({
+      host: serverIP,
+      port,
+      rejectUnauthorized: false,
+      ALPNProtocols: ['bar']
+    }, common.mustNotCall()).on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ECONNRESET');
+
+      // OpenSSL's s_client should output the TLS alert number, which is 120
+      // for the 'no_application_protocol' alert.
+      const { opensslCli } = common;
+      if (opensslCli) {
+        const addr = `${serverIP}:${port}`;
+        let stderr = '';
+        spawn(opensslCli, ['s_client', '--alpn', 'bar', addr], {
+          stdio: ['ignore', 'ignore', 'pipe']
+        }).stderr
+          .setEncoding('utf8')
+          .on('data', (chunk) => stderr += chunk)
+          .on('close', common.mustCall(() => {
+            assert.match(stderr, /SSL alert number 120/);
+            server.close();
+          }));
+      } else {
+        server.close();
+      }
+    }));
+  }));
 }
 
 Test1();