fixup! tls: add certificateCompression option

Address various review comments:
* Pack compression args into a uint32 instead of
  transferring as a full string[].
* Reject compression settings if TLS 1.3 is excluded.
* Use a constant for 'compress with all algorithms' arg
* Use RAII for compressed cert data
* Add tests to verify we reject large certs and that *record*
  compression is not enabled.

Signed-off-by: Tim Perry <pimterry@gmail.com>

Author: pimterry

lib/internal/tls/secure-context.js

@@ -218,7 +218,33 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     }
 
     if (certificateCompression.length > 0) {
-      context.setCertificateCompression(certificateCompression);
+      // Pack length + algorithm IDs into a single Uint32 for a cheap
+      // JS->C++ crossing. Layout:
+      //   bits  0-7 : length (1..3)
+      //   bits  8-15: alg id at position 0
+      //   bits 16-23: alg id at position 1
+      //   bits 24-31: alg id at position 2
+      // IDs match OpenSSL's TLSEXT_comp_cert_zlib (1), _brotli (2), _zstd (3)
+      if (certificateCompression.length > 3) {
+        throw new ERR_INVALID_ARG_VALUE(
+          `${name}.certificateCompression`, certificateCompression,
+          'can specify at most 3 algorithms');
+      }
+      let packed = certificateCompression.length;
+      for (let i = 0; i < certificateCompression.length; i++) {
+        const algoName = certificateCompression[i];
+        let id;
+        if (algoName === 'zlib') id = 1;
+        else if (algoName === 'brotli') id = 2;
+        else if (algoName === 'zstd') id = 3;
+        else {
+          throw new ERR_INVALID_ARG_VALUE(
+            `${name}.certificateCompression[${i}]`, algoName,
+            "must be 'zlib', 'brotli', or 'zstd'");
+        }
+        packed |= id << (8 * (i + 1));
+      }
+      context.setCertificateCompression(packed);
     }
   }
 

src/crypto/crypto_context.cc

@@ -2149,54 +2149,49 @@ void SecureContext::SetCertificateCompression(
   Environment* env = sc->env();
 
   CHECK_GE(args.Length(), 1);
-  CHECK(args[0]->IsArray());
+  CHECK(args[0]->IsUint32());
+
+  // Cert compression requires TLS 1.3:
+  long max_proto =  // NOLINT(runtime/int)
+      SSL_CTX_get_max_proto_version(sc->ctx_.get());
+  if (max_proto != 0 && max_proto < TLS1_3_VERSION) {
+    return THROW_ERR_INVALID_ARG_VALUE(
+        env,
+        "certificateCompression requires a TLS protocol range that includes "
+        "TLSv1.3");
+  }
 
-  Local<Array> arr = args[0].As<Array>();
-  uint32_t len = arr->Length();
+  // JS packs (length | alg0<<8 | alg1<<16 | alg2<<24) into a single Uint32.
+  // IDs match TLSEXT_comp_cert_zlib (1), _brotli (2), _zstd (3).
+  uint32_t packed = args[0].As<v8::Uint32>()->Value();
+  size_t len = packed & 0xff;
 
   // TLSEXT_comp_cert_limit is the limit for a zero-terminated algs array,
   // total number of available algs is one fewer.
-  constexpr uint32_t kMaxCompAlgs = TLSEXT_comp_cert_limit - 1;
+  constexpr size_t kMaxCompAlgs = TLSEXT_comp_cert_limit - 1;
   if (len == 0 || len > kMaxCompAlgs) {
     return THROW_ERR_INVALID_ARG_VALUE(
         env,
         "certificateCompression must specify fewer than %d algorithms",
-        kMaxCompAlgs);
+        static_cast<int>(kMaxCompAlgs));
   }
 
 #ifndef OPENSSL_NO_COMP_ALG
   int algs[kMaxCompAlgs];
-  for (uint32_t i = 0; i < len; i++) {
-    Local<Value> val;
-    if (!arr->Get(env->context(), i).ToLocal(&val) || !val->IsString()) {
-      return THROW_ERR_INVALID_ARG_VALUE(
-          env, "certificateCompression entries must be strings");
-    }
-    Utf8Value name(env->isolate(), val);
-    if (name.ToStringView() == "zlib") {
-      algs[i] = TLSEXT_comp_cert_zlib;
-    } else if (name.ToStringView() == "brotli") {
-      algs[i] = TLSEXT_comp_cert_brotli;
-    } else if (name.ToStringView() == "zstd") {
-      algs[i] = TLSEXT_comp_cert_zstd;
-    } else {
-      return THROW_ERR_INVALID_ARG_VALUE(
-          env,
-          "certificateCompression algorithm must be 'zlib', 'brotli', or "
-          "'zstd'");
-    }
+  for (size_t i = 0; i < len; i++) {
+    algs[i] = (packed >> (8 * (i + 1))) & 0xff;
   }
   if (!SSL_CTX_set1_cert_comp_preference(
           sc->ctx_.get(), algs, static_cast<size_t>(len))) {
     return THROW_ERR_CRYPTO_OPERATION_FAILED(
         env, "Failed to set certificate compression preference");
   }
 
-  // Pre-compress the loaded certificate(s) for all supported algorithms, where
-  // 0 arg means 'compress with all algorithms in the preference list'.
+  // Pre-compress the loaded certificate(s) for all supported algorithms.
   // Returns 0 when no certificate is loaded (e.g. client-only context) or
   // when compression did not reduce size - both are non-fatal.
-  SSL_CTX_compress_certs(sc->ctx_.get(), 0);
+  constexpr int kCompressAllAlgs = 0;
+  SSL_CTX_compress_certs(sc->ctx_.get(), kCompressAllAlgs);
 
   // Store preferences for propagation during SNI context switches.
   memcpy(sc->cert_comp_prefs_, algs, sizeof(int) * len);
@@ -2206,17 +2201,17 @@ void SecureContext::SetCertificateCompression(
   // setSniContext uses SSL_use_certificate which doesn't carry comp_cert data,
   // so we extract it here and re-apply via SSL_set1_compressed_cert later.
   sc->compressed_certs_.clear();
-  for (uint32_t i = 0; i < len; i++) {
+  for (size_t i = 0; i < len; i++) {
     unsigned char* data = nullptr;
     size_t orig_len = 0;
     size_t comp_len =
         SSL_CTX_get1_compressed_cert(sc->ctx_.get(), algs[i], &data, &orig_len);
+    ncrypto::DataPointer comp(data, comp_len);
     if (comp_len > 0 && data != nullptr) {
       sc->compressed_certs_.push_back(
           {algs[i],
            std::vector<unsigned char>(data, data + comp_len),
            orig_len});
-      OPENSSL_free(data);
     }
   }
 #else

test/parallel/test-tls-certificate-compression.js

@@ -37,13 +37,13 @@ const fixtureCert = fixtures.readKey('agent1-cert.pem');
   // Invalid algorithm name should throw
   assert.throws(
     () => tls.createSecureContext({ certificateCompression: ['invalid'] }),
-    /certificateCompression algorithm must be/
+    /must be 'zlib', 'brotli', or 'zstd'/
   );
 
   // Non-string entries should throw
   assert.throws(
     () => tls.createSecureContext({ certificateCompression: [1] }),
-    /certificateCompression entries must be strings/
+    /must be 'zlib', 'brotli', or 'zstd'/
   );
 
   // Valid single algorithms should not throw
@@ -65,8 +65,95 @@ const fixtureCert = fixtures.readKey('agent1-cert.pem');
 
   // Default (no certificateCompression) still works
   tls.createSecureContext({ key: fixtureKey, cert: fixtureCert });
+
+  // Protocol range that excludes TLSv1.3 should throw - RFC 8879 is 1.3-only.
+  assert.throws(
+    () => tls.createSecureContext({
+      key: fixtureKey, cert: fixtureCert,
+      maxVersion: 'TLSv1.2',
+      certificateCompression: ['zlib'],
+    }),
+    /TLSv1\.3/,
+  );
 }
 
+// Test: certificate compression must not enable record-level compression.
+//
+// RFC 8879 certificate compression is unrelated to TLS record compression
+// (the CRIME attack vector). The latter is disabled by Node at startup via
+// sk_SSL_COMP_zero, and by modern OpenSSL defaults as well. To confirm, we
+// Verify a ClientHello sent by Node only advertises null record compression.
+(async () => {
+  const { promise: clientHelloReceived, resolve: onClientHello } =
+    Promise.withResolvers();
+  const tcpServer = net.createServer((socket) => {
+    socket.once('data', (chunk) => {
+      onClientHello(chunk);
+      socket.destroy();
+    });
+    socket.on('error', () => {});
+  });
+  tcpServer.listen(0);
+  await once(tcpServer, 'listening');
+
+  const probe = tls.connect({
+    port: tcpServer.address().port,
+    rejectUnauthorized: false,
+    certificateCompression: ['zlib', 'brotli', 'zstd'],
+  });
+  probe.on('error', () => {});
+
+  const clientHello = await clientHelloReceived;
+  tcpServer.close();
+  probe.destroy();
+
+  // ClientHello layout (RFC 8446 4.1.2 + 5.1 record layer):
+  //   record header (5) | handshake header (4) | legacy_version (2)
+  //   | random (32) | session_id <1..32> | cipher_suites <2..>
+  //   | compression_methods <1..> | extensions <2..>
+  assert.strictEqual(clientHello[0], 0x16); // Handshake record
+  assert.strictEqual(clientHello[5], 0x01); // ClientHello
+  let i = 5 + 4 + 2 + 32;
+  i += 1 + clientHello[i];                  // Skip legacy_session_id
+  i += 2 + clientHello.readUInt16BE(i);     // Skip cipher_suites
+  const methods = clientHello.subarray(i + 1, i + 1 + clientHello[i]);
+  // Must only advertise the null compression method.
+  assert.deepStrictEqual([...methods], [0x00]);
+})().then(common.mustCall());
+
+// Test: a CompressedCertificate whose uncompressed length exceeds OpenSSL's
+// default max_cert_list (100 KB) is rejected before decompression, protecting
+// against decompression bombs in malicious server's cert chains.
+(async () => {
+  // Cert just repeats our existing fixture many times
+  const bigChain = Buffer.concat(
+    Array(200).fill(Buffer.from(fixtureCert))
+  );
+
+  const server = tls.createServer({
+    key: fixtureKey, cert: bigChain,
+    minVersion: 'TLSv1.3',
+    certificateCompression: ['zlib'],
+  }, common.mustNotCall());
+
+  // The aborted handshake surfaces as a tlsClientError on the server too.
+  server.on('tlsClientError', () => {});
+  server.listen(0);
+  await once(server, 'listening');
+
+  // Client starts handshake, server sends enormous cert, client rejects:
+  const client = tls.connect({
+    port: server.address().port,
+    rejectUnauthorized: false,
+    minVersion: 'TLSv1.3',
+    certificateCompression: ['zlib'],
+  });
+  const [err] = await once(client, 'error');
+  assert.strictEqual(err.code, 'ERR_SSL_EXCESSIVE_MESSAGE_SIZE');
+
+  server.close();
+})().then(common.mustCall());
+
 // Test: TLS connection with certificate compression reduces handshake size.
 //
 // To see meaningful compression, we generate a certificate with many SANs to