Repository navigation
feat(pe): inspect Authenticode certificate tables - #4830
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Medium impact] The PR appears safe to merge, with a non-blocking correction needed for older certificate name encodings. Findings
Fix with agent prompt### Issue 1
src/native/pe/PeCertificateDer.ts:227-229
Names stored as `TeletexString` (`0x14`) fall through to Latin-1 here. For example, the valid T.61 bytes `c2 65` become `Âe` instead of `é`. Inspection succeeds, but the returned issuer and subject names contain the wrong text. This is a non-blocking concern for older certificates. Decode this encoding correctly, or reject unsupported name encodings instead of silently treating them as Latin-1.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
CLI["CLI: resolve selected path"] --> Service["Check input"]
MCP["MCP: absolute selected path"] --> Service
Service --> Reader["Read bounded, stable regular file"]
Reader --> PE["Read PE headers and certificate table"]
PE --> DER["Decode type-2 PKCS#7 within limits"]
DER --> Evidence["Return ranges, hashes, and identity fields"]
Reviews (7) · Last reviewed commit: "fix(merge): preserve PE certificate and ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed all five Greptile findings in e68166d, each with a regression test:
|
There was a problem hiding this comment.
TL;DR: I checked the two open Greptile threads against 7eef463. One still happens. A very long OID makes the cert parser stall for about 20 seconds. The other does not: a truncated PKCS#7 payload is rejected.
Edit: I retested this PR at head e1e756c on Node 22, 24 and 26; the results are here.
I ran named test files and small local probes. I did not run any real signed program or a live server.
Findings and verdicts
Head 7eef463. "Reproduces" means a probe I wrote shows the problem. "Does not reproduce" means the same probe now gives the safe answer.
| # | Thread | Sev | Verdict | What I saw |
|---|---|---|---|---|
| 6 | Oversized OIDs freeze requests | P1 | Reproduces | A type-2 entry with a 262,144-byte OID inside a 262,688-byte PE took 19,930 ms in parsePeCertificates with the default limits. A normal 9-byte OID took 0 ms |
| 2 | Truncated PKCS#7 succeeds | P1 | Does not reproduce | SEQUENCE { INTEGER 1, SET {} } is rejected ("missing encapContentInfo"). A SignerInfo with no signature field is rejected ("missing its signature OCTET STRING") |
How the stall grows. Each row is one run of parsePeCertificates on a PE with one type-2 entry. The OID is that many bytes of 0xff ending in 0x7f.
| OID bytes | Time |
|---|---|
| 16,384 | 19 ms |
| 32,768 | 63 ms |
| 65,536 | 234 ms |
| 131,072 | 1,001 ms |
| 262,144 | 19,930 ms |
The parser decodes the contentType OID before it checks for signedData. The loop keeps growing one BigInt, and the DER node budget does not limit one OID's length. The 262,688-byte file is far below the 64 MiB default file limit. A cap on OID length and arc size before the loop would stop it.
Two notes on the table. The OID needs non-zero high bits to grow: bytes of 0x80 stay at zero and finish instantly. The box was shared with other test jobs, and I ran each size once. Read the 19.9 seconds as one run, not a benchmark.
Two other shapes of thread 2 are still accepted. One is an empty OCTET STRING signature (04 00). The other is an empty signerInfos SET {}. Neither is the exact case in the thread. The tool says it does not verify signatures, so I did not count them as a hit. Read "removing the signature bytes" literally and the 04 00 shape is a partial match.
More on the truncated PKCS#7 thread
Three shapes are rejected. One is the bot's SEQUENCE { INTEGER 1, SET {} }. One has no signerInfos. One has a SignerInfo with no signature field. A full SignerInfo is accepted, as it should be.
Test results, 8 test files this PR touches:
| Node | Result |
|---|---|
| 24.18.0 | 8 files, 35 tests passed |
Not run
- A live MCP or CLI server stall. I called the parser function only, so I did not see a server freeze.
- No real signed Windows apps. Every input was built by script.
- Node 22 and 26. I used Node 24 only.
- The four resolved threads. I only noted them as resolved.
- Main and the PR base. Main has no copy of this feature, so it has no fix either.
- Whether a maintainer already commented on the PR.
Thanks, mvanhorn, for the work on this.
Test setup
Commands
npm --prefix <tree> ci --ignore-scripts
npm --prefix <tree> run build:cached
npm --prefix <tree> run test:prepare
npm --prefix <tree> run test:focused -- <8 files> (Node 24)
node <probe>.mjs (imports dist/native/pe/PeCertificateDer.js and PeCertificateParser.js from the built tree)
The 8 files: src/native/pe/PeCertificateParser.test.ts, src/domain/native/peCertificates.test.ts, src/application/binaryDiagnostics/PeCertificatesService.test.ts, src/contracts/toolEffects.test.ts, tests/acceptance/peCertificatesCli.test.ts, tests/acceptance/tools/localToolContracts.test.ts, tests/boundary/filesystem/peCertificates.test.ts, tests/boundary/mcp/peCertificatesMcp.test.ts.
None of them mentions doctor; I checked with grep first.
Probe for the oversized OID, in the shape of the DER bytes:
ContentInfo ::= SEQUENCE {
OBJECT IDENTIFIER, content = (N - 1) bytes of 0xff, then 0x7f
[0] SEQUENCE { INTEGER 1 } }
The payload sits in a WIN_CERTIFICATE entry of type 2 in a minimal PE32 image, and parsePeCertificates runs with the schema defaults (max_file_bytes 67,108,864; max_entries 8).
🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊
|
Thanks for checking. c653f48 caps OBJECT IDENTIFIER length at 256 bytes and each arc at 32 bytes before decodeOid runs. The 262,144-byte OID case now fails with "OBJECT IDENTIFIER exceeds 256 bytes" in under a millisecond, and there is a regression test for it. I left the truncated PKCS#7 path as is since it did not reproduce. I also merged current main; format, lint, typecheck, build and the focused PE certificate tests pass locally. |
|
TL;DR: I retested e1e756c. My oversized-OID finding is fixed: every size now fails in under 2 ms. Two of the three open bot threads still reproduce. One huge certificate name uses about 16 times its size in heap. A 4-byte zero table is reported complete. The old truncated-PKCS#7 thread is still not reproduced. Edit: this comment first said I was retesting. The results are below. I only ran files I made and named test files. I did not run any real signed program or a live server. "Reproduces" means a probe I wrote shows the problem in the thread. My earlier findings
For the OID, every size I tried before (16,384 up to 262,144 bytes) now fails in 0 to 1 ms. A normal OID still parses. A 256-byte OID is decoded, a 257-byte one is rejected, and a real 2.25 UUID-style arc still decodes. The 3 open bot threads
Thread 3 has two shapes the code still accepts: an empty Heap numbers for thread 1Heap still held after the parse and a forced garbage collection, one subject name, Node 24.18.0.
The thread says an 8 MiB BMPString raised heap by about 128 MiB. I measured the same. The 56 MiB file is under the 64 MiB default file limit. 8 MiB BMP on Node 22 and 26 also gave 128 MiB. Each number is one run, on a box that may have been shared. I tested one name in one certificate. Several large names would add up. I did not test that. Tests
At 7eef463 the same files had 35 tests. The new one covers the oversized OID. Nothing covers the other two open threads. The OID, PKCS#7 and table probes gave identical answers on all three Node versions. Not run
Thanks, mvanhorn, for the quick OID fix. Test setupCommandsThe 8 files: src/application/binaryDiagnostics/PeCertificatesService.test.ts, src/contracts/toolEffects.test.ts, src/domain/native/peCertificates.test.ts, src/native/pe/PeCertificateParser.test.ts, tests/acceptance/peCertificatesCli.test.ts, tests/acceptance/tools/localToolContracts.test.ts, tests/boundary/filesystem/peCertificates.test.ts, tests/boundary/mcp/peCertificatesMcp.test.ts. None mentions Probe shapes, built by script inside a minimal PE32 image with one type-2 entry: 🤖 Written and tested with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊 |
|
Restack is Conflict files: On
|
Add inspect_pe_certificates for IMAGE_DIRECTORY_ENTRY_SECURITY WIN_CERTIFICATE entries and structurally decoded PKCS#7 signer and X.509 identity fields. The workflow reads a stable PE image without executing it or verifying signatures, chains, timestamps, or Authenticode digest coverage.
- Cap decoded DER values with a shared node budget so sibling fan-out cannot exhaust memory before the PKCS#7 check - Require SignedData version, encapContentInfo and a final signerInfos SET in RFC 5652 order, and a signature OCTET STRING in each SignerInfo - Decode OBJECT IDENTIFIER arcs as unbounded base-128 values, including a multi-byte first combined arc - Decode UniversalString names as UTF-32BE and reject invalid values - Share PE header admission with the resource layout so a section table that does not fit SizeOfHeaders is rejected
A crafted type-2 certificate with a 262,144-byte contentType OID stalled decodeOid for about 20 seconds, because one arc kept growing a BigInt before the signedData check ran. decodeOid now rejects an OID longer than 256 bytes and an arc longer than 32 bytes before decoding it. Certificate OIDs, including 2.25 UUID arcs, fit under both caps.
SignedData now requires one SignerInfo with a non-empty signature and a complete issuerAndSerialNumber. Certificate names share a 64 KiB decoded-byte budget and are copied once. A security directory that ends in a short WIN_CERTIFICATE header fails instead of reporting a complete table. The certificate CLI forwards the caller abort signal into cleanup. Co-authored-by: morluto <morluto@users.noreply.github.com>
The certificate restack follows current main. Workflow files stay at the fork tip so this branch update does not rewrite .github/workflows. Co-authored-by: morluto <morluto@users.noreply.github.com>
e1e756c to
c13f794
Compare
|
PR head is now |
Certificate and signer serial numbers and validity times now count against the shared decoded-text budget before they are converted, so a crafted certificate cannot push megabytes of text into the hashed evidence. The budget error now names certificate text rather than only names. Restore the CI workflow to match main: the CI_TIMING_TIER input, the tier-named timing artifacts and their publication conditions, the Windows typecheck, and the path and snapshot suites in the curated Windows lane.
Keep byte entropy from main next to the PE certificate provider, contract, CLI command and server wiring.
|
Addressed the three Greptile findings in 1ebf689, which also merges main into the branch.
format:check, lint, typecheck and build pass, along with the focused PE certificate, byte entropy, tool contract, catalog, ciTestReports, ciTestHistory and curatedWindowsLane tests. |
Keep Authenticode certificate inspection, including the decoded-text budget, beside ELF notes and the current CI workflow. Co-authored-by: morluto <morluto@users.noreply.github.com>
Reject X.509 records that omit subjectPublicKeyInfo or signatureValue, return subjectKeyIdentifier bytes as bounded hex, and advertise an absolute path in the PE certificate input schema. Co-authored-by: morluto <morluto@users.noreply.github.com>
ContentInfo and its signedData wrapper no longer ignore trailing values, and directory strings must round-trip as UTF-8 instead of becoming U+FFFD. Co-authored-by: morluto <morluto@users.noreply.github.com>
| default: | ||
| chargeDecodedName(budget, node.content.length); | ||
| return node.content.toString("latin1"); |
There was a problem hiding this comment.
Names stored as TeletexString (0x14) fall through to Latin-1 here. For example, the valid T.61 bytes c2 65 become Âe instead of é. Inspection succeeds, but the returned issuer and subject names contain the wrong text. This is a non-blocking concern for older certificates. Decode this encoding correctly, or reject unsupported name encodings instead of silently treating them as Latin-1.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/native/pe/PeCertificateDer.ts
Line: 227-229
Comment:
**Older certificate names change**
Names stored as `TeletexString` (`0x14`) fall through to Latin-1 here. For example, the valid T.61 bytes `c2 65` become `Âe` instead of `é`. Inspection succeeds, but the returned issuer and subject names contain the wrong text. This is a non-blocking concern for older certificates. Decode this encoding correctly, or reject unsupported name encodings instead of silently treating them as Latin-1.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
Add
inspect_pe_certificates/inspect-pe-certificatesfor one explicitlocal PE32/PE32+ image. The workflow reads
IMAGE_DIRECTORY_ENTRY_SECURITYas a file-offsetWIN_CERTIFICATEtable andreturns entry ranges, types, payload SHA-256, and structurally decoded PKCS#7
signedData signer/X.509 identity fields when type 2 is present.
It does not execute the image, verify signatures, chains, timestamps, or
Authenticode digest coverage. A missing security directory is observed absence
of that table, not proof the image is unsigned in every catalog or overlay
sense. Malformed PE headers, truncated entries, and non-signedData PKCS#7 fail
without partial success. No Windows SDK is required.
Usage
MCP:
inspect_pe_certificateswith an absolutepath.Tests
Merge resolution at
3ca73d13aagainstmain(c7efd3a38): preserves both PE certificate inspection and native export inspection across CLI/MCP registration, composition, effect metadata, and local-contract validation.npm run test:focusedselecting the PE certificate domain/parser/service, CLI/MCP/filesystem tests, tool-effects and local-contract tests, and native-export/PE-resource filesystem tests — exit 0, 10 files, 51 tests passed. Runtime and packaged-skill build passed.npm run check— exit 0.npm run docs:generateandnpm run docs:check— exit 0; includes 9 README consistency tests.Earlier validation:
npm run check:fastnpm run test:local --domain, parser, and service tests (10 passed)npm run test:focused --CLI, MCP, and filesystem tests (7 passed on thisfeature; plus the shared local-contract NUL case)
Demo
Author readiness
Human owner: @mvanhorn
Before merge
type(scope): outcome.