PKCS#7 improvements#10760
Open
Frauschi wants to merge 1 commit into
Open
Conversation
|
retest this please |
|
…ute handling Server-side PKCS#7 encode improvements that let downstream EST/SCEP enrollment code (wolfCert) drive the existing encoder through the public API rather than hand-rolling DER. Everything is gated under the existing HAVE_PKCS7 — no new build options and no new public functions; the convenience wrappers live caller-side. Allow degenerate (certs-only) SignedData encode Relax the hashOID != 0 requirement in PKCS7_EncodeSigned() when sidType == DEGENERATE_SID, so a caller can produce a certs-only bundle (no signer, attributes, or eContent — the form used by EST /cacerts and SCEP GetCACert) by selecting DEGENERATE_SID via wc_PKCS7_SetSignerIdentifierType() and calling wc_PKCS7_EncodeSignedData(). The output round-trips through wc_PKCS7_VerifySignedData(). Size the signed-attribute array to the actual count The SignerInfo attribute working array is now sized to the real attribute count instead of a fixed [7] array. An inline buffer (sized MAX_SIGNED_ATTRIBS_SZ, the historical footprint) covers the common allocation-free case; a heap buffer is used only when the count exceeds it. The default-attribute count comes from a single helper (wc_PKCS7_GetDefaultSignedAttribCount) so the sizing matches the emission logic exactly, and the canned-attribute write is bound-checked against the array capacity. This also fixes a latent overflow where the backing array was hardcoded [7] while the bound check used MAX_SIGNED_ATTRIBS_SZ. The macro is retained for source compatibility but no longer caps the count. Document the decoded-attribute value shape Documented the stable shape of PKCS7DecodedAttrib.value (the contents of the SET OF AttributeValue, outer SET tag stripped) so callers can rely on it. No behavior change. Fix multi-certificate decode in non-streaming builds Bound the additional-certificate loop in wc_PKCS7_VerifySignedData against the absolute end of the certificate set (idx + length) rather than the relative length. In NO_PKCS7_STREAM builds the old bound dropped trailing certificates (all but the first when a large eContent preceded the set), failing verification when the signer cert was among those dropped. Streaming builds were unaffected. Tests Added coverage in pkcs7signed_test: degenerate certs-only encode via the public API, nine-attribute encode (beyond the inline capacity), decoded attribute value shape for PrintableString and OCTET STRING, and a multi-certificate decode regression with large content that triggers the bound bug under NO_PKCS7_STREAM. Config-sensitive cases are guarded.
59bc162 to
6449ca0
Compare
Contributor
Author
|
Jenkins retest this please |
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
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.
Summary
Server-side PKCS#7/CMS encode improvements so downstream EST/SCEP enrollment code can drive the existing encoder through the public API instead of hand-rolling DER, plus a related multi-certificate decode bug fix found along the way. All changes are gated under the existing
HAVE_PKCS7- no new build options and no new public functions.Changes
SignedDataencodeRelaxes the
hashOID != 0requirement inPKCS7_EncodeSigned()whensidType == DEGENERATE_SID. A caller can now produce a degenerate, certs-onlySignedData(no signer, no signed attributes, no eContent - the form used by EST/cacertsand SCEPGetCACertresponses) entirely through the existing public API:The output round-trips through
wc_PKCS7_VerifySignedData()(which repopulatespkcs7->cert[]).The
SignerInfoattribute working array is now sized to the real attribute count instead of a fixed[7]array. An inline buffer (sizedMAX_SIGNED_ATTRIBS_SZ, the historical footprint) covers the common case with no heap allocation - important for no-malloc/static-memory builds - and a heap buffer is used only when the count exceeds it. The default-attribute count comes from a single helper (wc_PKCS7_GetDefaultSignedAttribCount()) so the sizing matches the emission logic exactly, and the canned-attribute write is bound-checked against the array capacity so any future drift fails withBUFFER_Erather than overflowing.This also fixes a latent overflow where the backing array was hardcoded
[7]while the bound check usedMAX_SIGNED_ATTRIBS_SZ. Profiles such as SCEPCertRep(RFC 8894), which carry several user attributes alongside the CMS auto-defaults, now encode without spuriousBUFFER_E.MAX_SIGNED_ATTRIBS_SZis retained for source compatibility but no longer caps the count (it only sizes the embedded inline buffer).Documents the stable, guaranteed shape of
PKCS7DecodedAttrib.value(the contents of theSET OF AttributeValue, with the outerSETtag stripped) so callers can rely on it. Documentation only - no behavior change.Bounds the additional-certificate loop in
wc_PKCS7_VerifySignedData()against the absolute end of the certificate set (idx + length) rather than the relative length. InNO_PKCS7_STREAMbuilds the previous bound stopped short and dropped trailing certificates - and, with a large eContent preceding the set, could drop all but the first certificate, causing verification to fail when the signer cert was among those dropped. Streaming builds were unaffected.Public API
No additions or changes.
MAX_SIGNED_ATTRIBS_SZis retained for source compatibility but no longer caps the attribute count (it now sizes the inline encode-time working buffer).Tests
Added coverage in
pkcs7signed_test:PrintableStringandOCTET STRING.NO_PKCS7_STREAM.Config-sensitive cases are guarded (
MAX_PKCS7_CERTS >= 3; the nine-attribute case requires heap or an enlarged inline array). The multi-cert test only validates the bound fix underNO_PKCS7_STREAM(documented in the test); in streaming builds it is a general multi-cert round-trip check.Verification
-Werror) on the default/streaming configuration.wolfcrypt/test/testwolfcryptpasses (PKCS7signed test passed!).