diff --git a/src/boringssl.zig b/src/boringssl.zig index 78a83c6b1fc..4b62d22ab60 100644 --- a/src/boringssl.zig +++ b/src/boringssl.zig @@ -116,10 +116,48 @@ pub fn ip2String(ip: *boring.ASN1_OCTET_STRING, outIP: *[INET6_ADDRSTRLEN + 1]u8 return outIP[0..size]; } +/// Matches a DNS name pattern (possibly with a leading `*.` wildcard) against +/// `hostname`. Mirrors Node.js `check()` in lib/tls.js for a single pattern. +fn matchDnsName(pattern: []const u8, hostname: []const u8) bool { + if (pattern.len == 0) return false; + if (!X509.isSafeAltName(pattern, false)) return false; + + if (pattern[0] == '*') { + // RFC 6125 Section 6.4.3: Wildcard must match exactly one label. + // Enforce "*." prefix (wildcard must be leftmost and followed by a dot). + if (pattern.len >= 2 and pattern[1] == '.') { + const suffix = pattern[2..]; + // Disallow "*.tld" (suffix must contain at least one dot for proper domain hierarchy) + if (strings.containsChar(suffix, '.')) { + // Host must be at least "label.suffix" (suffix_len + 1 for dot + at least 1 char for label) + if (hostname.len > suffix.len + 1) { + const dot_index = hostname.len - suffix.len - 1; + // The character before suffix must be a dot, and there must be no other + // dots in the prefix (single-label wildcard only). + if (hostname[dot_index] == '.' and !strings.containsChar(hostname[0..dot_index], '.')) { + const host_suffix = hostname[dot_index + 1 ..]; + // RFC 4343: DNS names are case-insensitive + if (strings.eqlCaseInsensitiveASCII(suffix, host_suffix, true)) { + return true; + } + } + } + } + } + } + // RFC 4343: DNS names are case-insensitive + return strings.eqlCaseInsensitiveASCII(pattern, hostname, true); +} + pub fn checkX509ServerIdentity( x509: *boring.X509, hostname: []const u8, ) bool { + const host_is_ip = strings.isIPAddress(hostname); + // Node.js: CN is consulted only when the certificate carries no + // DNS / IP / URI subjectAltName entries. Track whether any were seen. + var has_identifier_san = false; + // we check with native code if the cert is valid const index = boring.X509_get_ext_by_NID(x509, boring.NID_subject_alt_name, -1); if (index >= 0) { @@ -130,7 +168,7 @@ pub fn checkX509ServerIdentity( return false; } - if (strings.isIPAddress(hostname)) { + if (host_is_ip) { // we safely ensure buffer size with max len + 1 var canonicalIPBuf: [INET6_ADDRSTRLEN + 1]u8 = undefined; var certIPBuf: [INET6_ADDRSTRLEN + 1]u8 = undefined; @@ -143,12 +181,17 @@ pub fn checkX509ServerIdentity( for (0..boring.sk_GENERAL_NAME_num(names)) |i| { const gen = boring.sk_GENERAL_NAME_value(names, i); if (gen) |name| { - if (name.name_type == .GEN_IPADD) { - if (ip2String(name.d.ip, &certIPBuf)) |cert_ip| { - if (strings.eql(host_ip, cert_ip)) { - return true; + switch (name.name_type) { + .GEN_DNS, .GEN_URI => has_identifier_san = true, + .GEN_IPADD => { + has_identifier_san = true; + if (ip2String(name.d.ip, &certIPBuf)) |cert_ip| { + if (strings.eql(host_ip, cert_ip)) { + return true; + } } - } + }, + else => {}, } } } @@ -160,41 +203,17 @@ pub fn checkX509ServerIdentity( for (0..boring.sk_GENERAL_NAME_num(names)) |i| { const gen = boring.sk_GENERAL_NAME_value(names, i); if (gen) |name| { - if (name.name_type == .GEN_DNS) { - const dnsName = name.d.dNSName; - var dnsNameSlice = dnsName.data[0..@as(usize, @intCast(dnsName.length))]; - // ignore empty dns names (should never happen) - if (dnsNameSlice.len > 0) { - if (X509.isSafeAltName(dnsNameSlice, false)) { - if (dnsNameSlice[0] == '*') { - // RFC 6125 Section 6.4.3: Wildcard must match exactly one label - // Enforce "*." prefix (wildcards must be leftmost and followed by a dot) - if (dnsNameSlice.len >= 2 and dnsNameSlice[1] == '.') { - const suffix = dnsNameSlice[2..]; - // Disallow "*.tld" (suffix must contain at least one dot for proper domain hierarchy) - if (std.mem.indexOfScalar(u8, suffix, '.') != null) { - // Host must be at least "label.suffix" (suffix_len + 1 for dot + at least 1 char for label) - if (hostname.len > suffix.len + 1) { - const dot_index = hostname.len - suffix.len - 1; - // The character before suffix must be a dot, and there must be no other dots - // in the prefix (single-label wildcard only) - if (hostname[dot_index] == '.' and std.mem.indexOfScalar(u8, hostname[0..dot_index], '.') == null) { - const host_suffix = hostname[dot_index + 1 ..]; - // RFC 4343: DNS names are case-insensitive - if (strings.eqlCaseInsensitiveASCII(suffix, host_suffix, true)) { - return true; - } - } - } - } - } - } - // RFC 4343: DNS names are case-insensitive - if (strings.eqlCaseInsensitiveASCII(dnsNameSlice, hostname, true)) { - return true; - } + switch (name.name_type) { + .GEN_IPADD, .GEN_URI => has_identifier_san = true, + .GEN_DNS => { + has_identifier_san = true; + const dnsName = name.d.dNSName; + const dnsNameSlice = dnsName.data[0..@as(usize, @intCast(dnsName.length))]; + if (matchDnsName(dnsNameSlice, hostname)) { + return true; } - } + }, + else => {}, } } } @@ -202,6 +221,30 @@ pub fn checkX509ServerIdentity( } } } + + // Node.js tls.checkServerIdentity: when the certificate has no + // DNS/IP/URI subjectAltName entries, fall back to the Subject + // Common Name. Never for IP-literal hosts (RFC 2818 §3.1). + if (!host_is_ip and !has_identifier_san) { + if (boring.X509_get_subject_name(x509)) |subject| { + var last: c_int = -1; + while (true) { + const entry_idx = boring.X509_NAME_get_index_by_NID(subject, boring.NID_commonName, last); + if (entry_idx < 0) break; + last = entry_idx; + const entry = boring.X509_NAME_get_entry(subject, entry_idx) orelse continue; + const data = boring.X509_NAME_ENTRY_get_data(entry) orelse continue; + const cn_ptr = boring.ASN1_STRING_get0_data(data); + const cn_len = boring.ASN1_STRING_length(data); + if (cn_ptr == null or cn_len <= 0) continue; + const cn = cn_ptr[0..@intCast(cn_len)]; + if (matchDnsName(cn, hostname)) { + return true; + } + } + } + } + return false; } diff --git a/src/http/HTTPContext.zig b/src/http/HTTPContext.zig index 0b158f32b8b..20956ae5c99 100644 --- a/src/http/HTTPContext.zig +++ b/src/http/HTTPContext.zig @@ -417,9 +417,12 @@ pub fn NewHTTPContext(comptime ssl: bool) type { // if checkServerIdentity returns false, we dont call firstCall — the connection was rejected const ssl_ptr = @as(*BoringSSL.SSL, @ptrCast(socket.getNativeHandle())); if (!client.checkServerIdentity(comptime ssl, socket, handshake_error, ssl_ptr, true)) { - client.flags.did_have_handshaking_error = true; - client.unregisterAbortTracker(); - if (!socket.isClosed()) terminateSocket(socket); + // checkServerIdentity already called closeAndFail() → fail() + // → result callback, which may have destroyed the + // AsyncHTTP that embeds `client`. Socket is terminated + // and the abort tracker is unregistered there, so the + // only safe action is to return without touching + // `client` again. return; } } diff --git a/src/http/ProxyTunnel.zig b/src/http/ProxyTunnel.zig index 33755a85e8d..c4b321e5373 100644 --- a/src/http/ProxyTunnel.zig +++ b/src/http/ProxyTunnel.zig @@ -164,17 +164,17 @@ fn onHandshake(this: *HTTPClient, handshake_success: bool, ssl_error: uws.us_bun .ssl => |socket| { if (!this.checkServerIdentity(true, socket, handshake_error, ssl_ptr, false)) { log("ProxyTunnel onHandshake checkServerIdentity failed", .{}); - this.flags.did_have_handshaking_error = true; - - this.unregisterAbortTracker(); + // checkServerIdentity already called closeAndFail() + // → fail() → result callback, which may have + // destroyed the AsyncHTTP that embeds `this`. Do not + // touch `this` after a `false` return. return; } }, .tcp => |socket| { if (!this.checkServerIdentity(false, socket, handshake_error, ssl_ptr, false)) { log("ProxyTunnel onHandshake checkServerIdentity failed", .{}); - this.flags.did_have_handshaking_error = true; - this.unregisterAbortTracker(); + // see .ssl arm — `this` may be freed here. return; } }, diff --git a/src/js/node/_http_client.ts b/src/js/node/_http_client.ts index f97851b3b79..07e6c48bb4c 100644 --- a/src/js/node/_http_client.ts +++ b/src/js/node/_http_client.ts @@ -779,6 +779,10 @@ function ClientRequest(input, options, cb) { validateInteger(mergedTlsOptions.secureOptions, "options.secureOptions"); this._ensureTls().secureOptions = mergedTlsOptions.secureOptions; } + if (mergedTlsOptions.checkServerIdentity !== undefined) { + validateFunction(mergedTlsOptions.checkServerIdentity, "options.checkServerIdentity"); + this._ensureTls().checkServerIdentity = mergedTlsOptions.checkServerIdentity; + } this[kPath] = options.path || "/"; if (cb) { this.once("response", cb); diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index ae65effefa4..462b6019f34 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -197,6 +197,19 @@ function emitListeningNextTick(self, hostname, port) { } } +// Node.js only requests a client certificate when `requestCert: true`. +// The uSockets SSL context treats `ca` alone as "verify peer", so without +// these two flags an `https.Server({ ca })` would reject every client that +// doesn't present a cert. Mirror tls.Server (net.ts): default `requestCert` +// to false and, when not requesting, force `rejectUnauthorized` to false so +// the CA is loaded into the trust store without requiring a client cert. +function normalizeServerTls(tls) { + const requestCert = !!tls.requestCert; + tls.requestCert = requestCert; + tls.rejectUnauthorized = requestCert ? tls.rejectUnauthorized !== false : false; + return tls; +} + function Server(options, callback): void { if (!(this instanceof Server)) return new Server(options, callback); EventEmitter.$call(this); @@ -251,14 +264,16 @@ function Server(options, callback): void { } if (this[isTlsSymbol]) { - this[tlsSymbol] = { + this[tlsSymbol] = normalizeServerTls({ serverName, key, cert, ca, passphrase, secureOptions, - }; + requestCert: options.requestCert, + rejectUnauthorized: options.rejectUnauthorized, + }); } else { this[tlsSymbol] = null; } @@ -385,7 +400,7 @@ Server.prototype.listen = function () { const otherTLS = arguments[0].tls; if (otherTLS && $isObject(otherTLS)) { - tls = otherTLS; + tls = normalizeServerTls({ ...otherTLS }); } } else if (typeof arguments[0] === "string" && !(Number(arguments[0]) >= 0)) { // (path[...][, cb]) diff --git a/test/js/node/http/node-https-checkServerIdentity.test.ts b/test/js/node/http/node-https-checkServerIdentity.test.ts new file mode 100644 index 00000000000..bce69df8fb3 --- /dev/null +++ b/test/js/node/http/node-https-checkServerIdentity.test.ts @@ -0,0 +1,213 @@ +// Regression tests for node-compat group u9sf0l. +// +// The core bug was an ASAN use-after-poison in +// src/http/HTTPContext.zig onHandshake: when the native +// checkServerIdentity() rejected the peer certificate, it called +// closeAndFail() → fail() → result callback, which destroyed the +// AsyncHTTP (and its embedded HTTPClient). onHandshake then wrote to +// client.flags.did_have_handshaking_error on freed memory. +// +// Triggering the bug requires `rejectUnauthorized: true`, a trusted +// CA, and a hostname that does NOT match the certificate's identity. +// Previously any CN-only cert (no SAN) would hit this, because the +// native checkX509ServerIdentity never fell back to the Subject CN. + +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +const keys = join(import.meta.dir, "..", "test", "fixtures", "keys"); +const agent1 = { + key: readFileSync(join(keys, "agent1-key.pem"), "utf8"), // CN=agent1, no SAN, signed by ca1 + cert: readFileSync(join(keys, "agent1-cert.pem"), "utf8"), + ca: readFileSync(join(keys, "ca1-cert.pem"), "utf8"), +}; + +describe("https.request checkServerIdentity", () => { + // Direct repro of the ASAN crash: trusted CA + servername that does not + // match the cert. Before the fix this hit use-after-poison in + // HTTPContext.onHandshake on ASAN builds instead of emitting 'error'. + test("hostname mismatch emits error without crashing", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const https = require("https"); + const server = https.createServer({ + key: ${JSON.stringify(agent1.key)}, + cert: ${JSON.stringify(agent1.cert)}, + }, (req, res) => { res.writeHead(200); res.end("ok"); }); + server.listen(0, () => { + const req = https.request({ + port: server.address().port, + rejectUnauthorized: true, + ca: ${JSON.stringify(agent1.ca)}, + servername: "not-agent1", + }, () => { + console.log("UNEXPECTED_RESPONSE"); + server.close(); + }); + req.on("error", err => { + console.log("ERROR_CODE=" + err.code); + server.close(); + }); + req.end(); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ERROR_CODE=ERR_TLS_CERT_ALTNAME_INVALID"); + expect(exitCode).toBe(0); + }); + + // Node's tls.checkServerIdentity falls back to the Subject CN when the + // certificate carries no DNS/IP/URI SANs. agent1's cert is CN=agent1 with + // no SAN. With `servername: "agent1"` the request must succeed. + test("falls back to Subject CN when no SAN is present", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const https = require("https"); + const server = https.createServer({ + key: ${JSON.stringify(agent1.key)}, + cert: ${JSON.stringify(agent1.cert)}, + }, (req, res) => { res.writeHead(200); res.end("ok"); }); + server.listen(0, () => { + const req = https.request({ + port: server.address().port, + rejectUnauthorized: true, + ca: ${JSON.stringify(agent1.ca)}, + servername: "agent1", + }, res => { + let body = ""; + res.on("data", d => body += d); + res.on("end", () => { + console.log("BODY=" + body); + server.close(); + }); + }); + req.on("error", err => { + console.log("UNEXPECTED_ERROR=" + (err.code || err.message)); + server.close(); + }); + req.end(); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("BODY=ok"); + expect(exitCode).toBe(0); + }); + + // A user-supplied `checkServerIdentity` must override the native check. + // agent1's CN is "agent1" so the native check for hostname "localhost" + // would fail; the custom callback makes the request succeed and must + // actually be invoked. + test("custom checkServerIdentity overrides the native check", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const https = require("https"); + const server = https.createServer({ + key: ${JSON.stringify(agent1.key)}, + cert: ${JSON.stringify(agent1.cert)}, + }, (req, res) => { res.writeHead(200); res.end("ok"); }); + server.listen(0, () => { + let called = false; + const req = https.request({ + port: server.address().port, + rejectUnauthorized: true, + ca: ${JSON.stringify(agent1.ca)}, + checkServerIdentity: (host, cert) => { + called = true; + return undefined; + }, + }, res => { + let body = ""; + res.on("data", d => body += d); + res.on("end", () => { + console.log("CALLED=" + called + " BODY=" + body); + server.close(); + }); + }); + req.on("error", err => { + console.log("UNEXPECTED_ERROR=" + (err.code || err.message)); + server.close(); + }); + req.end(); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("CALLED=true BODY=ok"); + expect(exitCode).toBe(0); + }); + + // Node.js only requests a client certificate when `requestCert: true`. + // Passing `ca` alone must not make the server reject clients that don't + // present one. + test("https.Server with ca but no requestCert accepts clients without a cert", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const https = require("https"); + const server = https.createServer({ + key: ${JSON.stringify(agent1.key)}, + cert: ${JSON.stringify(agent1.cert)}, + ca: ${JSON.stringify(agent1.ca)}, + }, (req, res) => { res.writeHead(200); res.end("ok"); }); + server.listen(0, () => { + const req = https.request({ + port: server.address().port, + rejectUnauthorized: true, + ca: ${JSON.stringify(agent1.ca)}, + servername: "agent1", + }, res => { + let body = ""; + res.on("data", d => body += d); + res.on("end", () => { + console.log("BODY=" + body); + server.close(); + }); + }); + req.on("error", err => { + console.log("UNEXPECTED_ERROR=" + (err.code || err.message)); + server.close(); + }); + req.end(); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("BODY=ok"); + expect(exitCode).toBe(0); + }); +});