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: #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

@@ -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])`

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()`

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');

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) {

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();
         }));
     }

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');
+  }));
 });

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();