From d83779be08ddddf42355d7422a3af91638fb869e Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 20:40:20 -0500 Subject: [PATCH 01/18] fix: validate the encapsulated text of a PEM message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pemToDer()` and `derToPem()` checked a PEM's boundaries and nothing between them: the encapsulated text was `([^-]*)`. `Buffer.from(value, "base64")` discards what it does not recognize, so a corrupt certificate decoded to whatever bytes survived, reaching `KeyInfo` when signing and OpenSSL when verifying, instead of raising an error naming the problem. Structure and data are now separate checks, the data taken with its line breaks removed so that a line may end anywhere without the base64 group becoming ambiguous. Line endings and blanks are normalized before matching rather than described in the patterns, because an `eol` alternation inside a repeated group backtracks exponentially and `[ \t]+` against an anchor is quadratic; this parser reads certificates out of the document under inspection, so every pattern has to stay linear. recheck 4 reports `safe` for all four. Nothing that worked stops working. Every newly rejected value already produced a PEM that OpenSSL refused, and the throw simply moves earlier. What it adds: blanks anywhere in the data, a leading UTF-8 BOM, any line width, a blank line after the header, and concatenated messages. `derToPem()` also returns the same bytes for the same certificate however it arrived, where it used to carry the input's line layout into its output — including blanks after a boundary, which OpenSSL will not read. The three deprecated regex exports move to `index.ts` as frozen copies of what 6.1 exported, since the parser no longer uses them. Co-Authored-By: Claude Opus 5 --- README.md | 35 +++++- src/index.ts | 19 +++- src/signed-xml.ts | 16 +-- src/utils.ts | 178 +++++++++++++++++++++++------- test/signature-unit-tests.spec.ts | 31 ++++++ test/utils-tests.spec.ts | 146 ++++++++++++++++++++++++ 6 files changed, 368 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 26a33a83..746fc4f5 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,13 @@ The `enveloped-signature` transform removes only the `Signature` element being v - `getCanonXml()` finds the loaded signature in the node's document the same way, and removes nothing when the signature is not there. +### Malformed certificates + +A certificate whose encapsulated data is not base64 is rejected rather than decoded as far as it +goes. `Buffer.from(value, "base64")` discards what it does not recognize, so a corrupt certificate +used to reach `KeyInfo`, or OpenSSL, as whatever bytes survived that. What the parser accepts is +described under [X.509 / Key formats](#x509--key-formats). + ### Deprecated ahead of 7.0 These exports are deprecated and will be removed in 7.0: @@ -87,7 +94,7 @@ These exports are deprecated and will be removed in 7.0: | `encodeSpecialCharactersInAttribute`, `encodeSpecialCharactersInText` | these are the escaping step of `C14nCanonicalization` and `ExclusiveCanonicalization`, so use those; a custom canonicalizer must apply [C14N escaping](https://www.w3.org/TR/xml-c14n#ProcessingModel) itself | | `isArrayHasLength` | `Array.isArray(x) && x.length > 0` | | `validateDigestValue` | decode both from base64, then compare with `a.length === b.length && crypto.timingSafeEqual(a, b)` — `timingSafeEqual` alone throws on a length mismatch instead of returning `false`. Never `===` | -| `BASE64_REGEX`, `EXTRACT_X509_CERTS`, `PEM_FORMAT_REGEX` | no replacement; these are internal parsing details | +| `BASE64_REGEX`, `EXTRACT_X509_CERTS`, `PEM_FORMAT_REGEX` | `derToPem()` and `pemToDer()` apply the rules these described, and validate the encapsulated data as well; see [X.509 / Key formats](#x509--key-formats) | Calling one prints a `DeprecationWarning` naming its replacement. The three regexes cannot warn — `util.deprecate` needs a call to intercept — so TypeScript users see the `@deprecated` tag and @@ -552,6 +559,32 @@ MIIBxDCCAW6gAwIBAgIQxUSX... -----END CERTIFICATE----- ``` +### What the parser accepts + +`derToPem()` and `pemToDer()` read [RFC 7468](https://www.rfc-editor.org/rfc/rfc7468) textual +messages, and `derToPem()` also reads bare base64 with a label supplied by the caller. Either form +is judged by the same rules, and `derToPem()` returns the same certificate whatever it arrived as: +`\n` line endings, lines of 64 characters, one message after another. + +Accepted: + +- `\n`, `\r\n` and `\r` line endings, and a leading UTF-8 BOM. +- any line width, a single line included, and a blank line after the header. +- blanks anywhere in the encapsulated data. XMLDSig carries a certificate as + [`xs:base64Binary`](https://www.w3.org/TR/xmlschema11-2/#base64Binary), whose lexical space + allows whitespace, so a pretty-printed document indents it and a value that has been through a + text field may have had its line endings replaced by spaces. +- several messages in one value, of which `derToPem()` keeps all and `pemToDer()` takes none. + +Rejected, with an error rather than a certificate: + +- data outside the base64 alphabet, padding away from the end, or a final quantum that is not + whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). +- a header with no data under it, and a blank line in the middle of the data. +- a value that opens a message it does not close, or one whose header and footer labels disagree. + [Section 3](https://www.rfc-editor.org/rfc/rfc7468#section-3) permits a parser to disregard the + footer's label, but OpenSSL will not read such a message, so neither does this one. + ### Converting .pfx certificates to pem If you have .pfx certificates you can convert them to .pem using [openssl](http://www.openssl.org/): diff --git a/src/index.ts b/src/index.ts index a690d8e4..c70093ed 100644 --- a/src/index.ts +++ b/src/index.ts @@ -110,13 +110,26 @@ export const validateDigestValue = deprecate( * The three regexes below cannot carry a runtime warning: `util.deprecate` wraps a function, and * a `RegExp` has no call to intercept. TypeScript consumers see the `@deprecated` tag; JavaScript * consumers get no signal until the name goes away in 7.0. + * + * They are defined here rather than in `utils.ts` because the parser no longer uses them. They + * are frozen copies of what 6.1 exported, so that a consumer still reading them sees what it has + * always seen until 7.0 removes them. */ /** @deprecated Will be removed in 7.0. This is an internal parsing detail with no replacement. */ -export const PEM_FORMAT_REGEX = utils.PEM_FORMAT_REGEX; +export const PEM_FORMAT_REGEX = new RegExp( + "^-----BEGIN [A-Z\x20]{1,48}-----([^-]*)-----END [A-Z\x20]{1,48}-----$", + "s", +); /** @deprecated Will be removed in 7.0. This is an internal parsing detail with no replacement. */ -export const EXTRACT_X509_CERTS = utils.EXTRACT_X509_CERTS; +export const EXTRACT_X509_CERTS = new RegExp( + "-----BEGIN CERTIFICATE-----[^-]*-----END CERTIFICATE-----", + "g", +); /** @deprecated Will be removed in 7.0. This is an internal parsing detail with no replacement. */ -export const BASE64_REGEX = utils.BASE64_REGEX; +export const BASE64_REGEX = new RegExp( + "^(?:[A-Za-z0-9\\+\\/]{4}\\n{0,1})*(?:[A-Za-z0-9\\+\\/]{2}==|[A-Za-z0-9\\+\\/]{3}=)?$", + "s", +); diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 97bbfd4a..7d04538b 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -228,23 +228,15 @@ export class SignedXml { publicCert = publicCert.toString("latin1"); } - let publicCertMatches: string[] = []; - if (typeof publicCert === "string") { - publicCertMatches = publicCert.match(utils.EXTRACT_X509_CERTS) || []; - } + const certificates = typeof publicCert === "string" ? utils.pemCertificates(publicCert) : []; // X509Data requires at least one child: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data - if (publicCertMatches.length === 0) { + if (certificates.length === 0) { return null; } - const x509Certs = publicCertMatches - .map( - (c) => - `<${prefix}X509Certificate>${utils - .pemToDer(c) - .toString("base64")}`, - ) + const x509Certs = certificates + .map((cert) => `<${prefix}X509Certificate>${cert}`) .join(""); return `<${prefix}X509Data>${x509Certs}`; diff --git a/src/utils.ts b/src/utils.ts index 5aced960..b63d0cac 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -93,31 +93,85 @@ export function encodeSpecialCharactersInText(text: string): string { }); } -/** - * PEM format has wide range of usages, but this library - * is enforcing RFC7468 which focuses on PKIX, PKCS and CMS. - * +/* + * RFC 7468 'textualmsg', with the deviations below. * https://www.rfc-editor.org/rfc/rfc7468 * - * PEM_FORMAT_REGEX is validating given PEM file against RFC7468 'stricttextualmsg' definition. + * - line length is not enforced and several messages may be concatenated: section 2. Section 3 + * lets a parser disregard the label of the post-encapsulation boundary, but this one requires + * the two to agree, because OpenSSL will not read a message whose labels disagree. + * - whitespace around the message is discarded, the '*W' of 'laxtextualmsg' in Figure 2, and a + * leading UTF-8 BOM with it. + * - blanks are discarded wherever they fall in the encapsulated data, not only at the ends of + * lines as Figure 1 permits. XMLDSig carries a certificate as xs:base64Binary, whose lexical + * space allows whitespace, and a pretty-printed document indents it. + * https://www.w3.org/TR/xmlschema11-2/#base64Binary * - * With few exceptions; - * - 'posteb' MAY have 'eol', but it is not mandatory. - * - 'preeb' and 'posteb' lines are limited to 64 characters, but - * should not cause any issues in context of PKIX, PKCS and CMS. + * Structure and data are separate checks, the data taken with its line breaks removed, so that a + * line may end anywhere without `{4}` having to become the ambiguous `{1,4}`. Line endings and + * blanks are normalized away rather than matched, because an 'eol' alternation inside a repeated + * group backtracks exponentially and `[ \t]+` against an anchor is quadratic. Every pattern here + * has to stay provably linear, which only an analyzer can establish and no timing test can: + * `npx recheck@4 check '' ''`. */ -export const PEM_FORMAT_REGEX = new RegExp( - "^-----BEGIN [A-Z\x20]{1,48}-----([^-]*)-----END [A-Z\x20]{1,48}-----$", - "s", -); -export const EXTRACT_X509_CERTS = new RegExp( - "-----BEGIN CERTIFICATE-----[^-]*-----END CERTIFICATE-----", - "g", -); -export const BASE64_REGEX = new RegExp( - "^(?:[A-Za-z0-9\\+\\/]{4}\\n{0,1})*(?:[A-Za-z0-9\\+\\/]{2}==|[A-Za-z0-9\\+\\/]{3}=)?$", - "s", -); +const PEM_FORMAT_REGEX = + /^(?:-----BEGIN [A-Z\x20]{1,48}-----\n+(?:[A-Za-z0-9+/=]+\n)+-----END [A-Z\x20]{1,48}-----\n*)+$/; +const PEM_MESSAGE_REGEX = + /-----BEGIN ([A-Z\x20]{1,48})-----\n+((?:[A-Za-z0-9+/=]+\n)+)-----END ([A-Z\x20]{1,48})-----/g; +const BASE64_LINES_REGEX = /^(?:[A-Za-z0-9+/=]+\n)*[A-Za-z0-9+/=]+$/; +const BASE64_DATA_REGEX = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; + +// A Buffer is decoded latin1 to keep every byte, which leaves a UTF-8 BOM as three characters +// rather than the U+FEFF that `trim()` would take, so both representations are removed here. +const BOM_REGEX = /^(?:\uFEFF|\u00EF\u00BB\u00BF)/; + +function normalizePemInput(text: string): string { + return text + .replace(BOM_REGEX, "") + .replace(/\r\n|\r/g, "\n") + .split("\n") + .map((line) => { + const boundary = line.trim(); + // The blanks of a label are data, as in "RSA PUBLIC KEY"; those of an encoded line are not. + return boundary.startsWith("-----") ? boundary : line.replace(/[ \t]+/g, ""); + }) + .join("\n") + .trim(); +} + +interface PemMessage { + label: string; + endLabel: string; + data: string; +} + +function pemMessages(pem: string): PemMessage[] { + const messages: PemMessage[] = []; + + // Global regexes carry `lastIndex` between calls, so the loop runs to exhaustion to return it. + PEM_MESSAGE_REGEX.lastIndex = 0; + let message = PEM_MESSAGE_REGEX.exec(pem); + while (message !== null) { + messages.push({ + label: message[1], + endLabel: message[3], + // A line break inside base64 is presentation and never data, so where a line ends is a + // question for the structure check alone, and the data is carried de-lined from here on. + data: message[2].replace(/\n/g, ""), + }); + message = PEM_MESSAGE_REGEX.exec(pem); + } + + return messages; +} + +function isBase64Data(data: string): boolean { + return BASE64_DATA_REGEX.test(data); +} + +function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { + return label === endLabel && isBase64Data(data); +} /** * -----BEGIN [LABEL]----- @@ -130,13 +184,10 @@ export const BASE64_REGEX = new RegExp( * This function normalizes PEM presentation to; * - contain PEM header and footer as they are given * - normalize line endings to '\n' - * - normalize line length to maximum of 64 characters + * - split lines longer than 64 characters, leaving shorter ones as they are * - ensure that 'preeb' has line ending '\n' * - * With a couple of notes: - * - 'eol' is normalized to '\n' - * - * @param pem The PEM string to normalize to RFC7468 'stricttextualmsg' definition + * @param pem The PEM string to normalize */ export function normalizePem(pem: string): string { return `${( @@ -147,45 +198,90 @@ export function normalizePem(pem: string): string { ).join("\n")}\n`; } +// Rebuilt from the data rather than passed through, so that the same certificate produces the +// same bytes whatever line width, line ending or blanks it arrived with. +function formatPemMessage(label: string, data: string): string { + return normalizePem(`-----BEGIN ${label}-----\n${data}\n-----END ${label}-----`); +} + +/** + * Returns the base64 data of each `CERTIFICATE` message in a PEM value, and `[]` when the value + * holds no certificate: bare base64, or messages of other labels. + * + * @param pem The PEM value to read certificates from + * @throws Error if the value opens a message it does not close as a well-formed PEM, or if a + * certificate's labels disagree or its data is not base64 + */ +export function pemCertificates(pem: string): string[] { + const text = normalizePemInput(pem); + + if (!PEM_FORMAT_REGEX.test(text)) { + // A value with no boundaries at all holds no certificate to publish, but one that opens a + // message it cannot finish is a certificate we failed to read, and dropping it would sign + // without the KeyInfo the caller asked for. + if (text.includes("-----BEGIN ")) { + throw new Error("Invalid PEM format."); + } + + return []; + } + + const certificates = pemMessages(text).filter((message) => message.label === "CERTIFICATE"); + if (!certificates.every(isWellFormedMessage)) { + throw new Error("Invalid PEM format."); + } + + return certificates.map((certificate) => certificate.data); +} + /** * @param pem The PEM-encoded base64 certificate to strip headers from + * @throws Error if the value is not a single well-formed PEM message */ export function pemToDer(pem: string): Buffer { - if (!PEM_FORMAT_REGEX.test(pem.trim())) { + const text = normalizePemInput(pem); + const messages = PEM_FORMAT_REGEX.test(text) ? pemMessages(text) : []; + + if (messages.length > 1) { + throw new Error(`Expected a single PEM message, but found ${messages.length}.`); + } + + if (messages.length === 0 || !isWellFormedMessage(messages[0])) { throw new Error("Invalid PEM format."); } - return Buffer.from( - pem - .replace(/(\r\n|\r)/g, "") - .replace(/-----BEGIN [A-Z\x20]{1,48}-----\n?/, "") - .replace(/-----END [A-Z\x20]{1,48}-----\n?/, ""), - "base64", - ); + return Buffer.from(messages[0].data, "base64"); } /** * @param der The DER-encoded base64 certificate to add PEM headers too * @param pemLabel The label of the header and footer to add + * @throws Error if the value is neither a well-formed PEM nor base64, or if it is base64 and no + * label was given */ export function derToPem( der: string | Buffer, pemLabel?: "CERTIFICATE" | "PRIVATE KEY" | "RSA PUBLIC KEY", ): string { - const trimmed = Buffer.isBuffer(der) ? der.toString("base64").trim() : der.trim(); + const text = normalizePemInput(Buffer.isBuffer(der) ? der.toString("base64") : der); + + if (PEM_FORMAT_REGEX.test(text)) { + const messages = pemMessages(text); + if (!messages.every(isWellFormedMessage)) { + throw new Error("Unknown DER format."); + } - if (PEM_FORMAT_REGEX.test(trimmed)) { - return normalizePem(trimmed); + return messages.map((message) => formatPemMessage(message.label, message.data)).join(""); } - const base64Der = trimmed.replace(/\r\n|\r| /g, ""); - if (BASE64_REGEX.test(base64Der)) { + const data = text.replace(/\n/g, ""); + + if (BASE64_LINES_REGEX.test(text) && isBase64Data(data)) { if (pemLabel == null) { throw new Error("PEM label is required when DER is given."); } - const pem = `-----BEGIN ${pemLabel}-----\n${base64Der}\n-----END ${pemLabel}-----`; - return normalizePem(pem); + return formatPemMessage(pemLabel, data); } throw new Error("Unknown DER format."); diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 4d16b442..a8a1ee4f 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1440,6 +1440,37 @@ describe("Signature unit tests", function () { }); }); + function signWithPublicCert(publicCert: string) { + const sig = new SignedXml({ + privateKey: fs.readFileSync("./test/static/client.pem"), + publicCert, + canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", + signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + }); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + }); + + return () => sig.computeSignature(""); + } + + it("refuses to sign with a publicCert whose two labels disagree", function () { + const publicCert = fs + .readFileSync("./test/static/client_public.pem", "latin1") + .replace("-----END CERTIFICATE-----", "-----END PRIVATE KEY-----"); + + expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); + }); + + it("refuses to sign with a publicCert whose certificate is not base64", function () { + const lines = fs.readFileSync("./test/static/client_public.pem", "latin1").trim().split("\n"); + const publicCert = [lines[0], "not base64 at all!", lines[lines.length - 1]].join("\n"); + + expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); + }); + it("adds id and type attributes to Reference elements when provided", function () { const xml = ""; const sig = new SignedXml(); diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index d90e5145..287a0028 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -1,3 +1,4 @@ +import * as crypto from "crypto"; import * as fs from "fs"; import * as utils from "../src/utils"; import { expect } from "chai"; @@ -72,6 +73,138 @@ describe("Utils tests", function () { const derBuffer = fs.readFileSync("./test/static/client_public.der"); expect(() => utils.derToPem(derBuffer)).to.throw(); }); + + describe("judges the same data with and without encapsulation boundaries", function () { + const wrap = (data: string) => + `-----BEGIN CERTIFICATE-----\n${data}\n-----END CERTIFICATE-----`; + + const accepted = { + "a whole quantum": "QUJD", + "a quantum padded to two characters": "QUJDRQ==", + "a quantum padded to three characters": "QUJDREU=", + "a line break between quanta": "QUJD\nREVG", + "a line break anywhere in the data": "QU\nJDRE\nVG", + "blanks in the data": " QU JD\tREVG ", + }; + + const rejected = { + "a lone character": "A", + "a character and a pad": "A=", + "three characters and two pads": "AAA==", + "a whole quantum and a pad": "AAAA=", + "a quantum and one character": "AAAAA", + "a pad in the middle of the data": "QUJD=REVG", + "a character outside the base64 alphabet": "QU-JD", + "nothing at all": "", + "only blanks": " ", + }; + + Object.entries(accepted).forEach(([description, data]) => { + it(`accepts ${description} either way, and reads the same certificate`, function () { + expect(utils.derToPem(wrap(data))).to.equal(utils.derToPem(data, "CERTIFICATE")); + }); + }); + + Object.entries(rejected).forEach(([description, data]) => { + it(`rejects ${description} either way, for the same reason`, function () { + expect(() => utils.derToPem(wrap(data), "CERTIFICATE")).to.throw("Unknown DER format."); + expect(() => utils.derToPem(data, "CERTIFICATE")).to.throw("Unknown DER format."); + }); + }); + }); + + describe("accepts what common tooling produces", function () { + const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const lines = normalizedPem.trim().split("\n"); + const body = lines.slice(1, -1); + const rebuild = (bodyLines: string[]) => + [lines[0], ...bodyLines, lines[lines.length - 1]].join("\n"); + + it("blanks at the ends of lines", function () { + expect(utils.derToPem(rebuild(body.map((line) => `${line} `)))).to.equal(normalizedPem); + }); + + it("a pretty-printer's indentation", function () { + expect(utils.derToPem(rebuild(body.map((line) => ` ${line}`)))).to.equal(normalizedPem); + }); + + it("a line ending replaced by a space", function () { + expect(utils.derToPem(rebuild([body.join(" ")]))).to.equal(normalizedPem); + }); + + it("a blank line after the header", function () { + expect(utils.derToPem(rebuild(["", ...body]))).to.equal(normalizedPem); + }); + + it("a line width other than 64", function () { + const rewrapped = body.join("").match(/.{1,70}/g) ?? []; + + expect(utils.derToPem(rebuild(rewrapped))).to.equal(normalizedPem); + }); + + it("a UTF-8 BOM, as decoded text", function () { + expect(utils.derToPem(`\uFEFF${normalizedPem}`)).to.equal(normalizedPem); + }); + + it("a UTF-8 BOM, as the bytes a latin1 read gives", function () { + const withBom = Buffer.concat([ + Buffer.from([0xef, 0xbb, 0xbf]), + Buffer.from(normalizedPem, "latin1"), + ]); + + expect(utils.derToPem(withBom.toString("latin1"))).to.equal(normalizedPem); + }); + + it("several certificates in one value", function () { + const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); + + expect(utils.derToPem(bundle).match(/-----BEGIN CERTIFICATE-----/g)).to.have.lengthOf(2); + }); + + it("and hands OpenSSL something it can load", function () { + // Blanks after an encapsulation boundary are the 'preeb *WSP eol' of RFC 7468 Figure 1, + // and OpenSSL will not read a certificate that carries them. + const untidy = rebuild(body) + .split("\n") + .map((line) => `${line} `) + .join("\r\n"); + + expect(() => crypto.createPublicKey(utils.derToPem(untidy))).to.not.throw(); + }); + }); + + describe("rejects data that is not base64", function () { + const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const lines = normalizedPem.trim().split("\n"); + const corrupt = (body: string) => [lines[0], body, lines[lines.length - 1]].join("\n"); + + it("a body that is not base64 at all", function () { + expect(() => utils.derToPem(corrupt("not base64 at all!"))).to.throw("Unknown DER format."); + }); + + it("a body that is one long run of blanks", function () { + expect(() => utils.derToPem(corrupt(" ".repeat(80000)))).to.throw("Unknown DER format."); + }); + + it("a body with a blank line in the middle of the data", function () { + const body = lines.slice(1, -1); + const interrupted = [...body.slice(0, 2), "", ...body.slice(2)].join("\n"); + + expect(() => utils.derToPem(corrupt(interrupted))).to.throw("Unknown DER format."); + }); + + it("a message whose two labels disagree", function () { + const mismatched = normalizedPem.replace("-----END CERTIFICATE-----", "-----END KEY-----"); + + expect(() => utils.derToPem(mismatched)).to.throw("Unknown DER format."); + }); + + it("a body whose final quantum is incomplete", function () { + expect(() => utils.derToPem(corrupt(`${lines.slice(1, -1).join("\n")}A`))).to.throw( + "Unknown DER format.", + ); + }); + }); }); describe("pemToDer", function () { @@ -87,5 +220,18 @@ describe("Utils tests", function () { it("will throw if the format is not PEM", function () { expect(() => utils.pemToDer("not a pem")).to.throw(); }); + + it("will throw if the encapsulated data is not base64", function () { + const pem = fs.readFileSync("./test/static/client_public.pem", "latin1").trim().split("\n"); + const corrupt = [pem[0], "not base64 at all!", pem[pem.length - 1]].join("\n"); + + expect(() => utils.pemToDer(corrupt)).to.throw("Invalid PEM format."); + }); + + it("will throw if the value holds more than one PEM message", function () { + const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); + + expect(() => utils.pemToDer(bundle)).to.throw("Expected a single PEM message, but found 3."); + }); }); }); From 960b780e676c620dce33e44a265f4c22df48f553 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 20:44:09 -0500 Subject: [PATCH 02/18] test: complete the quantum and line-ending cases RFC 4648 section 4 leaves four ways for a final quantum to be wrong, and the table named two of them. Adds the unpadded and over-padded forms, the two line-ending conventions in their bare form, and a bundle whose messages are separated by a blank line. Co-Authored-By: Claude Opus 5 --- test/utils-tests.spec.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 287a0028..5a5635d8 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -89,10 +89,13 @@ describe("Utils tests", function () { const rejected = { "a lone character": "A", + "two characters with no pad": "AA", + "three characters with no pad": "AAA", "a character and a pad": "A=", "three characters and two pads": "AAA==", "a whole quantum and a pad": "AAAA=", "a quantum and one character": "AAAAA", + "a whole quantum and two pads": "AAAA==", "a pad in the middle of the data": "QUJD=REVG", "a character outside the base64 alphabet": "QU-JD", "nothing at all": "", @@ -132,6 +135,24 @@ describe("Utils tests", function () { expect(utils.derToPem(rebuild([body.join(" ")]))).to.equal(normalizedPem); }); + for (const [name, eol] of [ + ["CRLF", "\r\n"], + ["CR", "\r"], + ] as const) { + it(`${name} line endings, with and without boundaries`, function () { + const bare = body.join("\n").replace(/\n/g, eol); + + expect(utils.derToPem(bare, "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.derToPem(rebuild(body).replace(/\n/g, eol))).to.equal(normalizedPem); + }); + } + + it("a blank line between concatenated messages", function () { + const pair = `${normalizedPem}\n\n${normalizedPem}`; + + expect(utils.derToPem(pair)).to.equal(`${normalizedPem}${normalizedPem}`); + }); + it("a blank line after the header", function () { expect(utils.derToPem(rebuild(["", ...body]))).to.equal(normalizedPem); }); From 83571b650632e08edc64b981674dc70bf921b40c Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 21:35:41 -0500 Subject: [PATCH 03/18] feat: own the PEM parser, and name it for what it does `derToPem()` never took DER alone: its input is a PEM message, several of them, bare base64, or a Buffer, and DER is one case of the last. It is renamed `toPem()` and re-exported through `util.deprecate` under the old name until 7.0. The label grammar is now RFC 7468's own, `%x21-2C / %x2E-7E` with `-` and SP as interior separators, in place of `[A-Z ]{1,48}`. That accepts the registered labels this rejected, `X509 CRL` and `PKCS7` among them, and the ones OpenSSL adds. Because a separator is always followed by a label character, `--` can never occur in a label, so a caller-supplied label cannot write a second boundary into the message; `toPem()` checks the label it is given against the same grammar for that reason. `pemCertificates()` is exported. It returns the base64 of each CERTIFICATE message and ignores every other label, which is what keeps a private key in the same value out of `KeyInfo`. A Buffer that opens with an encapsulation boundary is read as the bytes of a PEM file rather than base64-encoded, which used to produce a message whose body was base64-encoded PEM, with no error. `PemLabel` names the labels RFC 7468 registers and, like the other algorithm types, stays open to the rest. The three new patterns report `safe` under `npx recheck@4`. Co-Authored-By: Claude Opus 5 --- README.md | 36 +++++-- src/index.ts | 12 ++- src/signed-xml.ts | 2 +- src/types.ts | 19 ++++ src/utils.ts | 71 ++++++++++--- test/signed-references-tests.spec.ts | 2 +- test/utils-tests.spec.ts | 146 +++++++++++++++++++++------ 7 files changed, 230 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index 746fc4f5..d90d4fd3 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,11 @@ goes. `Buffer.from(value, "base64")` discards what it does not recognize, so a c used to reach `KeyInfo`, or OpenSSL, as whatever bytes survived that. What the parser accepts is described under [X.509 / Key formats](#x509--key-formats). +The error for a value the parser cannot read is `Invalid PEM format.`, in place of the +`Unknown DER format.` that `derToPem()` threw. A `Buffer` holding the bytes of a PEM file is read +as that file rather than base64-encoded, which is what made it a message with a body of +base64-encoded PEM. Both forms of PEM the parser does read produce the same canonical output. + ### Deprecated ahead of 7.0 These exports are deprecated and will be removed in 7.0: @@ -94,14 +99,15 @@ These exports are deprecated and will be removed in 7.0: | `encodeSpecialCharactersInAttribute`, `encodeSpecialCharactersInText` | these are the escaping step of `C14nCanonicalization` and `ExclusiveCanonicalization`, so use those; a custom canonicalizer must apply [C14N escaping](https://www.w3.org/TR/xml-c14n#ProcessingModel) itself | | `isArrayHasLength` | `Array.isArray(x) && x.length > 0` | | `validateDigestValue` | decode both from base64, then compare with `a.length === b.length && crypto.timingSafeEqual(a, b)` — `timingSafeEqual` alone throws on a length mismatch instead of returning `false`. Never `===` | -| `BASE64_REGEX`, `EXTRACT_X509_CERTS`, `PEM_FORMAT_REGEX` | `derToPem()` and `pemToDer()` apply the rules these described, and validate the encapsulated data as well; see [X.509 / Key formats](#x509--key-formats) | +| `BASE64_REGEX`, `EXTRACT_X509_CERTS`, `PEM_FORMAT_REGEX` | `toPem()`, `pemToDer()` and `pemCertificates()` apply the rules these described, and validate the encapsulated data as well; see [X.509 / Key formats](#x509--key-formats) | +| `derToPem` | `toPem()`, which is the same function under a name that describes it: it takes a PEM message, several of them, base64, or a Buffer, and DER is only one of those | Calling one prints a `DeprecationWarning` naming its replacement. The three regexes cannot warn — `util.deprecate` needs a call to intercept — so TypeScript users see the `@deprecated` tag and JavaScript users get no signal until the names go away. -`derToPem`, `pemToDer`, `normalizePem` and `findAncestorNs` are **not** deprecated and stay -exported. +`toPem`, `pemToDer`, `pemCertificates`, `normalizePem` and `findAncestorNs` are **not** +deprecated and stay exported. `getReferences()` and `references` are deprecated. Do not use them to obtain signed XML; use `getSignedReferences()` instead, as shown in [Verifying Xml documents](#verifying-xml-documents). @@ -561,10 +567,18 @@ MIIBxDCCAW6gAwIBAgIQxUSX... ### What the parser accepts -`derToPem()` and `pemToDer()` read [RFC 7468](https://www.rfc-editor.org/rfc/rfc7468) textual -messages, and `derToPem()` also reads bare base64 with a label supplied by the caller. Either form -is judged by the same rules, and `derToPem()` returns the same certificate whatever it arrived as: -`\n` line endings, lines of 64 characters, one message after another. +`toPem()`, `pemToDer()` and `pemCertificates()` read +[RFC 7468](https://www.rfc-editor.org/rfc/rfc7468) textual messages, and `toPem()` also reads bare +base64 with a label supplied by the caller. Either form is judged by the same rules, and `toPem()` +returns the same certificate whatever it arrived as: `\n` line endings, lines of 64 characters, +one message after another. + +- `toPem(value, label?)` returns canonical PEM. A Buffer that opens with an encapsulation + boundary is read as the bytes of a PEM file and any other as raw DER, so base64 text is given + as a string rather than a Buffer. +- `pemToDer(pem)` returns the decoded bytes of the one message a value holds. +- `pemCertificates(pem)` returns the base64 of each `CERTIFICATE` message and ignores messages of + any other label, so a private key in the same value is never published. Accepted: @@ -574,7 +588,10 @@ Accepted: [`xs:base64Binary`](https://www.w3.org/TR/xmlschema11-2/#base64Binary), whose lexical space allows whitespace, so a pretty-printed document indents it and a value that has been through a text field may have had its line endings replaced by spaces. -- several messages in one value, of which `derToPem()` keeps all and `pemToDer()` takes none. +- several messages in one value, of which `toPem()` keeps all, `pemCertificates()` takes the + certificates, and `pemToDer()` takes none. +- any label RFC 7468's grammar allows, which is every registered label and the ones OpenSSL adds, + such as `RSA PRIVATE KEY`. `PemLabel` names the registered ones. Rejected, with an error rather than a certificate: @@ -584,6 +601,9 @@ Rejected, with an error rather than a certificate: - a value that opens a message it does not close, or one whose header and footer labels disagree. [Section 3](https://www.rfc-editor.org/rfc/rfc7468#section-3) permits a parser to disregard the footer's label, but OpenSSL will not read such a message, so neither does this one. +- a label outside RFC 7468's grammar, which is one holding `--` or opening or closing with a + blank. A label is written into both boundaries, so one holding `--` would produce a message + this parser could not read back. ### Converting .pfx certificates to pem diff --git a/src/index.ts b/src/index.ts index c70093ed..2260806c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,7 +9,7 @@ export { export { SignedXml } from "./signed-xml"; export * from "./types"; -export { derToPem, findAncestorNs, normalizePem, pemToDer } from "./utils"; +export { findAncestorNs, normalizePem, pemCertificates, pemToDer, toPem } from "./utils"; /* * `index.ts` used to re-export `./utils` wholesale, so helpers written for `signed-xml.ts` to @@ -106,6 +106,16 @@ export const validateDigestValue = deprecate( "XML_CRYPTO_VALIDATE_DIGEST_VALUE", ); +/** + * @deprecated Will be removed in 7.0. Renamed to `toPem()`, which is what it has always done: + * its input is a PEM message, several of them, base64, or a Buffer of either PEM or DER. + */ +export const derToPem = deprecate( + utils.toPem, + "`derToPem()` is deprecated and will be removed in version 7.0. Use `toPem()` instead.", + "XML_CRYPTO_DER_TO_PEM", +); + /* * The three regexes below cannot carry a runtime warning: `util.deprecate` wraps a function, and * a `RegExp` has no call to intercept. TypeScript consumers see the `@deprecated` tag; JavaScript diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 7d04538b..d17284ae 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -253,7 +253,7 @@ export class SignedXml { if (keyInfo != null) { const cert = xpath.select1(".//*[local-name(.)='X509Certificate']", keyInfo); if (isDomNode.isNodeLike(cert)) { - return utils.derToPem(cert.textContent ?? "", "CERTIFICATE"); + return utils.toPem(cert.textContent ?? "", "CERTIFICATE"); } } diff --git a/src/types.ts b/src/types.ts index 2dc41052..2db9d08c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -26,6 +26,25 @@ export type HashAlgorithmType = | "http://www.w3.org/2001/04/xmlenc#sha512" | string; +/** + * The label carried by both boundaries of a PEM message. The values listed are the ones RFC 7468 + * defines; any label its grammar allows is accepted, because OpenSSL and the wider ecosystem use + * others, such as `RSA PRIVATE KEY`. + * + * @see https://www.rfc-editor.org/rfc/rfc7468 + */ +export type PemLabel = + | "CERTIFICATE" + | "X509 CRL" + | "CERTIFICATE REQUEST" + | "PKCS7" + | "CMS" + | "PRIVATE KEY" + | "ENCRYPTED PRIVATE KEY" + | "ATTRIBUTE CERTIFICATE" + | "PUBLIC KEY" + | string; + export type SignatureAlgorithmType = | "http://www.w3.org/2000/09/xmldsig#rsa-sha1" | "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" diff --git a/src/utils.ts b/src/utils.ts index b63d0cac..7ff71e40 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,5 +1,5 @@ import * as xpath from "xpath"; -import type { NamespacePrefix } from "./types"; +import type { NamespacePrefix, PemLabel } from "./types"; import * as isDomNode from "@xmldom/is-dom-node"; export function isArrayHasLength(array: unknown): array is unknown[] { @@ -106,6 +106,8 @@ export function encodeSpecialCharactersInText(text: string): string { * lines as Figure 1 permits. XMLDSig carries a certificate as xs:base64Binary, whose lexical * space allows whitespace, and a pretty-printed document indents it. * https://www.w3.org/TR/xmlschema11-2/#base64Binary + * - a label is limited to 48 label characters, which no registered label comes close to, so that + * a boundary cannot be made arbitrarily long. * * Structure and data are separate checks, the data taken with its line breaks removed, so that a * line may end anywhere without `{4}` having to become the ambiguous `{1,4}`. Line endings and @@ -114,10 +116,25 @@ export function encodeSpecialCharactersInText(text: string): string { * has to stay provably linear, which only an analyzer can establish and no timing test can: * `npx recheck@4 check '' ''`. */ -const PEM_FORMAT_REGEX = - /^(?:-----BEGIN [A-Z\x20]{1,48}-----\n+(?:[A-Za-z0-9+/=]+\n)+-----END [A-Z\x20]{1,48}-----\n*)+$/; -const PEM_MESSAGE_REGEX = - /-----BEGIN ([A-Z\x20]{1,48})-----\n+((?:[A-Za-z0-9+/=]+\n)+)-----END ([A-Z\x20]{1,48})-----/g; + +/* + * Section 3 gives `labelchar = %x21-2C / %x2E-7E` and + * `label = [ labelchar *( ["-" / SP] labelchar ) ]`. A separator is always followed by a label + * character, so `--` can never occur inside a label and no boundary can be smuggled into one. + * A label character is neither `-` nor a blank, which is what keeps the two disjoint and the + * patterns below unambiguous. + */ +const LABEL_CHAR = "[\\x21-\\x2C\\x2E-\\x7E]"; +const LABEL = `${LABEL_CHAR}(?:[-\\x20]?${LABEL_CHAR}){0,47}`; + +const LABEL_REGEX = new RegExp(`^${LABEL}$`); +const PEM_FORMAT_REGEX = new RegExp( + `^(?:-----BEGIN ${LABEL}-----\\n+(?:[A-Za-z0-9+/=]+\\n)+-----END ${LABEL}-----\\n*)+$`, +); +const PEM_MESSAGE_REGEX = new RegExp( + `-----BEGIN (${LABEL})-----\\n+((?:[A-Za-z0-9+/=]+\\n)+)-----END (${LABEL})-----`, + "g", +); const BASE64_LINES_REGEX = /^(?:[A-Za-z0-9+/=]+\n)*[A-Za-z0-9+/=]+$/; const BASE64_DATA_REGEX = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; @@ -253,22 +270,38 @@ export function pemToDer(pem: string): Buffer { return Buffer.from(messages[0].data, "base64"); } +// A Buffer holds either the bytes of a PEM file or raw DER. DER is ASN.1, whose every encoding +// opens with a tag byte, never with the `-` of a boundary, so the two cannot be confused. +function pemText(value: string | Buffer): string { + if (!Buffer.isBuffer(value)) { + return value; + } + + const text = value.toString("latin1"); + + return text.replace(BOM_REGEX, "").trimStart().startsWith("-----BEGIN ") + ? text + : value.toString("base64"); +} + /** - * @param der The DER-encoded base64 certificate to add PEM headers too - * @param pemLabel The label of the header and footer to add + * Returns a value as canonical PEM: one message per certificate or key, wrapped at 64 characters. + * The value may be a PEM message, several of them, base64 data with the label supplied by the + * caller, or a Buffer. A Buffer that opens with an encapsulation boundary is read as the bytes of + * a PEM file and any other as raw DER, so base64 text is given as a string rather than a Buffer. + * + * @param value The certificate or key to return as PEM + * @param pemLabel The label to give base64 data, which needs one; ignored when the value is PEM * @throws Error if the value is neither a well-formed PEM nor base64, or if it is base64 and no - * label was given + * label, or an unusable one, was given */ -export function derToPem( - der: string | Buffer, - pemLabel?: "CERTIFICATE" | "PRIVATE KEY" | "RSA PUBLIC KEY", -): string { - const text = normalizePemInput(Buffer.isBuffer(der) ? der.toString("base64") : der); +export function toPem(value: string | Buffer, pemLabel?: PemLabel): string { + const text = normalizePemInput(pemText(value)); if (PEM_FORMAT_REGEX.test(text)) { const messages = pemMessages(text); if (!messages.every(isWellFormedMessage)) { - throw new Error("Unknown DER format."); + throw new Error("Invalid PEM format."); } return messages.map((message) => formatPemMessage(message.label, message.data)).join(""); @@ -278,13 +311,19 @@ export function derToPem( if (BASE64_LINES_REGEX.test(text) && isBase64Data(data)) { if (pemLabel == null) { - throw new Error("PEM label is required when DER is given."); + throw new Error("A PEM label is required to wrap base64 data."); + } + + // The label is written into both boundaries, so one that is not a label would produce a + // message this parser could not read back, and `-----` in it would produce a second message. + if (!LABEL_REGEX.test(pemLabel)) { + throw new Error("Invalid PEM label."); } return formatPemMessage(pemLabel, data); } - throw new Error("Unknown DER format."); + throw new Error("Invalid PEM format."); } function collectAncestorNamespaces( diff --git a/test/signed-references-tests.spec.ts b/test/signed-references-tests.spec.ts index a5ae3e70..5f2e0542 100644 --- a/test/signed-references-tests.spec.ts +++ b/test/signed-references-tests.spec.ts @@ -77,7 +77,7 @@ describe("Signed references", function () { "%%%", ), { getCertFromKeyInfo: SignedXml.getCertFromKeyInfo }, - "Unknown DER format.", + "Invalid PEM format.", ], ]; diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 5a5635d8..eb56d492 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -7,7 +7,7 @@ import * as xpath from "xpath"; import * as isDomNode from "@xmldom/is-dom-node"; describe("Utils tests", function () { - describe("derToPem", function () { + describe("toPem", function () { it("will return a normalized PEM format when given an non-normalized PEM format", function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); const pemAsArray = normalizedPem.trim().split("\n"); @@ -16,14 +16,14 @@ describe("Utils tests", function () { pemAsArray[pemAsArray.length - 1] }`; - expect(utils.derToPem(nonNormalizedPem)).to.equal(normalizedPem); + expect(utils.toPem(nonNormalizedPem)).to.equal(normalizedPem); }); for (const eol of ["\r\n", "\r"]) { it(`will return a normalized PEM format when given a PEM with ${JSON.stringify(eol)} line endings`, function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); - expect(utils.derToPem(normalizedPem.replace(/\n/g, eol))).to.equal(normalizedPem); + expect(utils.toPem(normalizedPem.replace(/\n/g, eol))).to.equal(normalizedPem); }); } @@ -32,25 +32,25 @@ describe("Utils tests", function () { const pemAsArray = normalizedPem.trim().split("\n"); const base64String = pemAsArray.slice(1, -1).join(""); - expect(utils.derToPem(base64String, "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.toPem(base64String, "CERTIFICATE")).to.equal(normalizedPem); }); it("will throw if the format is neither PEM nor DER", function () { - expect(() => utils.derToPem("not a pem")).to.throw(); + expect(() => utils.toPem("not a pem")).to.throw(); }); it("will return a normalized PEM format when given a DER Buffer", function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); const derBuffer = fs.readFileSync("./test/static/client_public.der"); - expect(utils.derToPem(derBuffer, "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.toPem(derBuffer, "CERTIFICATE")).to.equal(normalizedPem); }); it("will return a normalized PEM format when given a base64 string with line breaks", function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); const base64String = fs.readFileSync("./test/static/client_public.der", "base64"); - expect(utils.derToPem(base64String, "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.toPem(base64String, "CERTIFICATE")).to.equal(normalizedPem); }); it("will return a normalized PEM format when given a base64 string with line breaks and spaces at the line breaks", function () { @@ -62,16 +62,16 @@ describe("Utils tests", function () { const normalizedPem = fs.readFileSync("./test/static/keyinfo.pem", "latin1"); - expect(utils.derToPem(cert.textContent ?? "", "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.toPem(cert.textContent ?? "", "CERTIFICATE")).to.equal(normalizedPem); }); it("will throw if the DER string is not base64 encoded", function () { - expect(() => utils.derToPem("not base64", "CERTIFICATE")).to.throw(); + expect(() => utils.toPem("not base64", "CERTIFICATE")).to.throw(); }); it("will throw if the PEM label is not provided", function () { const derBuffer = fs.readFileSync("./test/static/client_public.der"); - expect(() => utils.derToPem(derBuffer)).to.throw(); + expect(() => utils.toPem(derBuffer)).to.throw(); }); describe("judges the same data with and without encapsulation boundaries", function () { @@ -104,14 +104,14 @@ describe("Utils tests", function () { Object.entries(accepted).forEach(([description, data]) => { it(`accepts ${description} either way, and reads the same certificate`, function () { - expect(utils.derToPem(wrap(data))).to.equal(utils.derToPem(data, "CERTIFICATE")); + expect(utils.toPem(wrap(data))).to.equal(utils.toPem(data, "CERTIFICATE")); }); }); Object.entries(rejected).forEach(([description, data]) => { it(`rejects ${description} either way, for the same reason`, function () { - expect(() => utils.derToPem(wrap(data), "CERTIFICATE")).to.throw("Unknown DER format."); - expect(() => utils.derToPem(data, "CERTIFICATE")).to.throw("Unknown DER format."); + expect(() => utils.toPem(wrap(data), "CERTIFICATE")).to.throw("Invalid PEM format."); + expect(() => utils.toPem(data, "CERTIFICATE")).to.throw("Invalid PEM format."); }); }); }); @@ -124,15 +124,15 @@ describe("Utils tests", function () { [lines[0], ...bodyLines, lines[lines.length - 1]].join("\n"); it("blanks at the ends of lines", function () { - expect(utils.derToPem(rebuild(body.map((line) => `${line} `)))).to.equal(normalizedPem); + expect(utils.toPem(rebuild(body.map((line) => `${line} `)))).to.equal(normalizedPem); }); it("a pretty-printer's indentation", function () { - expect(utils.derToPem(rebuild(body.map((line) => ` ${line}`)))).to.equal(normalizedPem); + expect(utils.toPem(rebuild(body.map((line) => ` ${line}`)))).to.equal(normalizedPem); }); it("a line ending replaced by a space", function () { - expect(utils.derToPem(rebuild([body.join(" ")]))).to.equal(normalizedPem); + expect(utils.toPem(rebuild([body.join(" ")]))).to.equal(normalizedPem); }); for (const [name, eol] of [ @@ -142,29 +142,29 @@ describe("Utils tests", function () { it(`${name} line endings, with and without boundaries`, function () { const bare = body.join("\n").replace(/\n/g, eol); - expect(utils.derToPem(bare, "CERTIFICATE")).to.equal(normalizedPem); - expect(utils.derToPem(rebuild(body).replace(/\n/g, eol))).to.equal(normalizedPem); + expect(utils.toPem(bare, "CERTIFICATE")).to.equal(normalizedPem); + expect(utils.toPem(rebuild(body).replace(/\n/g, eol))).to.equal(normalizedPem); }); } it("a blank line between concatenated messages", function () { const pair = `${normalizedPem}\n\n${normalizedPem}`; - expect(utils.derToPem(pair)).to.equal(`${normalizedPem}${normalizedPem}`); + expect(utils.toPem(pair)).to.equal(`${normalizedPem}${normalizedPem}`); }); it("a blank line after the header", function () { - expect(utils.derToPem(rebuild(["", ...body]))).to.equal(normalizedPem); + expect(utils.toPem(rebuild(["", ...body]))).to.equal(normalizedPem); }); it("a line width other than 64", function () { const rewrapped = body.join("").match(/.{1,70}/g) ?? []; - expect(utils.derToPem(rebuild(rewrapped))).to.equal(normalizedPem); + expect(utils.toPem(rebuild(rewrapped))).to.equal(normalizedPem); }); it("a UTF-8 BOM, as decoded text", function () { - expect(utils.derToPem(`\uFEFF${normalizedPem}`)).to.equal(normalizedPem); + expect(utils.toPem(`\uFEFF${normalizedPem}`)).to.equal(normalizedPem); }); it("a UTF-8 BOM, as the bytes a latin1 read gives", function () { @@ -173,13 +173,19 @@ describe("Utils tests", function () { Buffer.from(normalizedPem, "latin1"), ]); - expect(utils.derToPem(withBom.toString("latin1"))).to.equal(normalizedPem); + expect(utils.toPem(withBom.toString("latin1"))).to.equal(normalizedPem); + }); + + it("a Buffer holding the bytes of a PEM file, rather than DER", function () { + expect(utils.toPem(fs.readFileSync("./test/static/client_public.pem"))).to.equal( + normalizedPem, + ); }); it("several certificates in one value", function () { const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); - expect(utils.derToPem(bundle).match(/-----BEGIN CERTIFICATE-----/g)).to.have.lengthOf(2); + expect(utils.toPem(bundle).match(/-----BEGIN CERTIFICATE-----/g)).to.have.lengthOf(2); }); it("and hands OpenSSL something it can load", function () { @@ -190,8 +196,49 @@ describe("Utils tests", function () { .map((line) => `${line} `) .join("\r\n"); - expect(() => crypto.createPublicKey(utils.derToPem(untidy))).to.not.throw(); + expect(() => crypto.createPublicKey(utils.toPem(untidy))).to.not.throw(); + }); + }); + + describe("labels", function () { + const data = fs + .readFileSync("./test/static/client_public.pem", "latin1") + .trim() + .split("\n") + .slice(1, -1) + .join(""); + + for (const label of ["CERTIFICATE", "PUBLIC KEY", "X509 CRL", "ENCRYPTED PRIVATE KEY"]) { + it(`wraps base64 in a message labelled "${label}"`, function () { + const pem = utils.toPem(data, label); + + expect(pem).to.contain(`-----BEGIN ${label}-----\n`); + expect(pem).to.contain(`-----END ${label}-----\n`); + // A label this parser writes is one it reads back, or the value is good for one trip. + expect(utils.toPem(pem)).to.equal(pem); + }); + } + + it("keeps a hyphen inside a label, which RFC 7468 allows", function () { + expect(utils.toPem(utils.toPem(data, "FOO-BAR"))).to.contain("-----BEGIN FOO-BAR-----"); + }); + + it("refuses a label that would write a boundary into the message", function () { + expect(() => utils.toPem(data, "A-----BEGIN CERTIFICATE-----B")).to.throw( + "Invalid PEM label.", + ); }); + + for (const [problem, label] of [ + ["is empty", ""], + ["opens with a blank", " CERTIFICATE"], + ["closes with a blank", "CERTIFICATE "], + ["holds a line break", "CERT\nIFICATE"], + ] as const) { + it(`refuses a label that ${problem}`, function () { + expect(() => utils.toPem(data, label)).to.throw("Invalid PEM label."); + }); + } }); describe("rejects data that is not base64", function () { @@ -200,29 +247,29 @@ describe("Utils tests", function () { const corrupt = (body: string) => [lines[0], body, lines[lines.length - 1]].join("\n"); it("a body that is not base64 at all", function () { - expect(() => utils.derToPem(corrupt("not base64 at all!"))).to.throw("Unknown DER format."); + expect(() => utils.toPem(corrupt("not base64 at all!"))).to.throw("Invalid PEM format."); }); it("a body that is one long run of blanks", function () { - expect(() => utils.derToPem(corrupt(" ".repeat(80000)))).to.throw("Unknown DER format."); + expect(() => utils.toPem(corrupt(" ".repeat(80000)))).to.throw("Invalid PEM format."); }); it("a body with a blank line in the middle of the data", function () { const body = lines.slice(1, -1); const interrupted = [...body.slice(0, 2), "", ...body.slice(2)].join("\n"); - expect(() => utils.derToPem(corrupt(interrupted))).to.throw("Unknown DER format."); + expect(() => utils.toPem(corrupt(interrupted))).to.throw("Invalid PEM format."); }); it("a message whose two labels disagree", function () { const mismatched = normalizedPem.replace("-----END CERTIFICATE-----", "-----END KEY-----"); - expect(() => utils.derToPem(mismatched)).to.throw("Unknown DER format."); + expect(() => utils.toPem(mismatched)).to.throw("Invalid PEM format."); }); it("a body whose final quantum is incomplete", function () { - expect(() => utils.derToPem(corrupt(`${lines.slice(1, -1).join("\n")}A`))).to.throw( - "Unknown DER format.", + expect(() => utils.toPem(corrupt(`${lines.slice(1, -1).join("\n")}A`))).to.throw( + "Invalid PEM format.", ); }); }); @@ -255,4 +302,41 @@ describe("Utils tests", function () { expect(() => utils.pemToDer(bundle)).to.throw("Expected a single PEM message, but found 3."); }); }); + + describe("pemCertificates", function () { + const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); + + it("returns the base64 of every certificate, and only certificates", function () { + const certificates = utils.pemCertificates(bundle); + + // The bundle carries a private key alongside its two certificates, and publishing that in + // KeyInfo would hand out the signing key: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data + expect(certificates).to.have.lengthOf(2); + for (const certificate of certificates) { + expect(certificate).to.match(/^[A-Za-z0-9+/]+={0,2}$/); + expect(() => + crypto.createPublicKey(utils.toPem(certificate, "CERTIFICATE")), + ).to.not.throw(); + } + }); + + it("returns an empty array when the value holds no message at all", function () { + const data = bundle.split("\n").slice(1, 19).join(""); + + expect(utils.pemCertificates("")).to.deep.equal([]); + expect(utils.pemCertificates(data)).to.deep.equal([]); + }); + + it("throws when the value opens a message it does not close", function () { + expect(() => utils.pemCertificates(bundle.replace(/-----END CERTIFICATE-----/, ""))).to.throw( + "Invalid PEM format.", + ); + }); + + it("throws when a certificate's data is not base64", function () { + const corrupt = bundle.replace(/^[A-Za-z0-9+/]{64}$/m, "not base64 at all!"); + + expect(() => utils.pemCertificates(corrupt)).to.throw("Invalid PEM format."); + }); + }); }); From 71c131a294ddada49b4e4564de1f5d1c1d68c4fa Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 21:44:28 -0500 Subject: [PATCH 04/18] test: cover the KeyInfo paths this branch left untested `codecov/patch` has been short of its 100% target since the first commit on this branch. Three branches went uncovered, all reachable from the public API: - a `publicCert` that is a `KeyObject`, which holds a key and never a certificate, so `KeyInfo` is omitted - a `KeyInfo` with no `X509Certificate` under it - `getCertFromKeyInfo()` with no `KeyInfo` at all The first of those looked like dead code after `Buffer.isBuffer()` converts its argument. It is not: `publicCert` is `string | Buffer | KeyObject`, and the compiler says so. Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 1 + test/signature-unit-tests.spec.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index d17284ae..ac3fc66a 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -228,6 +228,7 @@ export class SignedXml { publicCert = publicCert.toString("latin1"); } + // A KeyObject holds a key and never a certificate, so there is no X509Data to build from it. const certificates = typeof publicCert === "string" ? utils.pemCertificates(publicCert) : []; // X509Data requires at least one child: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index a8a1ee4f..96ec82ca 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1438,6 +1438,34 @@ describe("Signature unit tests", function () { it("when keyInfoAttributes are set without a publicCert", function () { expect(selectKeyInfo({ keyInfoAttributes: { Id: "key" } })).to.be.empty; }); + + it("when publicCert is a KeyObject, which holds a key and never a certificate", function () { + expect(selectKeyInfo({ publicCert: crypto.createPublicKey(privateKey) })).to.be.empty; + }); + }); + + describe("getCertFromKeyInfo", function () { + const parse = (xml: string) => new xmldom.DOMParser().parseFromString(xml, "text/xml"); + + it("returns the certificate a KeyInfo carries, as PEM", function () { + const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const data = normalizedPem.trim().split("\n").slice(1, -1).join(""); + const keyInfo = parse( + `${data}`, + ); + + expect(SignedXml.getCertFromKeyInfo(keyInfo)).to.equal(normalizedPem); + }); + + it("returns null when the KeyInfo carries no X509Certificate", function () { + const keyInfo = parse("client"); + + expect(SignedXml.getCertFromKeyInfo(keyInfo)).to.be.null; + }); + + it("returns null when there is no KeyInfo at all", function () { + expect(SignedXml.getCertFromKeyInfo(null)).to.be.null; + }); }); function signWithPublicCert(publicCert: string) { From 64941cc164135b4b4edffff9ccebedb670b26fd8 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 22:05:03 -0500 Subject: [PATCH 05/18] test: hold the key and degenerate-input guarantees node-saml tested The parser moves here, so the guarantees around it move here too rather than staying in node-saml's `test/crypto.spec.ts`. Everything that suite asserts about parsing now has an owner in this repository: - a private key and an RSA public key normalize from PEM, from bare base64 with a label, and from a body wrapped at a width that is not 64. Nothing drove a key through the parser before; every case was a certificate, and `client.pem` and `saml_external_ns.pem` were already here as fixtures. - a public key OpenSSL exports round-trips, and `pemCertificates()` returns nothing for it, because it holds no certificate. - `pemToDer()` returns the bytes of a message whatever its label, and OpenSSL loads a private key back from them. - an empty string, an empty Buffer, and a value that is only blanks are refused with `Invalid PEM format.` rather than reaching OpenSSL. Not carried over: the cases that belong to an options layer rather than a parser, which are the ones naming an option, and `null`/`false`, which the parameter type forbids. Co-Authored-By: Claude Opus 5 --- test/utils-tests.spec.ts | 41 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index eb56d492..e400ce19 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -241,6 +241,31 @@ describe("Utils tests", function () { } }); + describe("keys, not only certificates", function () { + for (const [name, file, label] of [ + ["a private key", "client.pem", "PRIVATE KEY"], + ["an RSA public key", "saml_external_ns.pem", "RSA PUBLIC KEY"], + ] as const) { + it(`normalizes ${name}, from PEM and from bare base64 alike`, function () { + const pem = fs.readFileSync(`./test/static/${file}`, "latin1"); + const normalized = `${pem.trim()}\n`; + const data = pem.trim().split("\n").slice(1, -1).join(""); + + expect(utils.toPem(pem)).to.equal(normalized); + expect(utils.toPem(data, label)).to.equal(normalized); + expect(utils.toPem(data.match(/.{1,32}/g)?.join("\n") ?? "", label)).to.equal(normalized); + }); + } + + it("round-trips a public key OpenSSL exports, which holds no certificate", function () { + const privateKey = fs.readFileSync("./test/static/client.pem", "latin1"); + const spki = crypto.createPublicKey(privateKey).export({ type: "spki", format: "pem" }); + + expect(utils.toPem(spki.toString())).to.equal(spki); + expect(utils.pemCertificates(spki.toString())).to.be.empty; + }); + }); + describe("rejects data that is not base64", function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); const lines = normalizedPem.trim().split("\n"); @@ -267,6 +292,16 @@ describe("Utils tests", function () { expect(() => utils.toPem(mismatched)).to.throw("Invalid PEM format."); }); + for (const [name, value] of [ + ["an empty string", ""], + ["an empty Buffer", Buffer.alloc(0)], + ["blanks and nothing else", " \n\t "], + ] as const) { + it(name, function () { + expect(() => utils.toPem(value, "CERTIFICATE")).to.throw("Invalid PEM format."); + }); + } + it("a body whose final quantum is incomplete", function () { expect(() => utils.toPem(corrupt(`${lines.slice(1, -1).join("\n")}A`))).to.throw( "Invalid PEM format.", @@ -289,6 +324,12 @@ describe("Utils tests", function () { expect(() => utils.pemToDer("not a pem")).to.throw(); }); + it("returns the bytes of a message of any label, which OpenSSL loads back", function () { + const key = utils.pemToDer(fs.readFileSync("./test/static/client.pem", "latin1")); + + expect(() => crypto.createPrivateKey({ key, format: "der", type: "pkcs8" })).to.not.throw(); + }); + it("will throw if the encapsulated data is not base64", function () { const pem = fs.readFileSync("./test/static/client_public.pem", "latin1").trim().split("\n"); const corrupt = [pem[0], "not base64 at all!", pem[pem.length - 1]].join("\n"); From 8020318089564d30f8c306b004a9bc83baa7c36e Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 22:14:08 -0500 Subject: [PATCH 06/18] fix: check every PEM message before filtering for certificates A message is a certificate by its opening label alone, so one reading `BEGIN PRIVATE KEY` and `END CERTIFICATE` was filtered away before `isWellFormedMessage()` ever saw it. `pemCertificates()` returned `[]`, `getKeyInfoContent()` read that as "no certificate here", and signing went on and omitted the `KeyInfo` the caller asked for. The mirror image, `BEGIN CERTIFICATE` with `END PRIVATE KEY`, threw, so the two directions of the same corruption behaved differently. Every message is now checked before any is filtered. A bundle that holds a well-formed private key alongside its certificates still yields only the certificates. Reported by CodeRabbit on #603. Co-Authored-By: Claude Opus 5 --- src/utils.ts | 15 +++++++++++---- test/signature-unit-tests.spec.ts | 8 ++++++++ test/utils-tests.spec.ts | 13 +++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 7ff71e40..524b17ad 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -107,7 +107,9 @@ export function encodeSpecialCharactersInText(text: string): string { * space allows whitespace, and a pretty-printed document indents it. * https://www.w3.org/TR/xmlschema11-2/#base64Binary * - a label is limited to 48 label characters, which no registered label comes close to, so that - * a boundary cannot be made arbitrarily long. + * a boundary cannot be made arbitrarily long, and it may not be empty, which the 'label' + * production of Figure 1 marks as 'empty ok'. A message labelled nothing names no format, and + * OpenSSL will not read one. * * Structure and data are separate checks, the data taken with its line breaks removed, so that a * line may end anywhere without `{4}` having to become the ambiguous `{1,4}`. Line endings and @@ -243,12 +245,17 @@ export function pemCertificates(pem: string): string[] { return []; } - const certificates = pemMessages(text).filter((message) => message.label === "CERTIFICATE"); - if (!certificates.every(isWellFormedMessage)) { + // Every message is checked before any is filtered: a message is a certificate by its opening + // label alone, so one that opens as something else and closes as a certificate would be + // filtered away unexamined, and signing would go on without the KeyInfo the caller asked for. + const messages = pemMessages(text); + if (!messages.every(isWellFormedMessage)) { throw new Error("Invalid PEM format."); } - return certificates.map((certificate) => certificate.data); + return messages + .filter((message) => message.label === "CERTIFICATE") + .map((certificate) => certificate.data); } /** diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 96ec82ca..ce1340d5 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1492,6 +1492,14 @@ describe("Signature unit tests", function () { expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); }); + it("refuses to sign with a publicCert that opens as a key and closes as a certificate", function () { + const publicCert = fs + .readFileSync("./test/static/client_public.pem", "latin1") + .replace("BEGIN CERTIFICATE", "BEGIN PRIVATE KEY"); + + expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); + }); + it("refuses to sign with a publicCert whose certificate is not base64", function () { const lines = fs.readFileSync("./test/static/client_public.pem", "latin1").trim().split("\n"); const publicCert = [lines[0], "not base64 at all!", lines[lines.length - 1]].join("\n"); diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index e400ce19..48c2296c 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -374,6 +374,19 @@ describe("Utils tests", function () { ); }); + it("throws when a message's labels disagree, whichever of them is a certificate", function () { + const pem = fs.readFileSync("./test/static/client_public.pem", "latin1"); + + // The opening label alone decides what a message is, so a message that opens as something + // else is not a certificate to filter away: it is a certificate that failed to parse. + expect(() => + utils.pemCertificates(pem.replace("BEGIN CERTIFICATE", "BEGIN PRIVATE KEY")), + ).to.throw("Invalid PEM format."); + expect(() => + utils.pemCertificates(pem.replace("END CERTIFICATE", "END PRIVATE KEY")), + ).to.throw("Invalid PEM format."); + }); + it("throws when a certificate's data is not base64", function () { const corrupt = bundle.replace(/^[A-Za-z0-9+/]{64}$/m, "not base64 at all!"); From dca2b4e47ea0eb6a75256460adf04c9d2dd0c087 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 22:21:21 -0500 Subject: [PATCH 07/18] test: pin the empty label on the reading side too The grammar marks `label` as 'empty ok', so refusing one is a deviation and wants a test on both sides. The label a caller supplies was already covered; a message that arrives labelled nothing was not. Co-Authored-By: Claude Opus 5 --- test/utils-tests.spec.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 48c2296c..3c803cb1 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -223,6 +223,14 @@ describe("Utils tests", function () { expect(utils.toPem(utils.toPem(data, "FOO-BAR"))).to.contain("-----BEGIN FOO-BAR-----"); }); + it("refuses a message labelled nothing, which the grammar marks as 'empty ok'", function () { + // A message labelled nothing names no format, and OpenSSL answers it with + // ERR_OSSL_UNSUPPORTED, so reading one would only move the failure later. + expect(() => utils.toPem("-----BEGIN -----\nQUFBQQ==\n-----END -----\n")).to.throw( + "Invalid PEM format.", + ); + }); + it("refuses a label that would write a boundary into the message", function () { expect(() => utils.toPem(data, "A-----BEGIN CERTIFICATE-----B")).to.throw( "Invalid PEM label.", From bef30b08c1c2ac7bc972e3c83e2c321bdc96bc40 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 17 Sep 2026 22:23:11 -0500 Subject: [PATCH 08/18] test: separate the two ways a PEM message is left unclosed Dropping a footer from the middle of a bundle leaves the next header where data should be, which is not the same malformation as a message left open at the end of the value. The test named the second and exercised only the first. Reported by CodeRabbit on #603. Co-Authored-By: Claude Opus 5 --- test/utils-tests.spec.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 3c803cb1..af2d1c65 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -377,9 +377,15 @@ describe("Utils tests", function () { }); it("throws when the value opens a message it does not close", function () { - expect(() => utils.pemCertificates(bundle.replace(/-----END CERTIFICATE-----/, ""))).to.throw( - "Invalid PEM format.", - ); + // Dropping one footer leaves the next header where data should be, which is a different + // malformation from a message left open at the end, so both are worth their own case. + const interleaved = bundle.replace(/-----END CERTIFICATE-----/, ""); + const unclosed = `${bundle}\n-----BEGIN CERTIFICATE-----\nQUFBQQ==\n`; + const empty = `${bundle}\n-----BEGIN CERTIFICATE-----\n`; + + for (const value of [interleaved, unclosed, empty]) { + expect(() => utils.pemCertificates(value)).to.throw("Invalid PEM format."); + } }); it("throws when a message's labels disagree, whichever of them is a certificate", function () { From fc8826b775c32215a37c81a89fe2038504449f96 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 08:26:37 -0500 Subject: [PATCH 09/18] fix: keep a written boundary on one line and read certificates out of text Three defects a reviewer found in the new parser. A label was bounded at 48 label characters, but RFC 7468's `label` puts a separator between them, so a label could reach 95 characters. `-----BEGIN ` and `-----` bracket it in 16, which is why master's `[A-Z\x20]{1,48}` bounded the total: 48 is the longest label whose opening boundary still fits the 64-character line `normalizePem` writes. Past that, `formatPemMessage` wrapped the boundary itself and produced a message the parser could not read back. The bound is now measured over the whole label, on the labels this module writes and the ones it reads, and only the data is handed to `normalizePem`, so the structure is never rewrapped whatever the label. `pemCertificates()` required the whole value to be PEM and threw otherwise, but the `EXTRACT_X509_CERTS` match it replaced found certificates wherever they sat. RFC 7468 section 5.2 shows a certificate written under its subject and issuer lines, and OpenSSL and keytool write them, so a `publicCert` carrying that text signed before and stopped signing here. It reads certificates out of a larger value again, still checking every message it finds, and still refusing an opening boundary no message was built from. Base64 given without boundaries now takes a blank line among its lines. That is the form XMLDSig carries in `X509Certificate`, an `xs:base64Binary` whose lexical space collapses whitespace, which is the same reason the interior blanks were allowed. A message's body is RFC 7468's and has no blank line in it. Co-Authored-By: Claude Opus 5 --- README.md | 27 +++++++---- src/utils.ts | 79 ++++++++++++++++++++----------- test/signature-unit-tests.spec.ts | 28 +++++++++++ test/utils-tests.spec.ts | 63 ++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index d90d4fd3..68cb221f 100644 --- a/README.md +++ b/README.md @@ -578,32 +578,41 @@ one message after another. as a string rather than a Buffer. - `pemToDer(pem)` returns the decoded bytes of the one message a value holds. - `pemCertificates(pem)` returns the base64 of each `CERTIFICATE` message and ignores messages of - any other label, so a private key in the same value is never published. + any other label, so a private key in the same value is never published. It reads certificates + out of a larger value, passing over the explanatory text tools write around a message, while + `toPem()` rewrites the whole value and so must account for all of it. Accepted: - `\n`, `\r\n` and `\r` line endings, and a leading UTF-8 BOM. - any line width, a single line included, and a blank line after the header. -- blanks anywhere in the encapsulated data. XMLDSig carries a certificate as +- blanks anywhere in the encapsulated data, and a blank line as well when the base64 is given + without boundaries. XMLDSig carries a certificate as [`xs:base64Binary`](https://www.w3.org/TR/xmlschema11-2/#base64Binary), whose lexical space - allows whitespace, so a pretty-printed document indents it and a value that has been through a - text field may have had its line endings replaced by spaces. + collapses whitespace, so a pretty-printed document indents it and a value that has been through + a text field may have had its line endings replaced by spaces. +- explanatory text before, after or between the messages, which `pemCertificates()` passes over. + [Section 5.2](https://www.rfc-editor.org/rfc/rfc7468#section-5.2) shows a certificate written + under its subject and issuer lines, and OpenSSL and keytool both write them. - several messages in one value, of which `toPem()` keeps all, `pemCertificates()` takes the certificates, and `pemToDer()` takes none. -- any label RFC 7468's grammar allows, which is every registered label and the ones OpenSSL adds, - such as `RSA PRIVATE KEY`. `PemLabel` names the registered ones. +- any label of up to 48 characters that RFC 7468's grammar allows, which is every registered + label and the ones OpenSSL adds, such as `RSA PRIVATE KEY`. `PemLabel` names the registered + ones, the longest of which is 21 characters. Rejected, with an error rather than a certificate: - data outside the base64 alphabet, padding away from the end, or a final quantum that is not whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). -- a header with no data under it, and a blank line in the middle of the data. +- a header with no data under it, and a blank line in the middle of a message's data, which + RFC 7468's body does not have. - a value that opens a message it does not close, or one whose header and footer labels disagree. [Section 3](https://www.rfc-editor.org/rfc/rfc7468#section-3) permits a parser to disregard the footer's label, but OpenSSL will not read such a message, so neither does this one. - a label outside RFC 7468's grammar, which is one holding `--` or opening or closing with a - blank. A label is written into both boundaries, so one holding `--` would produce a message - this parser could not read back. + blank, and one longer than 48 characters. A label is written into both boundaries, so one + holding `--` would produce a message this parser could not read back, and `-----BEGIN ` and + `-----` bracket it in 16 characters, so 48 is the longest whose boundary still fits one line. ### Converting .pfx certificates to pem diff --git a/src/utils.ts b/src/utils.ts index 524b17ad..15cbfba1 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -103,13 +103,14 @@ export function encodeSpecialCharactersInText(text: string): string { * - whitespace around the message is discarded, the '*W' of 'laxtextualmsg' in Figure 2, and a * leading UTF-8 BOM with it. * - blanks are discarded wherever they fall in the encapsulated data, not only at the ends of - * lines as Figure 1 permits. XMLDSig carries a certificate as xs:base64Binary, whose lexical - * space allows whitespace, and a pretty-printed document indents it. + * lines as Figure 1 permits, and base64 given without boundaries may hold a blank line as + * well. XMLDSig carries a certificate as xs:base64Binary, whose lexical space collapses + * whitespace, and a pretty-printed document indents it. * https://www.w3.org/TR/xmlschema11-2/#base64Binary - * - a label is limited to 48 label characters, which no registered label comes close to, so that - * a boundary cannot be made arbitrarily long, and it may not be empty, which the 'label' - * production of Figure 1 marks as 'empty ok'. A message labelled nothing names no format, and - * OpenSSL will not read one. + * - a label is limited to 48 characters, which no registered label comes close to, so that an + * opening boundary still fits the line this module writes, and it may not be empty, which the + * 'label' production of Figure 1 marks as 'empty ok'. A message labelled nothing names no + * format, and OpenSSL will not read one. * * Structure and data are separate checks, the data taken with its line breaks removed, so that a * line may end anywhere without `{4}` having to become the ambiguous `{1,4}`. Line endings and @@ -129,6 +130,14 @@ export function encodeSpecialCharactersInText(text: string): string { const LABEL_CHAR = "[\\x21-\\x2C\\x2E-\\x7E]"; const LABEL = `${LABEL_CHAR}(?:[-\\x20]?${LABEL_CHAR}){0,47}`; +/* + * `-----BEGIN ` and `-----` bracket a label in 16 characters, so 48 is the longest label whose + * opening boundary still fits the 64-character line `normalizePem` writes. The repetition above + * bounds label characters alone, and the separators standing between them carry a label past 48 + * without exceeding it, so the length is measured rather than left to the pattern. + */ +const LABEL_MAX_LENGTH = 48; + const LABEL_REGEX = new RegExp(`^${LABEL}$`); const PEM_FORMAT_REGEX = new RegExp( `^(?:-----BEGIN ${LABEL}-----\\n+(?:[A-Za-z0-9+/=]+\\n)+-----END ${LABEL}-----\\n*)+$`, @@ -137,7 +146,11 @@ const PEM_MESSAGE_REGEX = new RegExp( `-----BEGIN (${LABEL})-----\\n+((?:[A-Za-z0-9+/=]+\\n)+)-----END (${LABEL})-----`, "g", ); -const BASE64_LINES_REGEX = /^(?:[A-Za-z0-9+/=]+\n)*[A-Za-z0-9+/=]+$/; +// Base64 given without boundaries is what XMLDSig carries in `X509Certificate`, an +// `xs:base64Binary` whose lexical space collapses whitespace, so a blank line among its lines is +// insignificant and is taken here. Inside a message the body is RFC 7468's, which has no blank +// line in it, and `PEM_FORMAT_REGEX` holds that line to its own shape. +const BASE64_TEXT_REGEX = /^[A-Za-z0-9+/=\n]+$/; const BASE64_DATA_REGEX = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; // A Buffer is decoded latin1 to keep every byte, which leaves a UTF-8 BOM as three characters @@ -188,8 +201,21 @@ function isBase64Data(data: string): boolean { return BASE64_DATA_REGEX.test(data); } +function isLabel(label: string): boolean { + return label.length <= LABEL_MAX_LENGTH && LABEL_REGEX.test(label); +} + function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { - return label === endLabel && isBase64Data(data); + return label === endLabel && isLabel(label) && isBase64Data(data); +} + +// Counted rather than matched, so that an opening boundary no message was built from is still +// seen, whatever left it unusable: a missing footer, a label too long to write back, or a body +// that is not base64. The blank of `-----BEGIN ` is left out deliberately, so that a boundary +// sharing its line with other text is counted here and refused, rather than passed over as prose +// and the certificate under it dropped from a signature without a word. +function countOpenings(text: string): number { + return text.split("-----BEGIN").length - 1; } /** @@ -218,14 +244,18 @@ export function normalizePem(pem: string): string { } // Rebuilt from the data rather than passed through, so that the same certificate produces the -// same bytes whatever line width, line ending or blanks it arrived with. +// same bytes whatever line width, line ending or blanks it arrived with. Only the data is handed +// to `normalizePem`, which wraps at 64 characters whatever it is given: a boundary broken across +// two lines would be a message this parser could no longer read back. function formatPemMessage(label: string, data: string): string { - return normalizePem(`-----BEGIN ${label}-----\n${data}\n-----END ${label}-----`); + return `-----BEGIN ${label}-----\n${normalizePem(data)}-----END ${label}-----\n`; } /** * Returns the base64 data of each `CERTIFICATE` message in a PEM value, and `[]` when the value - * holds no certificate: bare base64, or messages of other labels. + * holds no certificate: bare base64, or messages of other labels. Explanatory text before, after + * or between the messages is passed over, as RFC 7468 section 5.2 allows, but every message the + * value does hold is read and checked. * * @param pem The PEM value to read certificates from * @throws Error if the value opens a message it does not close as a well-formed PEM, or if a @@ -233,23 +263,16 @@ function formatPemMessage(label: string, data: string): string { */ export function pemCertificates(pem: string): string[] { const text = normalizePemInput(pem); + const messages = pemMessages(text); - if (!PEM_FORMAT_REGEX.test(text)) { - // A value with no boundaries at all holds no certificate to publish, but one that opens a - // message it cannot finish is a certificate we failed to read, and dropping it would sign - // without the KeyInfo the caller asked for. - if (text.includes("-----BEGIN ")) { - throw new Error("Invalid PEM format."); - } - - return []; - } - - // Every message is checked before any is filtered: a message is a certificate by its opening + // Section 5.2 shows explanatory text before a certificate, and the tools that write one put the + // subject and issuer there, so whatever surrounds a message is passed over rather than refused. + // An opening boundary that no message was built from is another matter: it is a certificate we + // failed to read, and dropping it would sign without the KeyInfo the caller asked for. Every + // message is checked before any is filtered, because a message is a certificate by its opening // label alone, so one that opens as something else and closes as a certificate would be - // filtered away unexamined, and signing would go on without the KeyInfo the caller asked for. - const messages = pemMessages(text); - if (!messages.every(isWellFormedMessage)) { + // filtered away unexamined. + if (countOpenings(text) !== messages.length || !messages.every(isWellFormedMessage)) { throw new Error("Invalid PEM format."); } @@ -316,14 +339,14 @@ export function toPem(value: string | Buffer, pemLabel?: PemLabel): string { const data = text.replace(/\n/g, ""); - if (BASE64_LINES_REGEX.test(text) && isBase64Data(data)) { + if (BASE64_TEXT_REGEX.test(text) && isBase64Data(data)) { if (pemLabel == null) { throw new Error("A PEM label is required to wrap base64 data."); } // The label is written into both boundaries, so one that is not a label would produce a // message this parser could not read back, and `-----` in it would produce a second message. - if (!LABEL_REGEX.test(pemLabel)) { + if (!isLabel(pemLabel)) { throw new Error("Invalid PEM label."); } diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index ce1340d5..e1bcbaf1 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1507,6 +1507,34 @@ describe("Signature unit tests", function () { expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); }); + it("signs with a publicCert carrying the explanatory text tools write around a certificate", function () { + // RFC 7468 section 5.2 shows a certificate written under its subject and issuer lines, and + // OpenSSL writes them, so a value that carries them still carries a certificate to publish. + const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const publicCert = `subject=/CN=client\nissuer=/CN=ca\n${certificate}Issued for testing.\n`; + const sig = new SignedXml({ + privateKey: fs.readFileSync("./test/static/client.pem"), + publicCert, + canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", + signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + }); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + }); + sig.computeSignature(""); + + const doc = new xmldom.DOMParser().parseFromString(sig.getSignedXml()); + const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); + isDomNode.assertIsArrayOfNodes(certificates); + + expect(certificates).to.have.lengthOf(1); + expect(certificates[0].textContent).to.equal( + certificate.trim().split("\n").slice(1, -1).join(""), + ); + }); + it("adds id and type attributes to Reference elements when provided", function () { const xml = ""; const sig = new SignedXml(); diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index af2d1c65..5beb77cd 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -157,6 +157,15 @@ describe("Utils tests", function () { expect(utils.toPem(rebuild(["", ...body]))).to.equal(normalizedPem); }); + it("a blank line in base64 given without boundaries", function () { + // XMLDSig carries a certificate as xs:base64Binary, whose lexical space collapses + // whitespace, so a blank line among the lines of an X509Certificate is insignificant. + // Inside a message the body is RFC 7468's, which has no blank line in it. + const interrupted = [...body.slice(0, 2), "", ...body.slice(2)].join("\n"); + + expect(utils.toPem(interrupted, "CERTIFICATE")).to.equal(normalizedPem); + }); + it("a line width other than 64", function () { const rewrapped = body.join("").match(/.{1,70}/g) ?? []; @@ -223,6 +232,30 @@ describe("Utils tests", function () { expect(utils.toPem(utils.toPem(data, "FOO-BAR"))).to.contain("-----BEGIN FOO-BAR-----"); }); + it("writes the longest label it takes on one line, and reads it back", function () { + // `-----BEGIN ` and `-----` bracket the label in 16 characters, so 48 is the longest one + // whose opening boundary still fits a 64-character line. + const longest = "A".repeat(48); + const pem = utils.toPem(data, longest); + + expect(`-----BEGIN ${longest}-----`).to.have.lengthOf(64); + expect(pem).to.contain(`-----BEGIN ${longest}-----\n`); + expect(pem).to.contain(`-----END ${longest}-----\n`); + expect(utils.toPem(pem)).to.equal(pem); + }); + + it("refuses a label too long for a boundary to carry on one line", function () { + // The separators of RFC 7468's 'label' count toward the length as its label characters + // do: 25 label characters with a space between each pair is 49, one over. + const tooLong = Array(25).fill("A").join(" "); + + expect(tooLong).to.have.lengthOf(49); + expect(() => utils.toPem(data, tooLong)).to.throw("Invalid PEM label."); + expect(() => + utils.toPem(`-----BEGIN ${tooLong}-----\nQUFBQQ==\n-----END ${tooLong}-----\n`), + ).to.throw("Invalid PEM format."); + }); + it("refuses a message labelled nothing, which the grammar marks as 'empty ok'", function () { // A message labelled nothing names no format, and OpenSSL answers it with // ERR_OSSL_UNSUPPORTED, so reading one would only move the failure later. @@ -369,6 +402,36 @@ describe("Utils tests", function () { } }); + describe("passes over the explanatory text around a message", function () { + // RFC 7468 section 5.2 shows a certificate written under its subject and issuer lines, and + // both OpenSSL and keytool put them there, so a value carrying them is still a value + // carrying a certificate. Section 2 allows the data before an encapsulation boundary. + const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const data = certificate.trim().split("\n").slice(1, -1).join(""); + + it("before the message", function () { + const value = `subject=/CN=client\nissuer=/CN=ca\n${certificate}`; + + expect(utils.pemCertificates(value)).to.deep.equal([data]); + }); + + it("after the message", function () { + expect(utils.pemCertificates(`${certificate}Issued for testing.\n`)).to.deep.equal([data]); + }); + + it("between two messages", function () { + const value = `${certificate}and its issuer:\n\n${certificate}`; + + expect(utils.pemCertificates(value)).to.deep.equal([data, data]); + }); + + it("but not an opening boundary no message was built from", function () { + const value = `${certificate}and one more:\n-----BEGIN CERTIFICATE-----\n`; + + expect(() => utils.pemCertificates(value)).to.throw("Invalid PEM format."); + }); + }); + it("returns an empty array when the value holds no message at all", function () { const data = bundle.split("\n").slice(1, 19).join(""); From 7e9c98ed9923fab4df17b604b077092d052b7bff Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 08:55:27 -0500 Subject: [PATCH 10/18] fix: require an encapsulation boundary to have its line to itself A message is now read out of a larger value, so what shares a boundary's line matters. `-----END CERTIFICATE-----suffix` was read as a certificate and the suffix passed over as though it were the explanatory text around the message, and so was a header with text in front of it when that text opened with the dashes that make `normalizePemInput` treat a line as a boundary. Figure 1 gives a boundary a line of its own. Anchoring that in the pattern is what it looks like it should be, but `^` and `$` under `m` make `PEM_MESSAGE_REGEX` exponential, which `recheck` reports and no timing test shows, so the position comes from the match index instead. `PEM_FORMAT_REGEX` lets one message follow another with no line between them, which the line check will not read as two, so `toPem()` counts openings the way `pemCertificates()` does rather than return a value with a certificate missing. The README gains the empty label under Rejected, separates spaces and tabs from blank lines, and says where each is taken. Co-Authored-By: Claude Opus 5 --- README.md | 23 +++++++++++++++-------- src/utils.ts | 30 ++++++++++++++++++++++-------- test/utils-tests.spec.ts | 23 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 68cb221f..0b374f09 100644 --- a/README.md +++ b/README.md @@ -585,30 +585,37 @@ one message after another. Accepted: - `\n`, `\r\n` and `\r` line endings, and a leading UTF-8 BOM. -- any line width, a single line included, and a blank line after the header. -- blanks anywhere in the encapsulated data, and a blank line as well when the base64 is given - without boundaries. XMLDSig carries a certificate as +- any line width, a single line included. +- spaces and tabs anywhere in the encapsulated data. XMLDSig carries a certificate as [`xs:base64Binary`](https://www.w3.org/TR/xmlschema11-2/#base64Binary), whose lexical space collapses whitespace, so a pretty-printed document indents it and a value that has been through a text field may have had its line endings replaced by spaces. +- a blank line after the header, and a blank line anywhere among the lines of base64 given + without boundaries, which is the form an `X509Certificate` element carries. A blank line inside + a message's body is rejected; RFC 7468's body does not have one. - explanatory text before, after or between the messages, which `pemCertificates()` passes over. [Section 5.2](https://www.rfc-editor.org/rfc/rfc7468#section-5.2) shows a certificate written under its subject and issuer lines, and OpenSSL and keytool both write them. - several messages in one value, of which `toPem()` keeps all, `pemCertificates()` takes the certificates, and `pemToDer()` takes none. -- any label of up to 48 characters that RFC 7468's grammar allows, which is every registered - label and the ones OpenSSL adds, such as `RSA PRIVATE KEY`. `PemLabel` names the registered - ones, the longest of which is 21 characters. +- any non-empty label of up to 48 characters that RFC 7468's grammar allows, which is every + registered label and the ones OpenSSL adds, such as `RSA PRIVATE KEY`. `PemLabel` names the + registered ones, the longest of which is 21 characters. Rejected, with an error rather than a certificate: - data outside the base64 alphabet, padding away from the end, or a final quantum that is not whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). -- a header with no data under it, and a blank line in the middle of a message's data, which - RFC 7468's body does not have. +- a header with no data under it, and a blank line in the middle of a message's data. +- a boundary sharing its line with other text. + [Figure 1](https://www.rfc-editor.org/rfc/rfc7468#section-3) gives an encapsulation boundary a + line of its own, so text on that line is not the explanatory text around a message. - a value that opens a message it does not close, or one whose header and footer labels disagree. [Section 3](https://www.rfc-editor.org/rfc/rfc7468#section-3) permits a parser to disregard the footer's label, but OpenSSL will not read such a message, so neither does this one. +- an empty label, which the `label` production marks as `empty ok`. A message labelled nothing + names no format, and OpenSSL answers one with `ERR_OSSL_UNSUPPORTED`, so `-----BEGIN -----` and + `toPem(data, "")` are both refused. - a label outside RFC 7468's grammar, which is one holding `--` or opening or closing with a blank, and one longer than 48 characters. A label is written into both boundaries, so one holding `--` would produce a message this parser could not read back, and `-----BEGIN ` and diff --git a/src/utils.ts b/src/utils.ts index 15cbfba1..4c390d02 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -184,13 +184,24 @@ function pemMessages(pem: string): PemMessage[] { PEM_MESSAGE_REGEX.lastIndex = 0; let message = PEM_MESSAGE_REGEX.exec(pem); while (message !== null) { - messages.push({ - label: message[1], - endLabel: message[3], - // A line break inside base64 is presentation and never data, so where a line ends is a - // question for the structure check alone, and the data is carried de-lined from here on. - data: message[2].replace(/\n/g, ""), - }); + const start = message.index; + const end = start + message[0].length; + + // An encapsulation boundary is a line of its own in Figure 1, and a message is read out of a + // larger value, so text sharing a boundary's line must not be passed over as the explanatory + // text around the message. Anchoring that in the pattern costs it the linearity every pattern + // here has to keep — `^` and `$` under `m` make it exponential, which `recheck` will say and + // a timing test will not — so the position is taken from the match, where it is two tests. + if ((start === 0 || pem[start - 1] === "\n") && (end === pem.length || pem[end] === "\n")) { + messages.push({ + label: message[1], + endLabel: message[3], + // A line break inside base64 is presentation and never data, so where a line ends is a + // question for the structure check alone, and the data is carried de-lined from here on. + data: message[2].replace(/\n/g, ""), + }); + } + message = PEM_MESSAGE_REGEX.exec(pem); } @@ -330,7 +341,10 @@ export function toPem(value: string | Buffer, pemLabel?: PemLabel): string { if (PEM_FORMAT_REGEX.test(text)) { const messages = pemMessages(text); - if (!messages.every(isWellFormedMessage)) { + // `PEM_FORMAT_REGEX` lets one message follow another with no line between them, which the + // line check in `pemMessages` will not read as two. Counting the openings here is what turns + // that into an error rather than a value returned with a certificate quietly missing. + if (countOpenings(text) !== messages.length || !messages.every(isWellFormedMessage)) { throw new Error("Invalid PEM format."); } diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 5beb77cd..018d0cb7 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -327,6 +327,15 @@ describe("Utils tests", function () { expect(() => utils.toPem(corrupt(interrupted))).to.throw("Invalid PEM format."); }); + it("two messages sharing one line", function () { + // Nothing stands between the footer and the next header, so the two boundaries fall on + // one line and neither is a line of its own. Refused rather than read as one message, + // which would return a value with the second certificate quietly missing. + const run = `${normalizedPem.trim()}${normalizedPem.trim()}`; + + expect(() => utils.toPem(run)).to.throw("Invalid PEM format."); + }); + it("a message whose two labels disagree", function () { const mismatched = normalizedPem.replace("-----END CERTIFICATE-----", "-----END KEY-----"); @@ -430,6 +439,20 @@ describe("Utils tests", function () { expect(() => utils.pemCertificates(value)).to.throw("Invalid PEM format."); }); + + for (const [place, value] of [ + ["before the header", `prefix${certificate}`], + // A line opening with `-----` is held to be a boundary and keeps its blanks, so this is + // the shape that reaches the header with text still in front of it. + ["before the header, itself opening with dashes", `-----X${certificate}`], + ["after the footer", `${certificate.trim()}suffix\n`], + ] as const) { + it(`and not text sharing a boundary's line, ${place}`, function () { + // Figure 1 gives an encapsulation boundary a line of its own, so text on that line is + // not the explanatory text around a message and is not passed over as though it were. + expect(() => utils.pemCertificates(value)).to.throw("Invalid PEM format."); + }); + } }); it("returns an empty array when the value holds no message at all", function () { From 8fea545109c777e4534dcde3287179d39443e992 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 12:55:58 -0500 Subject: [PATCH 11/18] fix: refuse an unaccounted opening boundary in pemToDer too `7e9c98e` gave `toPem()` and `pemCertificates()` a count of the opening boundaries, so that one producing no message is an error rather than a value returned with a certificate missing, and left `pemToDer()` without it. Three messages whose second and third run their boundaries together then left one message standing, which is not `> 1`, so the first was decoded and the rest discarded without a word. Reported by @markstos. The count moves into `pemMessages()`, where it holds for every caller and none can omit it. `pemToDer()` still reports `Expected a single PEM message, but found 3.` for a bundle that is well formed, because openings and messages agree there; a value whose boundaries run together is refused as malformed instead, which is what it is. Co-Authored-By: Claude Opus 5 --- src/utils.ts | 44 ++++++++++++++++++++++------------------ test/utils-tests.spec.ts | 11 ++++++++++ 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 4c390d02..ac76df75 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -177,6 +177,15 @@ interface PemMessage { data: string; } +// Counted rather than matched, so that an opening boundary no message was built from is still +// seen, whatever left it unusable: a missing footer, a label too long to write back, or a body +// that is not base64. The blank of `-----BEGIN ` is left out deliberately, so that a boundary +// sharing its line with other text is counted here and refused, rather than passed over as prose +// and the certificate under it dropped from a signature without a word. +function countOpenings(text: string): number { + return text.split("-----BEGIN").length - 1; +} + function pemMessages(pem: string): PemMessage[] { const messages: PemMessage[] = []; @@ -205,6 +214,14 @@ function pemMessages(pem: string): PemMessage[] { message = PEM_MESSAGE_REGEX.exec(pem); } + // Every opening boundary has to have produced a message. One that did not is a message this + // parser could not read — no footer, a label it will not write back, a body that is not base64, + // or a boundary sharing its line — and passing over it would hand back a value with a + // certificate or a key quietly missing from it. Held here so that no caller can omit it. + if (countOpenings(pem) !== messages.length) { + throw new Error("Invalid PEM format."); + } + return messages; } @@ -220,15 +237,6 @@ function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { return label === endLabel && isLabel(label) && isBase64Data(data); } -// Counted rather than matched, so that an opening boundary no message was built from is still -// seen, whatever left it unusable: a missing footer, a label too long to write back, or a body -// that is not base64. The blank of `-----BEGIN ` is left out deliberately, so that a boundary -// sharing its line with other text is counted here and refused, rather than passed over as prose -// and the certificate under it dropped from a signature without a word. -function countOpenings(text: string): number { - return text.split("-----BEGIN").length - 1; -} - /** * -----BEGIN [LABEL]----- * base64([DATA]) @@ -277,13 +285,12 @@ export function pemCertificates(pem: string): string[] { const messages = pemMessages(text); // Section 5.2 shows explanatory text before a certificate, and the tools that write one put the - // subject and issuer there, so whatever surrounds a message is passed over rather than refused. - // An opening boundary that no message was built from is another matter: it is a certificate we - // failed to read, and dropping it would sign without the KeyInfo the caller asked for. Every - // message is checked before any is filtered, because a message is a certificate by its opening - // label alone, so one that opens as something else and closes as a certificate would be - // filtered away unexamined. - if (countOpenings(text) !== messages.length || !messages.every(isWellFormedMessage)) { + // subject and issuer there, so whatever surrounds a message is passed over rather than refused; + // an opening boundary that produced no message is `pemMessages`' to refuse. Every message is + // checked before any is filtered, because a message is a certificate by its opening label + // alone, so one that opens as something else and closes as a certificate would be filtered away + // unexamined, and signing would go on without the KeyInfo the caller asked for. + if (!messages.every(isWellFormedMessage)) { throw new Error("Invalid PEM format."); } @@ -341,10 +348,7 @@ export function toPem(value: string | Buffer, pemLabel?: PemLabel): string { if (PEM_FORMAT_REGEX.test(text)) { const messages = pemMessages(text); - // `PEM_FORMAT_REGEX` lets one message follow another with no line between them, which the - // line check in `pemMessages` will not read as two. Counting the openings here is what turns - // that into an error rather than a value returned with a certificate quietly missing. - if (countOpenings(text) !== messages.length || !messages.every(isWellFormedMessage)) { + if (!messages.every(isWellFormedMessage)) { throw new Error("Invalid PEM format."); } diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 018d0cb7..ec0cb4be 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -392,6 +392,17 @@ describe("Utils tests", function () { expect(() => utils.pemToDer(bundle)).to.throw("Expected a single PEM message, but found 3."); }); + + it("will throw, not return the first, when later boundaries run together", function () { + // A footer and the next header on one line are not two messages, so counting the messages + // alone would find one and hand back its bytes with the rest of the value discarded. + const runTogether = fs + .readFileSync("./test/static/client_bundle.pem", "latin1") + .replace("-----\n-----BEGIN ", "----------BEGIN "); + + expect(runTogether).to.contain("----------BEGIN "); + expect(() => utils.pemToDer(runTogether)).to.throw("Invalid PEM format."); + }); }); describe("pemCertificates", function () { From 096202b6d208e74f64fb82f9732b507a1d74c2f0 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 14:48:46 -0500 Subject: [PATCH 12/18] feat: read certificates with Node's X509Certificate A CERTIFICATE message's data was judged as base64 and nothing more, so well-formed base64 of anything at all was accepted as a certificate: published in KeyInfo when signing, or returned by getCertFromKeyInfo to fail later in OpenSSL. Node's X509Certificate reads the DER as X.509, so the certificate is now judged by OpenSSL, and toPem() writes it back out with Node's serializer rather than this module's. Node reads the first certificate from the bytes it is given and ignores what follows, so the data is required to be that certificate's encoding exactly. A second certificate run into the first would otherwise be dropped silently, the same failure as one message passed over in a value. What the regex parser still owns is what Node does not do: finding every message in a value, which X509Certificate answers by taking the first. The external API is unchanged, and so is the output for every certificate the tests hold, byte for byte. Data under other labels keeps the base64 rules alone: ENCRYPTED PRIVATE KEY cannot be parsed without its passphrase, and CRL, PKCS7 and CMS have no Node parser. Co-Authored-By: Claude Opus 5 --- README.md | 4 +++ src/utils.ts | 37 +++++++++++++++++++++++-- test/signature-unit-tests.spec.ts | 17 ++++++++++++ test/utils-tests.spec.ts | 45 ++++++++++++++++++++++++++----- 4 files changed, 95 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 0b374f09..3c0cb2fe 100644 --- a/README.md +++ b/README.md @@ -606,6 +606,10 @@ Rejected, with an error rather than a certificate: - data outside the base64 alphabet, padding away from the end, or a final quantum that is not whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). +- a `CERTIFICATE` whose data is not exactly one X.509 certificate: base64 of something else, a + certificate cut short, or one with more bytes after it. That data is read by Node's + [`X509Certificate`](https://nodejs.org/api/crypto.html#class-x509certificate), so a certificate + is judged by OpenSSL and not by its base64 alone, and a certificate is written back out by it. - a header with no data under it, and a blank line in the middle of a message's data. - a boundary sharing its line with other text. [Figure 1](https://www.rfc-editor.org/rfc/rfc7468#section-3) gives an encapsulation boundary a diff --git a/src/utils.ts b/src/utils.ts index ac76df75..a38c8048 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,3 +1,4 @@ +import { X509Certificate } from "crypto"; import * as xpath from "xpath"; import type { NamespacePrefix, PemLabel } from "./types"; import * as isDomNode from "@xmldom/is-dom-node"; @@ -233,6 +234,32 @@ function isLabel(label: string): boolean { return label.length <= LABEL_MAX_LENGTH && LABEL_REGEX.test(label); } +/* + * A certificate is handed to Node, whose X509Certificate reads the DER as X.509 and not as base64 + * alone, so data that is well-formed base64 and no certificate is refused here rather than being + * published in KeyInfo or reaching OpenSSL later. What the parser above still owns is what Node + * does not: finding each message in a value, which X509Certificate answers by taking the first + * certificate and passing over the rest. It does the same within one message's data, reading the + * first certificate and ignoring the bytes after it, so the data has to be that certificate's + * encoding exactly, or a second certificate run into the first would be dropped without a word. + * Node's own message depends on the OpenSSL it was built with, so the error is this module's. + */ +function x509Certificate(data: string): X509Certificate { + const der = Buffer.from(data, "base64"); + let certificate: X509Certificate | undefined; + try { + certificate = new X509Certificate(der); + } catch { + // Refused below, with this module's message. + } + + if (certificate == null || !certificate.raw.equals(der)) { + throw new Error("Invalid PEM format."); + } + + return certificate; +} + function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { return label === endLabel && isLabel(label) && isBase64Data(data); } @@ -267,6 +294,10 @@ export function normalizePem(pem: string): string { // to `normalizePem`, which wraps at 64 characters whatever it is given: a boundary broken across // two lines would be a message this parser could no longer read back. function formatPemMessage(label: string, data: string): string { + if (label === "CERTIFICATE") { + return x509Certificate(data).toString(); + } + return `-----BEGIN ${label}-----\n${normalizePem(data)}-----END ${label}-----\n`; } @@ -296,7 +327,7 @@ export function pemCertificates(pem: string): string[] { return messages .filter((message) => message.label === "CERTIFICATE") - .map((certificate) => certificate.data); + .map((certificate) => x509Certificate(certificate.data).raw.toString("base64")); } /** @@ -315,7 +346,9 @@ export function pemToDer(pem: string): Buffer { throw new Error("Invalid PEM format."); } - return Buffer.from(messages[0].data, "base64"); + const [{ label, data }] = messages; + + return label === "CERTIFICATE" ? x509Certificate(data).raw : Buffer.from(data, "base64"); } // A Buffer holds either the bytes of a PEM file or raw DER. DER is ASN.1, whose every encoding diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index e1bcbaf1..d56ad52b 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1457,6 +1457,15 @@ describe("Signature unit tests", function () { expect(SignedXml.getCertFromKeyInfo(keyInfo)).to.equal(normalizedPem); }); + it("throws when the X509Certificate is base64 and no certificate", function () { + const data = Buffer.from("base64, and no certificate").toString("base64"); + const keyInfo = parse( + `${data}`, + ); + + expect(() => SignedXml.getCertFromKeyInfo(keyInfo)).to.throw("Invalid PEM format."); + }); + it("returns null when the KeyInfo carries no X509Certificate", function () { const keyInfo = parse("client"); @@ -1507,6 +1516,14 @@ describe("Signature unit tests", function () { expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); }); + it("refuses to sign with a publicCert whose certificate is base64 and no certificate", function () { + const data = Buffer.from("base64, and no certificate").toString("base64"); + const publicCert = `-----BEGIN CERTIFICATE-----\n${data}\n-----END CERTIFICATE-----\n`; + + // Published in KeyInfo, this would name a certificate no verifier could load. + expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); + }); + it("signs with a publicCert carrying the explanatory text tools write around a certificate", function () { // RFC 7468 section 5.2 shows a certificate written under its subject and issuer lines, and // OpenSSL writes them, so a value that carries them still carries a certificate to publish. diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index ec0cb4be..e12e7e49 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -75,8 +75,9 @@ describe("Utils tests", function () { }); describe("judges the same data with and without encapsulation boundaries", function () { - const wrap = (data: string) => - `-----BEGIN CERTIFICATE-----\n${data}\n-----END CERTIFICATE-----`; + // These are rules for base64, so they are held to a label whose data Node does not parse: + // under CERTIFICATE the data must also be a certificate, which none of these is. + const wrap = (data: string) => `-----BEGIN PKCS7-----\n${data}\n-----END PKCS7-----`; const accepted = { "a whole quantum": "QUJD", @@ -103,15 +104,15 @@ describe("Utils tests", function () { }; Object.entries(accepted).forEach(([description, data]) => { - it(`accepts ${description} either way, and reads the same certificate`, function () { - expect(utils.toPem(wrap(data))).to.equal(utils.toPem(data, "CERTIFICATE")); + it(`accepts ${description} either way, and reads the same data`, function () { + expect(utils.toPem(wrap(data))).to.equal(utils.toPem(data, "PKCS7")); }); }); Object.entries(rejected).forEach(([description, data]) => { it(`rejects ${description} either way, for the same reason`, function () { - expect(() => utils.toPem(wrap(data), "CERTIFICATE")).to.throw("Invalid PEM format."); - expect(() => utils.toPem(data, "CERTIFICATE")).to.throw("Invalid PEM format."); + expect(() => utils.toPem(wrap(data), "PKCS7")).to.throw("Invalid PEM format."); + expect(() => utils.toPem(data, "PKCS7")).to.throw("Invalid PEM format."); }); }); }); @@ -405,6 +406,38 @@ describe("Utils tests", function () { }); }); + describe("reads a CERTIFICATE message's data as X.509, and not only as base64", function () { + // The base64 rules above hold for every label. Under CERTIFICATE the data is handed to Node's + // X509Certificate as well, so data those rules accept is still refused if it is no certificate. + const certificate = fs.readFileSync("./test/static/client_public.der"); + const wrap = (der: Buffer) => + `-----BEGIN CERTIFICATE-----\n${der.toString("base64")}\n-----END CERTIFICATE-----\n`; + + for (const [problem, der] of [ + ["well-formed base64 that is no certificate", Buffer.from("base64, and no certificate")], + ["a certificate cut short", certificate.subarray(0, -1)], + // X509Certificate reads the first certificate in the bytes and ignores what follows. + ["a certificate with a second run into it", Buffer.concat([certificate, certificate])], + ] as const) { + it(`refuses ${problem}, in each function that reads one`, function () { + expect(() => utils.toPem(wrap(der))).to.throw("Invalid PEM format."); + expect(() => utils.toPem(der.toString("base64"), "CERTIFICATE")).to.throw( + "Invalid PEM format.", + ); + expect(() => utils.toPem(der, "CERTIFICATE")).to.throw("Invalid PEM format."); + expect(() => utils.pemToDer(wrap(der))).to.throw("Invalid PEM format."); + expect(() => utils.pemCertificates(wrap(der))).to.throw("Invalid PEM format."); + }); + } + + it("and holds the data of any other label to the base64 rules alone", function () { + const data = Buffer.from("base64, and no certificate"); + const pem = `-----BEGIN PKCS7-----\n${data.toString("base64")}\n-----END PKCS7-----\n`; + + expect(utils.pemToDer(pem)).to.deep.equal(data); + }); + }); + describe("pemCertificates", function () { const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); From e6afe06a7a01e2d8c5189fee64be71f0d3e7f5e8 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 15:01:09 -0500 Subject: [PATCH 13/18] fix: say why a certificate with data after it is refused 6.x passed a certificate with more bytes after it through, and OpenSSL used the first certificate and ignored the rest, so a value like that worked. It is refused now, and "Invalid PEM format." would leave whoever meets it looking at the parser rather than at the value. The refusal names what is wrong with it. Co-Authored-By: Claude Opus 5 --- src/utils.ts | 12 +++++++++- test/signature-unit-tests.spec.ts | 12 ++++++++++ test/utils-tests.spec.ts | 38 +++++++++++++++++++++---------- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index a38c8048..a2991fa2 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -253,7 +253,17 @@ function x509Certificate(data: string): X509Certificate { // Refused below, with this module's message. } - if (certificate == null || !certificate.raw.equals(der)) { + if (certificate == null) { + throw new Error("Invalid PEM format."); + } + + // Refused by name, because 6.x accepted it and OpenSSL used the first certificate: whoever + // meets this has to know that the value, and not the parser, is what changed. + if (certificate.raw.length < der.length) { + throw new Error("Expected a single certificate, but found more data after it."); + } + + if (!certificate.raw.equals(der)) { throw new Error("Invalid PEM format."); } diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index d56ad52b..0de58b49 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1466,6 +1466,18 @@ describe("Signature unit tests", function () { expect(() => SignedXml.getCertFromKeyInfo(keyInfo)).to.throw("Invalid PEM format."); }); + it("says why when one X509Certificate carries two certificates", function () { + const certificate = fs.readFileSync("./test/static/client_public.der"); + const data = Buffer.concat([certificate, certificate]).toString("base64"); + const keyInfo = parse( + `${data}`, + ); + + expect(() => SignedXml.getCertFromKeyInfo(keyInfo)).to.throw( + "Expected a single certificate, but found more data after it.", + ); + }); + it("returns null when the KeyInfo carries no X509Certificate", function () { const keyInfo = parse("client"); diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index e12e7e49..c155bd87 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -413,20 +413,34 @@ describe("Utils tests", function () { const wrap = (der: Buffer) => `-----BEGIN CERTIFICATE-----\n${der.toString("base64")}\n-----END CERTIFICATE-----\n`; - for (const [problem, der] of [ - ["well-formed base64 that is no certificate", Buffer.from("base64, and no certificate")], - ["a certificate cut short", certificate.subarray(0, -1)], - // X509Certificate reads the first certificate in the bytes and ignores what follows. - ["a certificate with a second run into it", Buffer.concat([certificate, certificate])], + const moreAfter = "Expected a single certificate, but found more data after it."; + + for (const [problem, der, error] of [ + [ + "well-formed base64 that is no certificate", + Buffer.from("base64, and no certificate"), + "Invalid PEM format.", + ], + ["a certificate cut short", certificate.subarray(0, -1), "Invalid PEM format."], + // X509Certificate reads the first certificate in the bytes and ignores what follows, which + // 6.x passed on for OpenSSL to do the same, so these are refused with a reason of their own. + [ + "a certificate with a second run into it", + Buffer.concat([certificate, certificate]), + moreAfter, + ], + [ + "a certificate with other bytes after it", + Buffer.concat([certificate, Buffer.from("more")]), + moreAfter, + ], ] as const) { it(`refuses ${problem}, in each function that reads one`, function () { - expect(() => utils.toPem(wrap(der))).to.throw("Invalid PEM format."); - expect(() => utils.toPem(der.toString("base64"), "CERTIFICATE")).to.throw( - "Invalid PEM format.", - ); - expect(() => utils.toPem(der, "CERTIFICATE")).to.throw("Invalid PEM format."); - expect(() => utils.pemToDer(wrap(der))).to.throw("Invalid PEM format."); - expect(() => utils.pemCertificates(wrap(der))).to.throw("Invalid PEM format."); + expect(() => utils.toPem(wrap(der))).to.throw(error); + expect(() => utils.toPem(der.toString("base64"), "CERTIFICATE")).to.throw(error); + expect(() => utils.toPem(der, "CERTIFICATE")).to.throw(error); + expect(() => utils.pemToDer(wrap(der))).to.throw(error); + expect(() => utils.pemCertificates(wrap(der))).to.throw(error); }); } From 8446c7268d65ecf2983efbd2bacd96b23e596283 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 15:08:01 -0500 Subject: [PATCH 14/18] fix: name a BER-encoded certificate for what it is 096202b required a certificate's data to equal Node's `.raw` exactly, which refuses a certificate encoded as BER rather than DER: OpenSSL reads one and re-encodes it, so `.raw` differs. e6afe06 then took any `.raw` shorter than the input to mean data after the certificate, and a BER length written in its longer form is shorter once re-encoded, so a BER certificate was reported as "found more data after it" with nothing after it at all. Data after a certificate leaves `.raw` as a prefix of the input, and a re-encoding does not, so that is what now tells the two apart. A BER certificate is refused with a message of its own. X.509 requires DER, and 6.x accepted BER for OpenSSL to read, so this is a refusal whoever meets it has to be told the reason for. Co-Authored-By: Claude Opus 5 --- src/utils.ts | 16 +++++++++------- test/utils-tests.spec.ts | 7 +++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index a2991fa2..ccc48eb7 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -257,17 +257,19 @@ function x509Certificate(data: string): X509Certificate { throw new Error("Invalid PEM format."); } - // Refused by name, because 6.x accepted it and OpenSSL used the first certificate: whoever - // meets this has to know that the value, and not the parser, is what changed. - if (certificate.raw.length < der.length) { - throw new Error("Expected a single certificate, but found more data after it."); + if (certificate.raw.equals(der)) { + return certificate; } - if (!certificate.raw.equals(der)) { - throw new Error("Invalid PEM format."); + // Each refusal below is named, because 6.x accepted both and OpenSSL read a certificate from + // them, so whoever meets one has to know that the value, and not the parser, is what changed. + // `.raw` is a prefix of the input only when the bytes after the first certificate are the + // difference; OpenSSL re-encodes one that is BER rather than DER, which X.509 requires. + if (der.subarray(0, certificate.raw.length).equals(certificate.raw)) { + throw new Error("Expected a single certificate, but found more data after it."); } - return certificate; + throw new Error("Expected a DER-encoded certificate."); } function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index c155bd87..9242bf40 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -434,6 +434,13 @@ describe("Utils tests", function () { Buffer.concat([certificate, Buffer.from("more")]), moreAfter, ], + [ + // The fixture's outer length is `82 01C4`. Written `83 0001C4` it is the same length in a + // longer form, which BER allows and DER forbids, and OpenSSL reads it and re-encodes it. + "a certificate encoded as BER rather than DER", + Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), certificate.subarray(2)]), + "Expected a DER-encoded certificate.", + ], ] as const) { it(`refuses ${problem}, in each function that reads one`, function () { expect(() => utils.toPem(wrap(der))).to.throw(error); From e09487b6f35ba5efa659419ac55ef4ddc9d14c3c Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 15:15:13 -0500 Subject: [PATCH 15/18] docs: credit Node for what it documents, not OpenSSL xml-crypto depends on Node's crypto API and not on OpenSSL, which Node wraps in a version that varies by release and build, and which Electron replaces with BoringSSL. The comments and README credited OpenSSL for what X509Certificate does. Node documents that `.raw` is the DER encoding of the certificate, which is all the accept-or-refuse decision rests on; how it treats bytes after a certificate, or BER, is observed behavior, and chooses only the message. The README's other mentions of OpenSSL stay. They cite it as the tooling that writes and reads PEM in practice, to explain where this parser departs from RFC 7468, and do not describe what Node does. Co-Authored-By: Claude Opus 5 --- README.md | 7 ++++--- src/utils.ts | 21 +++++++++++---------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 3c0cb2fe..bb798fcf 100644 --- a/README.md +++ b/README.md @@ -81,7 +81,7 @@ The `enveloped-signature` transform removes only the `Signature` element being v A certificate whose encapsulated data is not base64 is rejected rather than decoded as far as it goes. `Buffer.from(value, "base64")` discards what it does not recognize, so a corrupt certificate -used to reach `KeyInfo`, or OpenSSL, as whatever bytes survived that. What the parser accepts is +used to reach `KeyInfo`, or Node's crypto, as whatever bytes survived that. What the parser accepts is described under [X.509 / Key formats](#x509--key-formats). The error for a value the parser cannot read is `Invalid PEM format.`, in place of the @@ -607,9 +607,10 @@ Rejected, with an error rather than a certificate: - data outside the base64 alphabet, padding away from the end, or a final quantum that is not whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). - a `CERTIFICATE` whose data is not exactly one X.509 certificate: base64 of something else, a - certificate cut short, or one with more bytes after it. That data is read by Node's + certificate cut short, one with more bytes after it, or one encoded as BER rather than DER. That + data is read by Node's [`X509Certificate`](https://nodejs.org/api/crypto.html#class-x509certificate), so a certificate - is judged by OpenSSL and not by its base64 alone, and a certificate is written back out by it. + is judged as X.509 and not by its base64 alone, and Node writes it back out. - a header with no data under it, and a blank line in the middle of a message's data. - a boundary sharing its line with other text. [Figure 1](https://www.rfc-editor.org/rfc/rfc7468#section-3) gives an encapsulation boundary a diff --git a/src/utils.ts b/src/utils.ts index ccc48eb7..db1167ad 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -237,12 +237,11 @@ function isLabel(label: string): boolean { /* * A certificate is handed to Node, whose X509Certificate reads the DER as X.509 and not as base64 * alone, so data that is well-formed base64 and no certificate is refused here rather than being - * published in KeyInfo or reaching OpenSSL later. What the parser above still owns is what Node - * does not: finding each message in a value, which X509Certificate answers by taking the first - * certificate and passing over the rest. It does the same within one message's data, reading the - * first certificate and ignoring the bytes after it, so the data has to be that certificate's - * encoding exactly, or a second certificate run into the first would be dropped without a word. - * Node's own message depends on the OpenSSL it was built with, so the error is this module's. + * published in KeyInfo or failing later in Node's crypto. What the parser above still owns is what + * Node does not: finding each message in a value, which X509Certificate answers by taking the first + * certificate and passing over the rest. Node documents `.raw` as the DER encoding of the + * certificate, so the data is accepted only when it is exactly that. Node's own message depends + * on the crypto library it was built with, so the error is this module's. */ function x509Certificate(data: string): X509Certificate { const der = Buffer.from(data, "base64"); @@ -261,10 +260,12 @@ function x509Certificate(data: string): X509Certificate { return certificate; } - // Each refusal below is named, because 6.x accepted both and OpenSSL read a certificate from - // them, so whoever meets one has to know that the value, and not the parser, is what changed. - // `.raw` is a prefix of the input only when the bytes after the first certificate are the - // difference; OpenSSL re-encodes one that is BER rather than DER, which X.509 requires. + // Each refusal below is named, because 6.x accepted both and Node read a certificate from them, + // so whoever meets one has to know that the value, and not the parser, is what changed. Which + // one it is comes from how Node behaves rather than from anything it documents: `.raw` has been + // a prefix of the input when bytes follow the certificate, and a re-encoding of it when the + // certificate is BER rather than the DER that X.509 requires. A crypto library that refused + // either outright would reach `Invalid PEM format.` above, and the value would still be refused. if (der.subarray(0, certificate.raw.length).equals(certificate.raw)) { throw new Error("Expected a single certificate, but found more data after it."); } From e1e9d89a6b0c763a74ac1def1083ce705f6449ee Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 15:42:43 -0500 Subject: [PATCH 16/18] fix: accept BER certificates and keep their octets as given 096202b refused a certificate encoded as BER rather than DER, on the premise that X.509 requires DER, and had toPem() write certificates back out with Node's serializer. The standards this library implements say otherwise: RFC 7468 section 5.1 says a CERTIFICATE's data MUST be BER, DER strongly preferred, and XML Signature 1.1 says an implementation SHOULD NOT alter or re-encode a certificate, which could invalidate its signature. A BER publicCert could no longer be signed into KeyInfo, and a BER X509Certificate could no longer get through getCertFromKeyInfo, both after Node had read it as a certificate. Reported in review. Node now validates and does nothing more. The octets are kept as given in toPem(), pemToDer() and pemCertificates(), so KeyInfo carries the certificate the caller supplied. Comparing `.raw` with the input can no longer find data after a certificate, because a BER certificate's `.raw` is a re-encoding of it. Instead the data is read again without its last byte: exactly one certificate, whether DER or either form of BER, is then cut short and Node refuses it, while a certificate with anything after it is still whole. No ASN.1 is parsed here, and a crypto library that behaved otherwise could only make this refuse, never accept. PemLabel's comment gains the empty and 48-character exceptions, and pemToDer()'s JSDoc stops calling its argument a certificate. Both reported in review. Co-Authored-By: Claude Opus 5 --- README.md | 11 ++++-- src/types.ts | 4 +- src/utils.ts | 65 +++++++++++++++++-------------- test/signature-unit-tests.spec.ts | 42 ++++++++++++++++++++ test/utils-tests.spec.ts | 39 ++++++++++++++++--- 5 files changed, 119 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index bb798fcf..05b55ead 100644 --- a/README.md +++ b/README.md @@ -601,16 +601,19 @@ Accepted: - any non-empty label of up to 48 characters that RFC 7468's grammar allows, which is every registered label and the ones OpenSSL adds, such as `RSA PRIVATE KEY`. `PemLabel` names the registered ones, the longest of which is 21 characters. +- a certificate encoded as BER as well as DER, as + [RFC 7468 section 5.1](https://www.rfc-editor.org/rfc/rfc7468#section-5.1) allows. Its octets are + kept as given, because [XML Signature 1.1](https://www.w3.org/TR/xmldsig-core1/#sec-X509Data) + says an implementation SHOULD NOT alter or re-encode a certificate. Rejected, with an error rather than a certificate: - data outside the base64 alphabet, padding away from the end, or a final quantum that is not whole, per [RFC 4648 section 4](https://www.rfc-editor.org/rfc/rfc4648#section-4). - a `CERTIFICATE` whose data is not exactly one X.509 certificate: base64 of something else, a - certificate cut short, one with more bytes after it, or one encoded as BER rather than DER. That - data is read by Node's - [`X509Certificate`](https://nodejs.org/api/crypto.html#class-x509certificate), so a certificate - is judged as X.509 and not by its base64 alone, and Node writes it back out. + certificate cut short, or one with more bytes after it. Node's + [`X509Certificate`](https://nodejs.org/api/crypto.html#class-x509certificate) decides whether + the data is a certificate, so it is judged as X.509 and not by its base64 alone. - a header with no data under it, and a blank line in the middle of a message's data. - a boundary sharing its line with other text. [Figure 1](https://www.rfc-editor.org/rfc/rfc7468#section-3) gives an encapsulation boundary a diff --git a/src/types.ts b/src/types.ts index 2db9d08c..05ce63c9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -28,8 +28,8 @@ export type HashAlgorithmType = /** * The label carried by both boundaries of a PEM message. The values listed are the ones RFC 7468 - * defines; any label its grammar allows is accepted, because OpenSSL and the wider ecosystem use - * others, such as `RSA PRIVATE KEY`. + * defines; any other label its grammar allows is accepted too, when it is not empty and not over + * 48 characters, because OpenSSL and the wider ecosystem use others, such as `RSA PRIVATE KEY`. * * @see https://www.rfc-editor.org/rfc/rfc7468 */ diff --git a/src/utils.ts b/src/utils.ts index db1167ad..06abe07f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -235,42 +235,39 @@ function isLabel(label: string): boolean { } /* - * A certificate is handed to Node, whose X509Certificate reads the DER as X.509 and not as base64 + * A certificate is handed to Node, whose X509Certificate reads it as X.509 and not as base64 * alone, so data that is well-formed base64 and no certificate is refused here rather than being - * published in KeyInfo or failing later in Node's crypto. What the parser above still owns is what - * Node does not: finding each message in a value, which X509Certificate answers by taking the first - * certificate and passing over the rest. Node documents `.raw` as the DER encoding of the - * certificate, so the data is accepted only when it is exactly that. Node's own message depends - * on the crypto library it was built with, so the error is this module's. + * published in KeyInfo or failing later in Node's crypto. Node validates and does nothing more: + * the octets are kept as given, because RFC 7468 section 5.1 allows BER, and XML Signature 1.1 + * says an implementation SHOULD NOT alter or re-encode a certificate, which could invalidate it. + * https://www.w3.org/TR/xmldsig-core1/#sec-X509Data + * What the parser above still owns is what Node does not do: finding each message in a value, + * which X509Certificate answers by taking the first certificate and passing over the rest. */ -function x509Certificate(data: string): X509Certificate { - const der = Buffer.from(data, "base64"); - let certificate: X509Certificate | undefined; +function isX509Certificate(bytes: Buffer): boolean { try { - certificate = new X509Certificate(der); + return new X509Certificate(bytes) instanceof X509Certificate; } catch { - // Refused below, with this module's message. + return false; } +} - if (certificate == null) { - throw new Error("Invalid PEM format."); - } +function assertX509Certificate(data: string): void { + const bytes = Buffer.from(data, "base64"); - if (certificate.raw.equals(der)) { - return certificate; + if (!isX509Certificate(bytes)) { + throw new Error("Invalid PEM format."); } - // Each refusal below is named, because 6.x accepted both and Node read a certificate from them, - // so whoever meets one has to know that the value, and not the parser, is what changed. Which - // one it is comes from how Node behaves rather than from anything it documents: `.raw` has been - // a prefix of the input when bytes follow the certificate, and a re-encoding of it when the - // certificate is BER rather than the DER that X.509 requires. A crypto library that refused - // either outright would reach `Invalid PEM format.` above, and the value would still be refused. - if (der.subarray(0, certificate.raw.length).equals(certificate.raw)) { + // Node reads a certificate from the start of the bytes and passes over what follows it, which + // 6.x did the same with. The data is exactly one certificate when dropping its last byte leaves + // bytes Node cannot read, and that holds for DER and both forms of BER length alike, so no ASN.1 + // is parsed here. A crypto library that did not pass over what follows would refuse such data + // above, and one that read a certificate cut short would refuse every certificate here: either + // way what goes wrong is a refusal, never an acceptance. + if (isX509Certificate(bytes.subarray(0, -1))) { throw new Error("Expected a single certificate, but found more data after it."); } - - throw new Error("Expected a DER-encoded certificate."); } function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { @@ -308,7 +305,7 @@ export function normalizePem(pem: string): string { // two lines would be a message this parser could no longer read back. function formatPemMessage(label: string, data: string): string { if (label === "CERTIFICATE") { - return x509Certificate(data).toString(); + assertX509Certificate(data); } return `-----BEGIN ${label}-----\n${normalizePem(data)}-----END ${label}-----\n`; @@ -338,13 +335,18 @@ export function pemCertificates(pem: string): string[] { throw new Error("Invalid PEM format."); } - return messages + const certificates = messages .filter((message) => message.label === "CERTIFICATE") - .map((certificate) => x509Certificate(certificate.data).raw.toString("base64")); + .map((certificate) => certificate.data); + certificates.forEach(assertX509Certificate); + + return certificates; } /** - * @param pem The PEM-encoded base64 certificate to strip headers from + * Returns the decoded bytes of the one PEM message a value holds, whatever its label. + * + * @param pem The PEM message to decode * @throws Error if the value is not a single well-formed PEM message */ export function pemToDer(pem: string): Buffer { @@ -360,8 +362,11 @@ export function pemToDer(pem: string): Buffer { } const [{ label, data }] = messages; + if (label === "CERTIFICATE") { + assertX509Certificate(data); + } - return label === "CERTIFICATE" ? x509Certificate(data).raw : Buffer.from(data, "base64"); + return Buffer.from(data, "base64"); } // A Buffer holds either the bytes of a PEM file or raw DER. DER is ASN.1, whose every encoding diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 0de58b49..15822df2 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1478,6 +1478,21 @@ describe("Signature unit tests", function () { ); }); + it("returns a BER certificate with its octets as given", function () { + const der = fs.readFileSync("./test/static/client_public.der"); + const ber = Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), der.subarray(2)]); + const keyInfo = parse( + `${ber.toString("base64")}`, + ); + const pem = SignedXml.getCertFromKeyInfo(keyInfo); + + expect(pem).to.be.a("string"); + expect(crypto.createPublicKey(pem as string).asymmetricKeyType).to.equal("rsa"); + expect( + Buffer.from((pem as string).split("\n").slice(1, -2).join(""), "base64"), + ).to.deep.equal(ber); + }); + it("returns null when the KeyInfo carries no X509Certificate", function () { const keyInfo = parse("client"); @@ -1536,6 +1551,33 @@ describe("Signature unit tests", function () { expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); }); + it("signs a BER certificate into KeyInfo with its octets as given", function () { + // RFC 7468 section 5.1 allows BER, and XML Signature 1.1 says an implementation SHOULD NOT + // re-encode a certificate: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data + const der = fs.readFileSync("./test/static/client_public.der"); + const ber = Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), der.subarray(2)]); + const publicCert = `-----BEGIN CERTIFICATE-----\n${ber.toString("base64")}\n-----END CERTIFICATE-----\n`; + const sig = new SignedXml({ + privateKey: fs.readFileSync("./test/static/client.pem"), + publicCert, + canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", + signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + }); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + }); + sig.computeSignature(""); + + const doc = new xmldom.DOMParser().parseFromString(sig.getSignedXml()); + const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); + isDomNode.assertIsArrayOfNodes(certificates); + + expect(certificates).to.have.lengthOf(1); + expect(Buffer.from(certificates[0].textContent ?? "", "base64")).to.deep.equal(ber); + }); + it("signs with a publicCert carrying the explanatory text tools write around a certificate", function () { // RFC 7468 section 5.2 shows a certificate written under its subject and issuer lines, and // OpenSSL writes them, so a value that carries them still carries a certificate to publish. diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 9242bf40..f97ba645 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -415,6 +415,30 @@ describe("Utils tests", function () { const moreAfter = "Expected a single certificate, but found more data after it."; + // The fixture's outer length is `82 01C4`. Written `83 0001C4` it is the same length in a + // longer form, and written `80` with two zero octets after the contents it is indefinite: both + // are BER and not DER, which RFC 7468 section 5.1 and XML Signature 1.1 both allow. + const longForm = Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), certificate.subarray(2)]); + const indefinite = Buffer.concat([ + Buffer.from([0x30, 0x80]), + certificate.subarray(4), + Buffer.from([0, 0]), + ]); + + for (const [encoding, der] of [ + ["DER", certificate], + ["BER with a long-form length", longForm], + ["BER with an indefinite length", indefinite], + ] as const) { + it(`accepts a certificate encoded as ${encoding}, and keeps its octets as given`, function () { + // XML Signature 1.1 says an implementation SHOULD NOT alter or re-encode a certificate. + expect(utils.pemToDer(utils.toPem(wrap(der)))).to.deep.equal(der); + expect(utils.pemToDer(utils.toPem(der, "CERTIFICATE"))).to.deep.equal(der); + expect(utils.pemToDer(wrap(der))).to.deep.equal(der); + expect(utils.pemCertificates(wrap(der))).to.deep.equal([der.toString("base64")]); + }); + } + for (const [problem, der, error] of [ [ "well-formed base64 that is no certificate", @@ -423,7 +447,7 @@ describe("Utils tests", function () { ], ["a certificate cut short", certificate.subarray(0, -1), "Invalid PEM format."], // X509Certificate reads the first certificate in the bytes and ignores what follows, which - // 6.x passed on for OpenSSL to do the same, so these are refused with a reason of their own. + // 6.x passed on for Node to do the same, so these are refused with a reason of their own. [ "a certificate with a second run into it", Buffer.concat([certificate, certificate]), @@ -435,11 +459,14 @@ describe("Utils tests", function () { moreAfter, ], [ - // The fixture's outer length is `82 01C4`. Written `83 0001C4` it is the same length in a - // longer form, which BER allows and DER forbids, and OpenSSL reads it and re-encodes it. - "a certificate encoded as BER rather than DER", - Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), certificate.subarray(2)]), - "Expected a DER-encoded certificate.", + "a BER certificate with bytes after it", + Buffer.concat([longForm, Buffer.from("more")]), + moreAfter, + ], + [ + "an indefinite-length certificate with bytes after it", + Buffer.concat([indefinite, certificate]), + moreAfter, ], ] as const) { it(`refuses ${problem}, in each function that reads one`, function () { From b9b8b77ea722f49b9ea6c6a4db4b32008d3a3ea8 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 15:57:29 -0500 Subject: [PATCH 17/18] refactor: narrow the X509Certificate node to the element it is getCertFromKeyInfo() selected `.//*[local-name(.)='X509Certificate']` and narrowed the result to any node, whose textContent TypeScript types as `string | null`, so it read `cert.textContent ?? ""`. The XPath selects elements only, and an element's textContent is always a string, so the fallback could never run: it was the one partially covered changed line in codecov/patch, carried over from master's derToPem() call. Narrowing to an element says what the XPath already guarantees and drops the fallback, with the same behavior for every document. isX509Certificate() also constructs the certificate as a plain statement rather than testing it with instanceof, which success already implies. Both from review. Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 4 ++-- src/utils.ts | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index ac3fc66a..66902e71 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -253,8 +253,8 @@ export class SignedXml { static getCertFromKeyInfo(keyInfo?: Node | null): string | null { if (keyInfo != null) { const cert = xpath.select1(".//*[local-name(.)='X509Certificate']", keyInfo); - if (isDomNode.isNodeLike(cert)) { - return utils.toPem(cert.textContent ?? "", "CERTIFICATE"); + if (isDomNode.isElementNode(cert)) { + return utils.toPem(cert.textContent, "CERTIFICATE"); } } diff --git a/src/utils.ts b/src/utils.ts index 06abe07f..aa559d2c 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -246,7 +246,8 @@ function isLabel(label: string): boolean { */ function isX509Certificate(bytes: Buffer): boolean { try { - return new X509Certificate(bytes) instanceof X509Certificate; + new X509Certificate(bytes); + return true; } catch { return false; } From 27dca84770e1f8c70bfa2e72dca5fc748b8eb210 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Fri, 18 Sep 2026 16:17:44 -0500 Subject: [PATCH 18/18] fix: read bundles holding an encrypted key, and write canonical base64 Two regressions from review, both reproduced against master. A publicCert bundling a certificate with a traditional encrypted private key, as Node exports one with `type: "pkcs1"` and a cipher, signed on master and threw `Invalid PEM format.` here. The key's `Proc-Type` and `DEK-Info` header fields are RFC 1421's, which RFC 7468 does not permit, so the key's opening boundary produced no message and pemMessages() refused the value. Those two fields are now read. Letting them be skipped instead would have loosened the rule that every opening boundary produces a message, which is what keeps a certificate from being dropped. A certificate with header fields is refused, and toPem() and pemToDer() refuse a message with them, as master's derToPem() did, since its data is lost without them. The fields' values are held to what OpenSSL writes. A value that could hold the `-----BEGIN ` of a boundary would let one message's fields run on over every message after it, which recheck reports as polynomial. e1e9d89 kept certificate octets as given by copying the base64 text through, so data whose pad bits were not zero reached X509Certificate as text outside xs:base64Binary's lexical space, which a schema check refuses. Master decoded and re-encoded it. Base64 is now written as the base64 of its octets: the octets are unchanged, as XML Signature 1.1 asks, and the text is canonical. Co-Authored-By: Claude Opus 5 --- README.md | 6 +++ src/utils.ts | 46 ++++++++++++++---- test/signature-unit-tests.spec.ts | 79 ++++++++++++++++--------------- test/utils-tests.spec.ts | 53 +++++++++++++++++++++ 4 files changed, 137 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 05b55ead..e6650ef4 100644 --- a/README.md +++ b/README.md @@ -605,6 +605,12 @@ Accepted: [RFC 7468 section 5.1](https://www.rfc-editor.org/rfc/rfc7468#section-5.1) allows. Its octets are kept as given, because [XML Signature 1.1](https://www.w3.org/TR/xmldsig-core1/#sec-X509Data) says an implementation SHOULD NOT alter or re-encode a certificate. +- base64 whose pad bits are not zero, which is written back out with them zeroed, since only that + form is in [`xs:base64Binary`](https://www.w3.org/TR/xmlschema11-2/#base64Binary)'s lexical space. + The octets are the same either way. +- the `Proc-Type` and `DEK-Info` header fields of a traditional encrypted private key, as OpenSSL + and Node write one, so that `pemCertificates()` can read certificates out of a bundle holding + such a key. `toPem()` and `pemToDer()` refuse the key itself, whose data is lost without them. Rejected, with an error rather than a certificate: diff --git a/src/utils.ts b/src/utils.ts index aa559d2c..64412736 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -112,6 +112,10 @@ export function encodeSpecialCharactersInText(text: string): string { * opening boundary still fits the line this module writes, and it may not be empty, which the * 'label' production of Figure 1 marks as 'empty ok'. A message labelled nothing names no * format, and OpenSSL will not read one. + * - the header fields of RFC 1421 section 4.4 are read, which section 2 does not permit, but only + * the `Proc-Type` and `DEK-Info` that OpenSSL writes into a traditional encrypted private key, + * as Node exports one, so that a bundle may carry such a key beside its certificates. A message + * with them is not a certificate, and is neither rewritten nor decoded. * * Structure and data are separate checks, the data taken with its line breaks removed, so that a * line may end anywhere without `{4}` having to become the ambiguous `{1,4}`. Line endings and @@ -140,11 +144,17 @@ const LABEL = `${LABEL_CHAR}(?:[-\\x20]?${LABEL_CHAR}){0,47}`; const LABEL_MAX_LENGTH = 48; const LABEL_REGEX = new RegExp(`^${LABEL}$`); +// A traditional encrypted private key opens with `Proc-Type` and `DEK-Info` fields and a blank +// line. Their values are held to what OpenSSL writes, blanks already removed, and so can never hold +// the `-----BEGIN ` of a boundary: a value that could would let the fields of one message run on +// over every message after it, which `recheck` reports as polynomial. +const HEADERS = "(?:(?:Proc-Type|DEK-Info):[A-Za-z0-9,-]+\\n)+\\n+"; + const PEM_FORMAT_REGEX = new RegExp( - `^(?:-----BEGIN ${LABEL}-----\\n+(?:[A-Za-z0-9+/=]+\\n)+-----END ${LABEL}-----\\n*)+$`, + `^(?:-----BEGIN ${LABEL}-----\\n+(?:${HEADERS})?(?:[A-Za-z0-9+/=]+\\n)+-----END ${LABEL}-----\\n*)+$`, ); const PEM_MESSAGE_REGEX = new RegExp( - `-----BEGIN (${LABEL})-----\\n+((?:[A-Za-z0-9+/=]+\\n)+)-----END (${LABEL})-----`, + `-----BEGIN (${LABEL})-----\\n+(${HEADERS})?((?:[A-Za-z0-9+/=]+\\n)+)-----END (${LABEL})-----`, "g", ); // Base64 given without boundaries is what XMLDSig carries in `X509Certificate`, an @@ -175,6 +185,7 @@ function normalizePemInput(text: string): string { interface PemMessage { label: string; endLabel: string; + headers: boolean; data: string; } @@ -205,10 +216,11 @@ function pemMessages(pem: string): PemMessage[] { if ((start === 0 || pem[start - 1] === "\n") && (end === pem.length || pem[end] === "\n")) { messages.push({ label: message[1], - endLabel: message[3], + endLabel: message[4], + headers: message[2] != null, // A line break inside base64 is presentation and never data, so where a line ends is a // question for the structure check alone, and the data is carried de-lined from here on. - data: message[2].replace(/\n/g, ""), + data: message[3].replace(/\n/g, ""), }); } @@ -271,8 +283,18 @@ function assertX509Certificate(data: string): void { } } -function isWellFormedMessage({ label, endLabel, data }: PemMessage): boolean { - return label === endLabel && isLabel(label) && isBase64Data(data); +function isWellFormedMessage({ label, endLabel, headers, data }: PemMessage): boolean { + // Header fields are what a traditional encrypted key carries, and a certificate never has them. + const certificateWithHeaders = headers && label === "CERTIFICATE"; + + return label === endLabel && isLabel(label) && isBase64Data(data) && !certificateWithHeaders; +} + +// Base64 decodes to the same octets whatever its pad bits hold, but xs:base64Binary allows only +// zeros there, so data is written as the base64 of its octets and not as the text it arrived as. +// The octets themselves are untouched. https://www.w3.org/TR/xmlschema11-2/#base64Binary +function canonicalBase64(data: string): string { + return Buffer.from(data, "base64").toString("base64"); } /** @@ -309,7 +331,7 @@ function formatPemMessage(label: string, data: string): string { assertX509Certificate(data); } - return `-----BEGIN ${label}-----\n${normalizePem(data)}-----END ${label}-----\n`; + return `-----BEGIN ${label}-----\n${normalizePem(canonicalBase64(data))}-----END ${label}-----\n`; } /** @@ -341,7 +363,7 @@ export function pemCertificates(pem: string): string[] { .map((certificate) => certificate.data); certificates.forEach(assertX509Certificate); - return certificates; + return certificates.map(canonicalBase64); } /** @@ -358,7 +380,9 @@ export function pemToDer(pem: string): Buffer { throw new Error(`Expected a single PEM message, but found ${messages.length}.`); } - if (messages.length === 0 || !isWellFormedMessage(messages[0])) { + // A message with header fields holds a key encrypted under the cipher they name, and its bytes + // are of no use without them. + if (messages.length === 0 || !isWellFormedMessage(messages[0]) || messages[0].headers) { throw new Error("Invalid PEM format."); } @@ -400,7 +424,9 @@ export function toPem(value: string | Buffer, pemLabel?: PemLabel): string { if (PEM_FORMAT_REGEX.test(text)) { const messages = pemMessages(text); - if (!messages.every(isWellFormedMessage)) { + // A message with header fields is read so that a bundle can carry one, but not rewritten: its + // data is a key encrypted under the cipher they name, and written out without them it is lost. + if (!messages.every(isWellFormedMessage) || messages.some((message) => message.headers)) { throw new Error("Invalid PEM format."); } diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 15822df2..4367660b 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1517,7 +1517,20 @@ describe("Signature unit tests", function () { transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], }); - return () => sig.computeSignature(""); + return () => { + sig.computeSignature(""); + + return sig.getSignedXml(); + }; + } + + // The text of each X509Certificate that signing with this publicCert puts into KeyInfo. + function publishedCertificates(publicCert: string): string[] { + const doc = new xmldom.DOMParser().parseFromString(signWithPublicCert(publicCert)()); + const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); + isDomNode.assertIsArrayOfNodes(certificates); + + return certificates.map((certificate) => certificate.textContent ?? ""); } it("refuses to sign with a publicCert whose two labels disagree", function () { @@ -1557,25 +1570,34 @@ describe("Signature unit tests", function () { const der = fs.readFileSync("./test/static/client_public.der"); const ber = Buffer.concat([Buffer.from([0x30, 0x83, 0x00]), der.subarray(2)]); const publicCert = `-----BEGIN CERTIFICATE-----\n${ber.toString("base64")}\n-----END CERTIFICATE-----\n`; - const sig = new SignedXml({ - privateKey: fs.readFileSync("./test/static/client.pem"), - publicCert, - canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", - signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", - }); - sig.addReference({ - xpath: "//*[local-name(.)='x']", - digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], - }); - sig.computeSignature(""); - const doc = new xmldom.DOMParser().parseFromString(sig.getSignedXml()); - const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); - isDomNode.assertIsArrayOfNodes(certificates); + expect(publishedCertificates(publicCert)).to.deep.equal([ber.toString("base64")]); + }); - expect(certificates).to.have.lengthOf(1); - expect(Buffer.from(certificates[0].textContent ?? "", "base64")).to.deep.equal(ber); + it("publishes a certificate's base64 with its pad bits zeroed", function () { + // `w` and `x` differ only in bits past the last octet, so both decode to the same certificate, + // but xs:base64Binary allows only zeros there, and KeyInfo is XML that a schema may check. + // https://www.w3.org/TR/xmlschema11-2/#base64Binary + const pem = fs.readFileSync("./test/static/feide_public.pem", "latin1"); + const data = pem.trim().split("\n").slice(1, -1).join(""); + + expect(data).to.match(/4PF13w==$/); + expect(publishedCertificates(pem.replace("4PF13w==", "4PF13x=="))).to.deep.equal([data]); + }); + + it("signs with a publicCert bundling a certificate and a traditional encrypted key", function () { + // Node exports a PKCS#1 key encrypted the traditional way, with `Proc-Type` and `DEK-Info` + // header fields before its data, and a bundle may carry one beside its certificate. + const encrypted = crypto + .createPrivateKey(fs.readFileSync("./test/static/client.pem")) + .export({ type: "pkcs1", format: "pem", cipher: "aes-256-cbc", passphrase: "secret" }) + .toString(); + const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); + + expect(encrypted).to.contain("Proc-Type: 4,ENCRYPTED"); + expect(publishedCertificates(`${certificate}${encrypted}`)).to.deep.equal([ + certificate.trim().split("\n").slice(1, -1).join(""), + ]); }); it("signs with a publicCert carrying the explanatory text tools write around a certificate", function () { @@ -1583,27 +1605,10 @@ describe("Signature unit tests", function () { // OpenSSL writes them, so a value that carries them still carries a certificate to publish. const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); const publicCert = `subject=/CN=client\nissuer=/CN=ca\n${certificate}Issued for testing.\n`; - const sig = new SignedXml({ - privateKey: fs.readFileSync("./test/static/client.pem"), - publicCert, - canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", - signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", - }); - sig.addReference({ - xpath: "//*[local-name(.)='x']", - digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], - }); - sig.computeSignature(""); - const doc = new xmldom.DOMParser().parseFromString(sig.getSignedXml()); - const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); - isDomNode.assertIsArrayOfNodes(certificates); - - expect(certificates).to.have.lengthOf(1); - expect(certificates[0].textContent).to.equal( + expect(publishedCertificates(publicCert)).to.deep.equal([ certificate.trim().split("\n").slice(1, -1).join(""), - ); + ]); }); it("adds id and type attributes to Reference elements when provided", function () { diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index f97ba645..8da5baf6 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -173,6 +173,14 @@ describe("Utils tests", function () { expect(utils.toPem(rebuild(rewrapped))).to.equal(normalizedPem); }); + it("nonzero pad bits, written back as zeros", function () { + // `w` and `x` differ only in bits past the last octet, so both decode alike, but only the + // zeros are in xs:base64Binary's lexical space. + const feide = fs.readFileSync("./test/static/feide_public.pem", "latin1"); + + expect(utils.toPem(feide.replace("4PF13w==", "4PF13x=="))).to.equal(feide); + }); + it("a UTF-8 BOM, as decoded text", function () { expect(utils.toPem(`\uFEFF${normalizedPem}`)).to.equal(normalizedPem); }); @@ -308,6 +316,21 @@ describe("Utils tests", function () { }); }); + describe("a traditional encrypted key", function () { + // Node exports a PKCS#1 key encrypted the traditional way, with `Proc-Type` and `DEK-Info` + // header fields naming the cipher before the data they make readable. + const encrypted = crypto + .createPrivateKey(fs.readFileSync("./test/static/client.pem")) + .export({ type: "pkcs1", format: "pem", cipher: "aes-256-cbc", passphrase: "secret" }) + .toString(); + + it("is neither rewritten nor decoded, since its data is lost without its fields", function () { + expect(encrypted).to.contain("DEK-Info: AES-256-CBC,"); + expect(() => utils.toPem(encrypted)).to.throw("Invalid PEM format."); + expect(() => utils.pemToDer(encrypted)).to.throw("Invalid PEM format."); + }); + }); + describe("rejects data that is not base64", function () { const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); const lines = normalizedPem.trim().split("\n"); @@ -547,6 +570,36 @@ describe("Utils tests", function () { } }); + it("reads certificates out of a bundle that also holds a traditional encrypted key", function () { + const encrypted = crypto + .createPrivateKey(fs.readFileSync("./test/static/client.pem")) + .export({ type: "pkcs1", format: "pem", cipher: "aes-256-cbc", passphrase: "secret" }) + .toString(); + const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); + + expect(utils.pemCertificates(`${certificate}${encrypted}`)).to.deep.equal([ + certificate.trim().split("\n").slice(1, -1).join(""), + ]); + }); + + it("refuses a certificate with header fields, which only an encrypted key carries", function () { + const certificate = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const withFields = certificate.replace( + "-----\n", + "-----\nProc-Type: 4,ENCRYPTED\nDEK-Info: AES-256-CBC,00\n\n", + ); + + expect(() => utils.pemCertificates(withFields)).to.throw("Invalid PEM format."); + }); + + it("returns base64 with its pad bits zeroed", function () { + const feide = fs.readFileSync("./test/static/feide_public.pem", "latin1"); + + expect(utils.pemCertificates(feide.replace("4PF13w==", "4PF13x=="))).to.deep.equal([ + feide.trim().split("\n").slice(1, -1).join(""), + ]); + }); + it("returns an empty array when the value holds no message at all", function () { const data = bundle.split("\n").slice(1, 19).join("");