Skip to content

fix: retry certificate generation on serial numbers OpenSSL rejects - #2

Merged
metcoder95 merged 1 commit into
metcoder95:mainfrom
ViniciusDev26:fix/illegal-padding-serial-number
Sep 14, 2026
Merged

metcoder95 merged 1 commit into
metcoder95:mainfrom
ViniciusDev26:fix/illegal-padding-serial-number

Conversation

@ViniciusDev26

Copy link
Copy Markdown

Follow-up to your review on nodejs/undici#5800 — moving the fix here instead of keeping the workaround in undici's test suite.

The bug

selfsigned derives the certificate serial number from 9 random bytes and runs them through its own toPositiveHex():

cert.serialNumber = toPositiveHex(forge.util.bytesToHex(forge.random.getBytesSync(9)))

function toPositiveHex (hexString) {
  var msb = parseInt(hexString[0], 16);
  if (msb < 8) return hexString;
  msb -= 8;
  return msb.toString() + hexString.substring(1);
}

That makes the integer positive, but it does not re-minimise it, and DER requires minimal encoding. When the 9 random bytes start with 0x80 (or 0x00) followed by 0x00, the serial keeps two redundant leading zero bytes.

node-forge would normally strip those, except it only strips one, and says so in lib/asn1.js:

// ensure integer is minimally-encoded
// TODO: should all leading bytes be stripped vs just one?
// .. ex '00 00 01' => '01'?

So 00 00 01 … becomes 00 01 …, still non-minimal. A positive INTEGER with a leading 0x00 followed by a byte under 0x80 is illegal padding, and OpenSSL rejects it:

serial = 000001020304050607
DER    = 02080001020304050607

Error: error:068000DD:asn1 encoding routines::illegal padding
  code: 'ERR_OSSL_ASN1_ILLEGAL_PADDING'

It takes two or more zero bytes followed by a byte under 0x80, which needs b0 ∈ {0x00, 0x80}, b1 == 0x00 and b2 < 0x80: roughly 1 in 65536 certificates.

The reason it gets out the door is that selfsigned runs verifyCertificateChain() before returning and it passes — node-forge's own parser accepts the non-minimal encoding it just produced. The certificate only fails once OpenSSL reads it, from the middle of a TLS handshake, which is why it showed up as an unexplained flake in undici (nodejs/undici#5245) rather than as a generation error.

What this changes

New generate.js wraps selfsigned: it validates the certificate with X509Certificate and generates again if OpenSSL refuses it. Each attempt draws an independent serial number, so three attempts bring it down to about 1 in 2.8e14, and it throws rather than ever returning an unusable pair.

Both entry points go through it:

  • index.jsgenerate(), async path, unchanged public API.
  • install.js — the pair written on install, sync path. Same odds applied there, and it is the copy 13 undici test files build their server straight from.

selfsigned is still required lazily, so require('@metcoder95/https-pem') stays as cheap as it was.

Tests

tests/index.js stubs forge.random.getBytesSync with the pathological seed, so the failure is deterministic instead of 1 in 65536. Four of the new tests fail on main and pass here; the node-forge devDependency is for that stub only.

The first test asserts the upstream behaviour and reports a diagnostic instead of failing if selfsigned ever stops emitting these, so it will say when generate.js can be deleted.

Upstream

The real fix belongs in selfsigned, emitting a minimally encoded serial:

const bytes = Buffer.from(forge.random.getBytesSync(9), 'binary')
bytes[0] &= 0x7f
if (bytes[0] === 0) bytes[0] = 0x01
cert.serialNumber = bytes.toString('hex')

Finishing the node-forge TODO so it strips every redundant leading byte would fix the whole class of it. Happy to open either of those too — this is the workaround in the meantime.

`selfsigned` derives the serial number from 9 random bytes and runs them
through its own `toPositiveHex()`, which clears the sign bit but does not
re-minimise the resulting DER INTEGER. node-forge's encoder strips only one
of the redundant leading zero bytes, so roughly 1 in 65536 certificates goes
out with a positive INTEGER carrying illegal padding.

node-forge parses that back fine, so the `verifyCertificateChain()` check
`selfsigned` runs before returning passes and the pair looks valid. OpenSSL
refuses it, so it only fails later as ERR_OSSL_ASN1_ILLEGAL_PADDING from the
middle of a TLS handshake.

Every serial number is drawn independently, so `generate()` now validates the
certificate with `X509Certificate` and generates again when OpenSSL will not
load it. Three attempts bring the odds down to about 1 in 2.8e14, and it
throws rather than ever returning an unusable pair. `install.js` goes through
the same path, so the pair written on install cannot be a broken one either.

Refs: nodejs/undici#5245
@metcoder95
metcoder95 merged commit 6d42161 into metcoder95:main Sep 14, 2026
11 checks passed
ViniciusDev26 added a commit to ViniciusDev26/undici that referenced this pull request Sep 14, 2026
Fixes nodejs#5245.

`selfsigned` builds the certificate serial number from 9 random bytes and
runs them through its own `toPositiveHex()`, which clears the sign bit but
does not re-minimise the resulting DER INTEGER. When those bytes start with
0x80 (or 0x00) followed by 0x00, the serial keeps two redundant leading zero
bytes. node-forge strips only one of them -- the "should all leading bytes be
stripped vs just one?" TODO in its asn1.js -- so a positive INTEGER with a
leading 0x00 followed by a byte under 0x80 reaches OpenSSL, which rejects it
with ERR_OSSL_ASN1_ILLEGAL_PADDING.

It happens for roughly 1 in 65536 certificates, and `selfsigned`'s own
`verifyCertificateChain()` does not catch it because node-forge's parser
accepts the non-minimal encoding. The certificate only fails once OpenSSL
reads it, so every test that builds a TLS/HTTP2 server from a freshly
generated pair was flaky, most visibly test/http2-abort.js.

The fix landed upstream in metcoder95/https-pem#2: the package now validates
each certificate with X509Certificate and generates again when OpenSSL
refuses it, for both the async generator and the pair written by the
postinstall hook. Nothing is needed on undici's side beyond the bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants