Trim certificates that are padded with null bytes - #106
Closed
donachan-tesla wants to merge 1 commit into
Closed
Conversation
Sometimes certificates are padded with null bytes and Golang doesn't like this. We trim the trailing null byte if necessary before calling x509.ParseCertificate().
Nicolas-Peiffer
added a commit
that referenced
this pull request
Aug 3, 2026
…ming Some tokens return CKA_VALUE in a fixed-size buffer, null-padded past the end of the certificate it holds. x509.ParseCertificate rejects the padding as trailing data, so FindCertificate — and FindAllPairedCertificates, which reaches the same code through makeKeyPair — failed outright on those tokens. DER is self-delimiting, so the certificate's own length header says where it ends. findCertificate now hands the attribute to parseCertificateValue, which reads one ASN.1 element with asn1.Unmarshal into an asn1.RawValue, parses RawValue.FullBytes, and requires whatever follows to be null bytes. This differs from #106, which trims the buffer with bytes.Trim(raw, "\x00") before parsing. Content-based trimming cannot tell padding from certificate: the last byte of a certificate is the last byte of its signature, which is effectively random, so roughly one certificate in 256 legitimately ends in a null byte. Generating self-signed ECDSA certificates until one does — 81 attempts — and running both versions against it: baseline (no trim), unpadded: parses OK #106, unpadded: x509: malformed certificate #106, padded: x509: malformed certificate length-delimited, either way: parses OK So that one-in-256 breaks on tokens that pad, which the change was written for, and also breaks on tokens that do not, which worked before. bytes.Trim is bidirectional besides, so leading null bytes are stripped too: the buffer no longer starts with the certificate, and reinterpreting it silently is worse than reporting it. Trailing bytes that are not null are likewise an error here rather than something to discard — a truncated or misreported attribute should surface, not be papered over. Certificate parsing moves out of findCertificate so it can be tested without a token. TestParseCertificateValue covers plain DER, null-padded DER, and a certificate whose signature ends in a null byte both unpadded and padded — the case #106 gets wrong, generated by looping until one turns up rather than hardcoding a fixture. TestParseCertificateValueRejectsGarbage pins the error paths: non-null trailing data, leading null bytes, nil input, and a certificate truncated in half then null-padded back to its original length, which is precisely the corruption a padding-tolerant parser must not accept. Reported by @donachan-tesla in #106, with the null-padded tokens that make this reachable at all. Refs #106 — #106 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Hi @donachan-tesla 😃 , TY 🙏 for your interest and this issue. I am part of the new maintainers of In 2026 the big project for
The other big project for 2026 is giving + moving the crypto11 project under the Eclipse Foundation. This is an opportunity for us to reconsider PRs and issues. I ran your PR on a an LLM code assistant, and I implemented its suggested fix d5490a7. This is different from your suggestion, but it is probably better to use a parsing following ASN.1. Closing this PR. Target |
Nicolas-Peiffer
added a commit
that referenced
this pull request
Aug 4, 2026
FindCertificate needs an id, label or serial, and FindAllPairedCertificates only returns certificates that have a matching private key. A caller that knows nothing about a token — the case PR #71 was opened for in August 2020 — had no way to list what is on it. Add the plain enumerator, symmetric with FindAllKeys and FindAllKeyPairs. Take the API from #71, with three changes to the implementation: - Page C_FindObjects instead of taking a single call's worth. The original patch stopped at maxHandlePerFind, silently returning 20 certificates on a token holding more. Enumeration now goes through findKeysWithAttributes, which already pages and is not key-specific, so the batching is right in one place rather than two. - Match only CKC_X_509 objects. A WTLS or attribute certificate is not an X.509 certificate and would fail x509.ParseCertificate, so a single one would hide every real certificate on the token — the shape of #68. The token filters them out instead. - Parse via parseCertificateValue, not x509.ParseCertificate directly, so the null-padded CKA_VALUE handling from d5490a7 applies here too. The original patch would have regressed on the tokens #106 reported. A CKA_VALUE that claims to be X.509 but does not parse is still an error rather than a skip: that is corruption, not a kind of certificate this package cannot represent. The error now names the object by CKA_ID and CKA_LABEL, since failing anonymously part-way through a token full of certificates says nothing about which one is at fault. The label is quoted, not interpolated raw — it is arbitrary bytes chosen by whoever wrote the object. Document that enumeration is not trust. This API's whole audience is the caller who knows nothing about the token, which is exactly the caller most likely to feed the result into a trust store; anyone able to write to the token can add a certificate. Tests import a few certificates and then one more than a batch, matching on full DER so the finder is shown to return the right bytes and not merely the right count. They delete only what they imported: #71 proposed a removeAllCertificates helper that destroys every certificate object on the token, which is not something a test should do to a shared or production HSM. #71 Co-Authored-By: mekpavit <21259908+mekpavit@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nicolas-Peiffer
added a commit
that referenced
this pull request
Aug 6, 2026
FindCertificate returns one certificate and FindAllCertificates returns every certificate unordered, so a caller needing the issuers above a leaf — TLS being the obvious case — had to reassemble the chain itself. Add the walk, closing issue #91. Take the API and the algorithm from PR #83, which proposed it in June 2021 and cannot be rebased onto the current tree, with four changes: - Walk iteratively and skip certificates already placed. #83 recursed and guarded against repeats by scanning its own return value, which is always empty at the point it is read, so the guard never fired. Two CAs cross-signing each other recursed until the stack was exhausted, and anything able to write to the token decides whether that happens. - Settle the issuer by signature, not by name. #83 took the first object whose CKA_SUBJECT matched, so a token holding a renewed or cross-signed CA — two certificates, one distinguished name — returned whichever the token listed first and a chain that need not verify. - Return a short chain when the issuer is absent. #83's identifier fallback raised an error when nothing matched, so leaf plus intermediate on the token with the root in the system trust store — the ordinary arrangement — failed the call. Its test imported the root as well and never saw it. - Reuse findKeysWithAttributes and parseCertificateValue rather than adding a second pager and calling x509.ParseCertificate directly, which would have reverted d5490a7 for the null-padded CKA_VALUE of #106. Only run the identifier scan when the certificate names an authority key identifier: it costs a read of every certificate on the token, and an empty identifier matches every certificate that has no subject key identifier. FindAllCertificates and the new walk now share findX509Certificates, so the CKC_X_509 filter and the DER parsing stay in one place. The API is not extended to finding or deleting certificates by arbitrary attributes, which #83 also carried: that is unrelated to the chain and is its own decision. Tests cover the full chain, a missing root, a same-subject decoy CA imported ahead of the real one, a subject attribute that disagrees with the DER so only the key identifier can link the chain, and a cross-signed cycle. Removing the signature check, the identifier fallback or the already-placed guard each fails its test, the last by hanging. #91 #83 Co-Authored-By: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sometimes certificates are padded with null bytes and Golang doesn't like this. We trim the trailing null byte if necessary before calling
x509.ParseCertificate().