`toPem()` checks certificate data against its label but not private-key data

[cjbarth]

Summary

toPem() checks certificate data but not private-key data. The data of a CERTIFICATE message goes to Node's X509Certificate, and the value is refused unless it holds exactly one certificate (formatPemMessage()). There is no matching check for keys. Given base64 and "PRIVATE KEY", toPem() writes that label around whatever the data is and returns it. If the data is a PKCS #1 RSAPrivateKey, a public key, or not a key at all, the caller gets a PEM that Node can't read, with no error. The failure shows up later, inside Node's crypto, as DECODER routines::unsupported. pemToDer() likewise returns the bytes of such a message.

PKCS #1 is the case that matters in practice. Keys written as RSA PRIVATE KEY are common: OpenSSL 1.x's genrsa wrote them, and so do many keys already deployed. Strip the boundaries to store such a key on one line and nothing in the base64 says which structure it is. A caller that labels it PRIVATE KEY, which is RFC 7468's label for a private key, gets back a key that can't be used.

This came up in node-saml/node-saml#445, where the reporter's key is PKCS #1. node-saml/node-saml#447 works around it by parsing the key in node-saml. By the division of labour set out in #603, that puts the format check in the wrong library: xml-crypto owns "what a PEM message is, what base64 is, what a label may be", and node-saml owns "which label that option must carry".

Reproduction

Save as repro.js in the repo root and run npm run build && node repro.js. Tested at aa2cc0c.

const crypto = require("crypto");
const { toPem, pemToDer } = require("./lib");

const { privateKey, publicKey } = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 });
const pkcs1 = privateKey.export({ type: "pkcs1", format: "der" });
const pkcs8 = privateKey.export({ type: "pkcs8", format: "der" });
const spki = publicKey.export({ type: "spki", format: "der" });
const wrap = (label, der) =>
  `-----BEGIN ${label}-----\n${der.toString("base64")}\n-----END ${label}-----\n`;

const cases = [
  ["PKCS #8 base64, PRIVATE KEY", pkcs8.toString("base64"), "PRIVATE KEY"],
  ["PKCS #1 base64, RSA PRIVATE KEY", pkcs1.toString("base64"), "RSA PRIVATE KEY"],
  ["PKCS #1 base64, PRIVATE KEY", pkcs1.toString("base64"), "PRIVATE KEY"],
  ["PKCS #8 base64, RSA PRIVATE KEY", pkcs8.toString("base64"), "RSA PRIVATE KEY"],
  ["PKCS #1 DER Buffer, PRIVATE KEY", pkcs1, "PRIVATE KEY"],
  ["PKCS #1 under a PRIVATE KEY header", wrap("PRIVATE KEY", pkcs1)],
  ["public key base64, PRIVATE KEY", spki.toString("base64"), "PRIVATE KEY"],
  ["random base64, PRIVATE KEY", crypto.randomBytes(48).toString("base64"), "PRIVATE KEY"],
  ["random base64, CERTIFICATE", crypto.randomBytes(48).toString("base64"), "CERTIFICATE"],
];

for (const [name, value, label] of cases) {
  let pem;
  try {
    pem = toPem(value, label);
  } catch (error) {
    console.log(`${name.padEnd(36)} toPem() throws: ${error.message}`);
    continue;
  }
  try {
    crypto.createPrivateKey(pem);
    console.log(`${name.padEnd(36)} returned; Node reads it`);
  } catch (error) {
    console.log(`${name.padEnd(36)} returned; Node refuses it: ${error.message}`);
  }
}

try {
  pemToDer(wrap("PRIVATE KEY", pkcs1));
  console.log(`${"pemToDer(), PKCS #1 under PRIVATE KEY".padEnd(36)} returns bytes`);
} catch (error) {
  console.log(`pemToDer() throws: ${error.message}`);
}

Output, identical on Node 18.20.8 and 26.9.0:

PKCS #8 base64, PRIVATE KEY          returned; Node reads it
PKCS #1 base64, RSA PRIVATE KEY      returned; Node reads it
PKCS #1 base64, PRIVATE KEY          returned; Node refuses it: error:1E08010C:DECODER routines::unsupported
PKCS #8 base64, RSA PRIVATE KEY      returned; Node reads it
PKCS #1 DER Buffer, PRIVATE KEY      returned; Node refuses it: error:1E08010C:DECODER routines::unsupported
PKCS #1 under a PRIVATE KEY header   returned; Node refuses it: error:1E08010C:DECODER routines::unsupported
public key base64, PRIVATE KEY       returned; Node refuses it: error:1E08010C:DECODER routines::unsupported
random base64, PRIVATE KEY           returned; Node refuses it: error:1E08010C:DECODER routines::unsupported
random base64, CERTIFICATE           toPem() throws: Invalid PEM format.
pemToDer(), PKCS #1 under PRIVATE KEY returns bytes

The last toPem() row is the contrast: the same random bytes are refused when the label is CERTIFICATE.

Proposal

Check private-key data the way certificate data is already checked. In formatPemMessage() and pemToDer(), pass the data of a PRIVATE KEY or RSA PRIVATE KEY message to crypto.createPrivateKey() under that label, and throw Invalid PEM format. if Node can't read it.

• It can ship in a minor release. Every value it refuses is one that toPem() returns today and Node then rejects, the same kind of case feat: own the PEM parser and validate the encapsulated text #603 started refusing in 6.3.0.
• Refuse rather than relabel. The label stays the caller's choice, as it already is for base64. A caller that accepts either structure, such as node-saml's privateKey, tries PRIVATE KEY and then RSA PRIVATE KEY, and this check tells it which one fits.
• Use Node's reading, not a stricter structural one. OpenSSL reads PKCS question #8 data under RSA PRIVATE KEY (the fourth row above), and it does so even from DER typed pkcs1. So a check through Node accepts that mislabel. Refusing it would need an ASN.1 check of our own and would refuse input that works today. I'm not proposing that.
• Scope to decide: the same reasoning covers EC PRIVATE KEY, PUBLIC KEY and RSA PUBLIC KEY, which Node also reads. The rule could be "every label Node can parse" rather than the two private-key labels. ENCRYPTED PRIVATE KEY keeps the base64 rules alone, since Node can't read it without the passphrase, as feat: own the PEM parser and validate the encapsulated text #603 notes.

A separate question: text around a message in toPem()

The README treats this as settled. pemCertificates() passes over explanatory text, "while toPem() rewrites the whole value and so must account for all of it." I'd like to revisit it for one case, though the answer may well still be no.

node-saml has always handed decryptionPvk to Node as given, and Node reads past text around the key, such as the Bag Attributes lines that openssl pkcs12 -nocerts writes. Routing decryptionPvk through toPem() would refuse keys that decrypt today, so node-saml/node-saml#447 has two paths: PEM goes to Node untouched, and only bare base64 goes through toPem(). As a result, a bad PEM decryptionPvk either fails inside Node's crypto without naming the option, or node-saml has to parse the key itself. If toPem() passed over text outside its messages, as pemCertificates() does, the second path would go away and decryptionPvk would be read exactly as privateKey is.

The cost is that toPem() would drop that text without saying so. The text carries no key material: RFC 7468 section 5.2 calls it explanatory, and section 2 permits data before the boundary. Still, the change loosens toPem() for every caller. That includes node-saml's privateKey and idpCert, whose documentation currently says such text is refused. If the answer is no, node-saml keeps its pass-through, and the proposal above goes ahead regardless.