From eccdfd04581d781e6dc4ee2d2e6105c99cce1d11 Mon Sep 17 00:00:00 2001
From: Tim Perry <pimterry@gmail.com>
Date: Wed, 19 Oct 2022 16:51:33 +0200
Subject: [PATCH] tls: add support for ALPN fallback when no ALPN protocol
 matches

---
 doc/api/errors.md                            | 11 ++++
 doc/api/tls.md                               |  8 +++
 lib/_tls_wrap.js                             |  7 +++
 lib/internal/errors.js                       |  3 +
 src/crypto/crypto_tls.cc                     | 23 ++++++--
 src/env_properties.h                         |  1 +
 test/parallel/test-tls-alpn-server-client.js | 60 ++++++++++++++++++++
 7 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index e1e1b92f654505..0a518d67a34e9a 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -2698,6 +2698,17 @@ This error represents a failed test. Additional information about the failure
 is available via the `cause` property. The `failureType` property specifies
 what the test was doing when the failure occurred.
 
+<a id="ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS"></a>
+
+### `ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS`
+
+This error is thrown when creating a `TLSServer` if the TLS options sets
+`allowALPNFallback` to `true` without providing an `ALPNProtocols` argument.
+
+When `ALPNProtocols` is not provided, ALPN is skipped entirely, so the fallback
+would not be functional. To enable ALPN for all protocols, using the fallback
+in all cases, set `ALPNProtocols` to an empty array instead.
+
 <a id="ERR_TLS_CERT_ALTNAME_FORMAT"></a>
 
 ### `ERR_TLS_CERT_ALTNAME_FORMAT`
diff --git a/doc/api/tls.md b/doc/api/tls.md
index a9b23fc97501fc..c98231d1b46baf 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -2012,6 +2012,9 @@ 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/45075
+    description: The `options` parameter can now include `allowALPNFallback`.
   - version: v19.0.0
     pr-url: https://github.com/nodejs/node/pull/44031
     description: If `ALPNProtocols` is set, incoming connections that send an
@@ -2042,6 +2045,11 @@ changes:
     e.g. `0x05hello0x05world`, where the first byte is the length of the next
     protocol name. Passing an array is usually much simpler, e.g.
     `['hello', 'world']`. (Protocols should be ordered by their priority.)
+  * `allowALPNFallback`: {boolean} If `true`, if the `ALPNProtocols` option was
+    set and the client uses ALPN, but requests only protocols that are not do
+    not match the server's `ALPNProtocols`, the server will fall back to sending
+    an ALPN response accepting the first protocol the client suggested, instead
+    of closing the connection immediately with a fatal alert.
   * `clientCertEngine` {string} Name of an OpenSSL engine which can provide the
     client certificate.
   * `enableTrace` {boolean} If `true`, [`tls.TLSSocket.enableTrace()`][] will be
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 11ff14d725b36b..374937cfbd0628 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -71,6 +71,7 @@ const {
   ERR_INVALID_ARG_VALUE,
   ERR_MULTIPLE_CALLBACK,
   ERR_SOCKET_CLOSED,
+  ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS,
   ERR_TLS_DH_PARAM_SIZE,
   ERR_TLS_HANDSHAKE_TIMEOUT,
   ERR_TLS_INVALID_CONTEXT,
@@ -716,6 +717,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
     ssl.onnewsession = onnewsession;
     ssl.lastHandshakeTime = 0;
     ssl.handshakes = 0;
+    ssl.allowALPNFallback = !!options.allowALPNFallback;
 
     if (this.server) {
       if (this.server.listenerCount('resumeSession') > 0 ||
@@ -1100,6 +1102,7 @@ function tlsConnectionListener(rawSocket) {
     rejectUnauthorized: this.rejectUnauthorized,
     handshakeTimeout: this[kHandshakeTimeout],
     ALPNProtocols: this.ALPNProtocols,
+    allowALPNFallback: this.allowALPNFallback,
     SNICallback: this[kSNICallback] || SNICallback,
     enableTrace: this[kEnableTrace],
     pauseOnConnect: this.pauseOnConnect,
@@ -1198,6 +1201,10 @@ function Server(options, listener) {
   this._contexts = [];
   this.requestCert = options.requestCert === true;
   this.rejectUnauthorized = options.rejectUnauthorized !== false;
+  this.allowALPNFallback = options.allowALPNFallback === true;
+  if (this.allowALPNFallback && !options.ALPNProtocols) {
+    throw new ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS();
+  }
 
   if (options.sessionTimeout)
     this.sessionTimeout = options.sessionTimeout;
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index b162f961041221..d30f236f91c3f2 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -1612,6 +1612,9 @@ E('ERR_TEST_FAILURE', function(error, failureType) {
   this.cause = error;
   return msg;
 }, Error);
+E('ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS',
+  'The allowALPNFallback TLS option cannot be set without ALPNProtocols',
+  TypeError);
 E('ERR_TLS_CERT_ALTNAME_FORMAT', 'Invalid subject alternative name string',
   SyntaxError);
 E('ERR_TLS_CERT_ALTNAME_INVALID', function(reason, host, cert) {
diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc
index 1135448b1e2941..b098a354d8951c 100644
--- a/src/crypto/crypto_tls.cc
+++ b/src/crypto/crypto_tls.cc
@@ -39,6 +39,7 @@ using v8::Array;
 using v8::ArrayBuffer;
 using v8::ArrayBufferView;
 using v8::BackingStore;
+using v8::Boolean;
 using v8::Context;
 using v8::DontDelete;
 using v8::Exception;
@@ -237,6 +238,13 @@ int SelectALPNCallback(
   if (UNLIKELY(alpn_buffer.IsEmpty()) || !alpn_buffer->IsArrayBufferView())
     return SSL_TLSEXT_ERR_NOACK;
 
+  bool alpn_fallback_allowed =
+      w->object()
+          ->Get(env->context(), env->allow_alpn_fallback_string())
+          .ToLocalChecked()
+          .As<Boolean>()
+          ->Value();
+
   ArrayBufferViewContents<unsigned char> alpn_protos(alpn_buffer);
   int status = SSL_select_next_proto(
       const_cast<unsigned char**>(out),
@@ -249,10 +257,17 @@ int SelectALPNCallback(
   // 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;
+
+  // By default, 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, unless the allowALPNFallback option has been set, which enables
+  // OpenSSL's default protocol selection behavior, falling back to the client's
+  // first preference.
+  if (status == OPENSSL_NPN_NEGOTIATED || alpn_fallback_allowed) {
+    return SSL_TLSEXT_ERR_OK;
+  } else {
+    return SSL_TLSEXT_ERR_ALERT_FATAL;
+  }
 }
 
 int TLSExtStatusCallback(SSL* s, void* arg) {
diff --git a/src/env_properties.h b/src/env_properties.h
index 76c52f1ea57385..ae1bf5374d0f35 100644
--- a/src/env_properties.h
+++ b/src/env_properties.h
@@ -50,6 +50,7 @@
   V(ack_string, "ack")                                                         \
   V(address_string, "address")                                                 \
   V(aliases_string, "aliases")                                                 \
+  V(allow_alpn_fallback_string, "allowALPNFallback")                           \
   V(args_string, "args")                                                       \
   V(asn1curve_string, "asn1Curve")                                             \
   V(async_ids_stack_string, "async_ids_stack")                                 \
diff --git a/test/parallel/test-tls-alpn-server-client.js b/test/parallel/test-tls-alpn-server-client.js
index 2961c0ed1d4af4..66a6c411076dc4 100644
--- a/test/parallel/test-tls-alpn-server-client.js
+++ b/test/parallel/test-tls-alpn-server-client.js
@@ -206,6 +206,66 @@ function TestFatalAlert() {
       }
     }));
   }));
+
+  TestALPNFallback();
+}
+
+function TestALPNFallback() {
+  const serverOptions = {
+    ALPNProtocols: ['b'],
+    allowALPNFallback: true
+  };
+
+  const clientOptions = [{
+    ALPNProtocols: ['a', 'b']
+  }, {
+    ALPNProtocols: ['a']
+  }];
+
+  runTest(clientOptions, serverOptions, function(results) {
+    // 'b' is selected by ALPN if available
+    checkResults(results[0],
+                 { server: { ALPN: 'b' },
+                   client: { ALPN: 'b' } });
+
+    // 'a' is selected by ALPN if not, due to fallback
+    checkResults(results[1],
+                 { server: { ALPN: 'a' },
+                   client: { ALPN: 'a' } });
+  });
+
+  TestEmptyALPNFallback();
+}
+
+function TestEmptyALPNFallback() {
+  const serverOptions = {
+    ALPNProtocols: [],
+    allowALPNFallback: true
+  };
+
+  const clientOptions = [{
+    ALPNProtocols: ['a', 'b']
+  }];
+
+  runTest(clientOptions, serverOptions, function(results) {
+    // 'a' is selected by ALPN, due to fallback with empty array
+    checkResults(results[0],
+                 { server: { ALPN: 'a' },
+                   client: { ALPN: 'a' } });
+  });
+
+  TestFallbackWithoutProtocols();
+}
+
+function TestFallbackWithoutProtocols() {
+  assert.throws(() => {
+    tls.createServer({
+      allowALPNFallback: true
+    });
+  }, {
+    code: 'ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS',
+    message: 'The allowALPNFallback TLS option cannot be set without ALPNProtocols'
+  });
 }
 
 Test1();