From e76d1ebcf347a74226ecea2c326dc51f3d2d95d0 Mon Sep 17 00:00:00 2001 From: jamestexas <18285880+jamestexas@users.noreply.github.com> Date: Tue, 19 May 2026 14:43:32 -0600 Subject: [PATCH 1/4] build: add long-fuzz / overnight-fuzz / mutation-test / govulncheck targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit-equivalent test workflow as Makefile targets so they're scripted and reproducible: - `make long-fuzz` — run every fuzz target for FUZZTIME each (default 10m; override e.g. FUZZTIME=30m or FUZZTIME=1h). Anchored regexes (^Foo$) prevent multi-match. - `make overnight-fuzz` — long-fuzz with FUZZTIME=1h (≈16h total at current target count). For dedicated burn-in runs. - `make mutation-test` — run gremlins mutation testing on pkg/cms; installs the tool if missing. Surviving mutants indicate test gaps. - `make govulncheck` — surface stdlib/dependency CVEs reachable from our call graph; installs the tool if missing. The default target stays `help`, listing the new audit targets alongside the existing build/test/lint/docker ones. Co-Authored-By: Claude Opus 4.7 --- Makefile | 77 +++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 02be1ce..327482c 100644 --- a/Makefile +++ b/Makefile @@ -7,23 +7,35 @@ GOFMT=$(GOCMD) fmt GOVET=$(GOCMD) vet # Build targets -.PHONY: all build test fmt lint clean docker-test docker-build docker-shell help +.PHONY: all build test fmt lint clean docker-test docker-build docker-shell help \ + long-fuzz overnight-fuzz mutation-test govulncheck + +# Fuzz settings — override on the command line, e.g. +# make long-fuzz FUZZTIME=1h +FUZZTIME ?= 10m all: test build help: @echo "go-cms Makefile targets:" @echo "" - @echo " make build - Build the cms-test-tool binary" - @echo " make test - Run all tests with race detector and coverage" - @echo " make fmt - Format all Go code" - @echo " make lint - Run golangci-lint" - @echo " make clean - Remove build artifacts and test files" + @echo " make build - Build the cms-test-tool binary" + @echo " make test - Run all tests with race detector and coverage" + @echo " make fmt - Format all Go code" + @echo " make lint - Run golangci-lint" + @echo " make clean - Remove build artifacts and test files" + @echo "" + @echo "Audit-level test targets:" + @echo " make long-fuzz - Run every fuzz target for FUZZTIME each" + @echo " (default 10m; override with FUZZTIME=30m or 1h)" + @echo " make overnight-fuzz - long-fuzz with FUZZTIME=1h per target (~12h total)" + @echo " make mutation-test - Run gremlins mutation testing on pkg/cms" + @echo " make govulncheck - Surface stdlib/dependency CVEs" @echo "" @echo "Docker-based OpenSSL testing:" - @echo " make docker-build - Build Docker test image" - @echo " make docker-test - Run OpenSSL interop tests in Docker" - @echo " make docker-shell - Get a shell in Docker for debugging" + @echo " make docker-build - Build Docker test image" + @echo " make docker-test - Run OpenSSL interop tests in Docker" + @echo " make docker-shell - Get a shell in Docker for debugging" @echo "" @echo "Default target: make all (test + build)" @@ -56,4 +68,51 @@ docker-test: docker-shell: docker-build docker run --rm -it go-cms-test bash +# Long-running fuzz suite. Each target gets FUZZTIME of CPU time. Anchored +# regexes prevent multi-match (e.g. FuzzVerify vs FuzzVerifyAcceptsOnlyCanonicalForm). +long-fuzz: + @echo "Running every fuzz target for $(FUZZTIME) each." + @echo "Total wall time ≈ $(FUZZTIME) × (number of fuzz targets)." + @for target in \ + FuzzVerify \ + FuzzParseASN1Length \ + FuzzExtractSetContent \ + FuzzUnwrapContext0 \ + FuzzConstantTimeCompareBigInt \ + FuzzSignVerifyRoundtrip \ + FuzzSignDataWithoutAttributesRoundtrip \ + FuzzSignDataWithSignerRoundtrip \ + FuzzCase2SignDeterminism \ + FuzzInsertByte \ + FuzzDeleteByte \ + FuzzAppendTrailingData \ + FuzzCertBagSubstitution \ + FuzzVerifyAcceptsOnlyCanonicalForm \ + FuzzReplaceOIDBytes \ + FuzzDeclaredLengthOverflow \ + ; do \ + echo "=== $$target ($(FUZZTIME)) ==="; \ + $(GOTEST) -fuzz="^$$target$$" -fuzztime=$(FUZZTIME) ./pkg/cms || exit 1; \ + done + @echo "" + @echo "Long fuzz complete. Any crashes are now under pkg/cms/testdata/fuzz/." + +# Overnight fuzz: 1 hour per target. Roughly 16 hours wall time for the +# current target list. Run on a dedicated machine; not part of CI. +overnight-fuzz: + @$(MAKE) long-fuzz FUZZTIME=1h + +# Mutation testing — verifies that the test suite would actually fail if +# someone introduced a logic bug. Surviving mutants point at test gaps. +mutation-test: + @which gremlins >/dev/null 2>&1 || $(GOCMD) install github.com/go-gremlins/gremlins/cmd/gremlins@latest + @echo "Running gremlins on pkg/cms (this can take several minutes)." + @cd pkg/cms && GOWORK=off gremlins unleash --timeout-coefficient 5 + +# govulncheck — surface stdlib and dependency CVEs reachable from our +# call graph. Mirrors the CI check. +govulncheck: + @which govulncheck >/dev/null 2>&1 || $(GOCMD) install golang.org/x/vuln/cmd/govulncheck@latest + govulncheck ./... + .DEFAULT_GOAL := help From 4469f97b691025bde38b47a77c859744e7d35d7e Mon Sep 17 00:00:00 2001 From: jamestexas <18285880+jamestexas@users.noreply.github.com> Date: Tue, 19 May 2026 14:47:40 -0600 Subject: [PATCH 2/4] refactor(cms): replace local compareBytes with stdlib bytes.Compare compareBytes was a reimplementation of bytes.Compare with the same contract (lexicographic compare, -1/0/1). Switching to the stdlib function removes 22 lines of code, eliminates a small cluster of (false-positive) surviving mutants in mutation testing, and trims one ad-hoc helper from the package surface. Behavior is unchanged: bytes.Compare implements exactly the lexicographic byte ordering DER mandates for SET OF. Co-Authored-By: Claude Opus 4.7 --- pkg/cms/verifier.go | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/pkg/cms/verifier.go b/pkg/cms/verifier.go index fb07b44..1c05611 100644 --- a/pkg/cms/verifier.go +++ b/pkg/cms/verifier.go @@ -936,9 +936,12 @@ func validateAttributeSetOrder(signedAttrs []byte) error { remaining = rest } - // Verify SET OF ordering (lexicographic byte order) + // Verify SET OF ordering: DER requires strict ascending lexicographic + // order over the element encodings (RFC 5652 SignedAttributes is a + // SET OF, and §5.3 inherits DER from §10.1). bytes.Compare gives us + // exactly the lexicographic ordering specified. for i := 1; i < len(encodings); i++ { - if compareBytes(encodings[i-1], encodings[i]) >= 0 { + if bytes.Compare(encodings[i-1], encodings[i]) >= 0 { return NewValidationError("SignedAttributes", "", "attributes not in DER canonical order (RFC 5652 requires sorted SET OF)", nil) } @@ -947,32 +950,6 @@ func validateAttributeSetOrder(signedAttrs []byte) error { return nil } -// compareBytes performs lexicographic comparison of byte slices -// Returns -1 if a < b, 0 if a == b, 1 if a > b -func compareBytes(a, b []byte) int { - minLen := len(a) - if len(b) < minLen { - minLen = len(b) - } - - for i := 0; i < minLen; i++ { - if a[i] < b[i] { - return -1 - } - if a[i] > b[i] { - return 1 - } - } - - if len(a) < len(b) { - return -1 - } - if len(a) > len(b) { - return 1 - } - return 0 -} - // constantTimeCompareBigInt performs constant-time comparison of two big integers // Returns true if they are equal, false otherwise func constantTimeCompareBigInt(a, b *big.Int) bool { From ffff3972882e9d3f20824931bd608db0c1f9253b Mon Sep 17 00:00:00 2001 From: jamestexas <18285880+jamestexas@users.noreply.github.com> Date: Wed, 17 Jun 2026 10:47:05 -0600 Subject: [PATCH 3/4] test(cms): SKI-form CMS test harness + fix matchesSID canonicality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the largest mutation-testing gap on this branch (the SubjectKeyIdentifier path of matchesSID and the SignerInfo version-vs-SID cross-check) by adding a from-scratch CMS construction harness that the production signer cannot reach. Finding & fix: matchesSID previously expected EXPLICIT [0] wrapping of the SKI (A0 04 ): the code unconditionally tried asn1.Unmarshal(sidRaw.Bytes, &keyID) which only succeeds when there's a nested OCTET STRING TLV inside. RFC 5652's ASN.1 module declares IMPLICIT TAGS, so the canonical encoding is 80 with no nested OCTET STRING — exactly what OpenSSL and github.com/github/ietf-cms emit. As a result, go-cms could not verify SKI-form SignerInfo produced by any standards-compliant CMS producer. Fix: treat sidRaw.Bytes as the raw SKI value when the [0] tag is primitive (canonical IMPLICIT form), and reject the constructed form outright. Rejecting the EXPLICIT variant also closes a malleability surface: under the old code, the same logical SID could be encoded two different ways, breaking content-addressing built on CMS blob hashes. New test harness (pkg/cms/cms_builder_test.go): - cmsBuildConfig + buildTestCMS: hand-assemble a CMS SignedData blob with explicit control over SID form (IAS vs SKI), SignerInfo and SignedData versions, EncapContentInfo OID, SKI corruption, IMPLICIT vs EXPLICIT SKI wrapping, and Case-1 vs Case-2 SignedAttributes. - The signature itself is always real Ed25519 over canonical-DER SignedAttributes (or content for Case 2) — verifier behaviour we observe is cryptographically authentic, not asn1-decode-passes-and- stops. SKI test suite (pkg/cms/ski_test.go, 8 tests): - TestBuilderHappyPath_{IAS,SKI}: harness sanity checks. - TestSKI_VersionMustBe3 / TestIAS_VersionMustBe1: prove the SignerInfo version ↔ SID-form cross-check (verifier.go:216) rejects SKI+v1 and IAS+v3. Previously LIVED in mutation testing because no SKI test exercised the branch. - TestSKI_KeyIdMismatchRejects: corrupt SKI bytes must fail matchesSID equality. - TestSKI_TamperResistance: SKI-form signature must still reject detached-data tampering. - TestSKI_RejectsExplicitWrapping: documents and locks in the canonicality rejection. - TestSKI_Case2: SKI × Case 2 intersection. Validation: - Full test suite passes under -race at 78.9% statement coverage (+1.0 from main). - Mutation testing: efficacy 76.36% → 79.76%, mutator coverage 83.77% → 85.76%. The version-vs-SID cross-check mutants in matchesSID flip from LIVED to KILLED. Co-Authored-By: Claude Opus 4.7 --- pkg/cms/cms_builder_test.go | 366 ++++++++++++++++++++++++++++++++++++ pkg/cms/ski_test.go | 171 +++++++++++++++++ pkg/cms/verifier.go | 28 ++- 3 files changed, 556 insertions(+), 9 deletions(-) create mode 100644 pkg/cms/cms_builder_test.go create mode 100644 pkg/cms/ski_test.go diff --git a/pkg/cms/cms_builder_test.go b/pkg/cms/cms_builder_test.go new file mode 100644 index 0000000..aa0989e --- /dev/null +++ b/pkg/cms/cms_builder_test.go @@ -0,0 +1,366 @@ +package cms + +import ( + "bytes" + "crypto/ed25519" + "crypto/rand" + "crypto/sha1" // #nosec G505 -- SKI is a PKIX identifier, not a security primitive + "crypto/sha512" + "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" + "math/big" + "testing" + "time" +) + +// cmsBuilder is an internal test-only harness that constructs CMS +// SignedData messages from scratch with explicit control over fields the +// production signer doesn't expose. It exists because the verifier's full +// behavioural surface — SKI-form SignerInfo, invalid version/SID pairs, +// non-default EncapContentInfo OIDs — cannot be exercised through the +// public SignData entry point. +// +// Two design rules: +// +// 1. The signature itself is always *real* — produced by ed25519 over +// the canonical-DER SignedAttributes (Case 1) or directly over the +// content (Case 2). Tests that probe verifier behaviour must observe +// real cryptographic outcomes, not asn1-decode-passes-and-stops. +// 2. Every knob has a sensible default that yields a verify-clean CMS, +// so tests can isolate exactly one deviation at a time. + +// sidForm selects how SignerIdentifier is encoded inside SignerInfo. +// +// - sidIAS: SEQUENCE { Issuer, SerialNumber } — RFC 5652 Case A, +// SignerInfo.Version MUST be 1. +// - sidSKI: [0] IMPLICIT OCTET STRING — RFC 5652 Case B, +// SignerInfo.Version MUST be 3. +type sidForm int + +const ( + sidIAS sidForm = iota + sidSKI +) + +// cmsBuildConfig collects every knob the builder exposes. Zero values +// resolve to verify-clean defaults; tests override only what they probe. +type cmsBuildConfig struct { + // Data is the detached content the verifier will be asked to + // reconstruct the digest of. Defaults to a short non-empty payload. + Data []byte + + // SIDForm controls SignerIdentifier encoding (default: sidIAS). + SIDForm sidForm + + // SIVersion overrides SignerInfo.Version. Zero means "derive from + // SIDForm" (1 for IAS, 3 for SKI). Tests that want to probe mismatch + // (e.g. SKI+version=1) set this explicitly. + SIVersion int + + // SDVersion overrides SignedData.Version. Zero means default of 1. + SDVersion int + + // EContentOID overrides EncapContentInfo.eContentType. Zero (nil) + // means oidData (1.2.840.113549.1.7.1). + EContentOID asn1.ObjectIdentifier + + // OmitAttrs produces a Case 2 (no signedAttributes) CMS when true. + OmitAttrs bool + + // CorruptSKI replaces SKI bytes with garbage when SIDForm is sidSKI. + // Used to test that matchesSID rejects key-id mismatch. + CorruptSKI bool + + // SKIUseExplicit forces the EXPLICIT-wrapped encoding of the SKI SID + // (`A0 04 `) instead of the canonical IMPLICIT + // form. Used to verify the canonicality check in matchesSID. + SKIUseExplicit bool +} + +// newBuilderSigner returns a self-signed Ed25519 cert/key/pool whose +// cert has a populated SubjectKeyId (so SKI-form SignerInfo can match +// against it). Reusable across builder-driven tests. +func newBuilderSigner(tb testing.TB) (*x509.Certificate, ed25519.PrivateKey, *x509.CertPool) { + tb.Helper() + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + tb.Fatalf("ed25519.GenerateKey: %v", err) + } + // Compute a PKIX-standard SubjectKeyId (RFC 5280 §4.2.1.2 method 1: + // 160-bit SHA-1 of the DER-encoded subjectPublicKey BIT STRING value). + // SHA-1 is the IETF-blessed input here — it's an identifier, not a + // security primitive. Go's auto-SKI only triggers for IsCA=true certs, + // so we set it explicitly. + skiSum := sha1.Sum(priv.Public().(ed25519.PublicKey)) // #nosec G401 -- see above + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(0xc115), + Subject: pkix.Name{Organization: []string{"go-cms builder"}}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + SubjectKeyId: skiSum[:], + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, priv.Public(), priv) + if err != nil { + tb.Fatalf("x509.CreateCertificate: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + tb.Fatalf("x509.ParseCertificate: %v", err) + } + if len(cert.SubjectKeyId) == 0 { + tb.Fatal("cert has empty SubjectKeyId; SKI tests need a non-empty one") + } + pool := x509.NewCertPool() + pool.AddCert(cert) + return cert, priv, pool +} + +// buildTestCMS assembles a CMS SignedData blob byte-for-byte according +// to cfg, producing a *real* Ed25519 signature over the canonical SET OF +// SignedAttributes (Case 1) or directly over Data (Case 2). +// +// The returned bytes are intended to be passed straight to Verify; for +// happy-path inputs the verifier accepts them. Tests modify cfg one +// knob at a time to probe specific verifier branches. +func buildTestCMS(tb testing.TB, cert *x509.Certificate, priv ed25519.PrivateKey, cfg cmsBuildConfig) []byte { + tb.Helper() + + // Resolve defaults. + data := cfg.Data + if data == nil { + data = []byte("builder-default-content") + } + siVersion := cfg.SIVersion + if siVersion == 0 { + if cfg.SIDForm == sidSKI { + siVersion = 3 + } else { + siVersion = 1 + } + } + sdVersion := cfg.SDVersion + if sdVersion == 0 { + sdVersion = 1 + } + eContentOID := cfg.EContentOID + if eContentOID == nil { + eContentOID = oidData + } + + // 1. Compute SignedAttributes (Case 1) and sign. + var signedAttrsImplicit []byte + var signature []byte + if cfg.OmitAttrs { + signature = ed25519.Sign(priv, data) + } else { + digest := sha512.Sum512(data) + attrs, err := createSignedAttributes(digest[:], time.Now().UTC()) + if err != nil { + tb.Fatalf("createSignedAttributes: %v", err) + } + setBytes, err := encodeAttributesAsSet(attrs) + if err != nil { + tb.Fatalf("encodeAttributesAsSet: %v", err) + } + signature = ed25519.Sign(priv, setBytes) + signedAttrsImplicit, err = encodeSignedAttributesImplicit(attrs) + if err != nil { + tb.Fatalf("encodeSignedAttributesImplicit: %v", err) + } + } + + // 2. Build SignerInfo. + siBytes := encodeSignerInfo(tb, cert, signedAttrsImplicit, signature, oidSHA512, siVersion, cfg.SIDForm, cfg.CorruptSKI, cfg.SKIUseExplicit) + + // 3. Build the rest of the structure (SignedData + ContentInfo). + return encodeOuterCMS(tb, cert, siBytes, oidSHA512, sdVersion, eContentOID) +} + +// encodeSignerInfo emits the SignerInfo SEQUENCE according to cfg. +// +// SignerInfo ::= SEQUENCE { +// version CMSVersion, +// sid SignerIdentifier, +// digestAlgorithm DigestAlgorithmIdentifier, +// signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL, +// signatureAlgorithm SignatureAlgorithmIdentifier, +// signature SignatureValue } +func encodeSignerInfo( + tb testing.TB, + cert *x509.Certificate, + signedAttrsImplicit []byte, + signature []byte, + digestOID asn1.ObjectIdentifier, + version int, + form sidForm, + corruptSKI bool, + skiUseExplicit bool, +) []byte { + tb.Helper() + var buf bytes.Buffer + + // Version INTEGER. + mustMarshal(tb, &buf, version) + + // SignerIdentifier. + switch form { + case sidIAS: + ias := struct { + Issuer pkix.RDNSequence + SerialNumber *big.Int + }{ + Issuer: cert.Issuer.ToRDNSequence(), + SerialNumber: cert.SerialNumber, + } + mustMarshal(tb, &buf, ias) + + case sidSKI: + ski := cert.SubjectKeyId + if corruptSKI { + ski = bytes.Repeat([]byte{0xff}, len(ski)) + } + if skiUseExplicit { + // Non-canonical EXPLICIT [0] wrapping for malleability tests: + // A0 04 . asn1.Marshal can't emit this + // directly so we hand-build the TLV. + buf.WriteByte(0xA0) + buf.WriteByte(byte(2 + len(ski))) + buf.WriteByte(0x04) + buf.WriteByte(byte(len(ski))) + buf.Write(ski) + } else { + // Canonical RFC 5652 §5.3 form: [0] IMPLICIT OCTET STRING — + // the OCTET STRING tag is replaced by [0]. Tag byte 0x80 + // (context, primitive, tag 0), length, raw SKI bytes. + buf.WriteByte(0x80) + buf.WriteByte(byte(len(ski))) + buf.Write(ski) + } + } + + // DigestAlgorithm. + mustMarshal(tb, &buf, pkix.AlgorithmIdentifier{Algorithm: digestOID}) + + // SignedAttrs [0] IMPLICIT (omitted for Case 2). + if signedAttrsImplicit != nil { + buf.Write(signedAttrsImplicit) + } + + // SignatureAlgorithm. + mustMarshal(tb, &buf, pkix.AlgorithmIdentifier{Algorithm: oidEd25519}) + + // Signature OCTET STRING. + mustMarshal(tb, &buf, signature) + + // Wrap in SEQUENCE. + content := buf.Bytes() + header := makeSequenceHeader(len(content)) + return append(header, content...) +} + +// encodeOuterCMS wraps SignerInfo bytes into a full ContentInfo → +// SignedData → SignerInfos chain. Mirrors signer.buildCMS but with +// explicit version + eContentType control. +func encodeOuterCMS( + tb testing.TB, + cert *x509.Certificate, + signerInfo []byte, + digestOID asn1.ObjectIdentifier, + sdVersion int, + eContentOID asn1.ObjectIdentifier, +) []byte { + tb.Helper() + var sdBuf bytes.Buffer + + // Version. + mustMarshal(tb, &sdBuf, sdVersion) + + // DigestAlgorithms SET OF AlgorithmIdentifier. + digestAlgBytes, err := asn1.Marshal([]pkix.AlgorithmIdentifier{{Algorithm: digestOID}}) + if err != nil { + tb.Fatalf("marshal digest algs: %v", err) + } + // asn1 emits SEQUENCE OF (0x30); CMS expects SET OF (0x31). + if len(digestAlgBytes) > 0 && digestAlgBytes[0] == 0x30 { + digestAlgBytes[0] = 0x31 + } + sdBuf.Write(digestAlgBytes) + + // EncapContentInfo: SEQUENCE { eContentType, [0] EXPLICIT eContent OPTIONAL } + // eContent omitted for detached signature. + encap := struct { + ContentType asn1.ObjectIdentifier + Content asn1.RawValue `asn1:"explicit,optional,tag:0"` + }{ContentType: eContentOID} + mustMarshal(tb, &sdBuf, encap) + + // Certificates [0] IMPLICIT — raw DER, no inner SET (matching the + // signer's chosen encoding; OpenSSL rejects an inner SET here). + certHeader := []byte{0xA0} + cl := len(cert.Raw) + switch { + case cl < 128: + certHeader = append(certHeader, byte(cl)) + case cl < 256: + certHeader = append(certHeader, 0x81, byte(cl)) + case cl < 65536: + certHeader = append(certHeader, 0x82, byte(cl>>8), byte(cl)) + default: + tb.Fatalf("cert too large: %d bytes", cl) + } + sdBuf.Write(certHeader) + sdBuf.Write(cert.Raw) + + // SignerInfos SET OF SignerInfo. + siSetHeader := makeSetHeader(len(signerInfo)) + sdBuf.Write(siSetHeader) + sdBuf.Write(signerInfo) + + // SignedData SEQUENCE. + sdContent := sdBuf.Bytes() + sdHeader := makeSequenceHeader(len(sdContent)) + signedData := append(sdHeader, sdContent...) + + // ContentInfo SEQUENCE { contentType id-signedData, content [0] EXPLICIT SignedData } + var ciBuf bytes.Buffer + mustMarshal(tb, &ciBuf, oidSignedData) + + // [0] EXPLICIT wraps the SignedData SEQUENCE. + wrappedSD := wrapExplicitContext0(signedData) + ciBuf.Write(wrappedSD) + + ciContent := ciBuf.Bytes() + ciHeader := makeSequenceHeader(len(ciContent)) + return append(ciHeader, ciContent...) +} + +// wrapExplicitContext0 produces "A0 " — the EXPLICIT [0] +// wrapping around the SignedData SEQUENCE in ContentInfo. +func wrapExplicitContext0(inner []byte) []byte { + header := []byte{0xA0} // context-specific, constructed, tag 0 + l := len(inner) + switch { + case l < 128: + header = append(header, byte(l)) + case l < 256: + header = append(header, 0x81, byte(l)) + case l < 65536: + header = append(header, 0x82, byte(l>>8), byte(l)) + default: + header = append(header, 0x83, byte(l>>16), byte(l>>8), byte(l)) + } + return append(header, inner...) +} + +// mustMarshal asn1.Marshals v and appends to buf, failing the test on +// error. Used internally to keep the builder code readable. +func mustMarshal(tb testing.TB, buf *bytes.Buffer, v any) { + tb.Helper() + b, err := asn1.Marshal(v) + if err != nil { + tb.Fatalf("asn1.Marshal(%T): %v", v, err) + } + buf.Write(b) +} diff --git a/pkg/cms/ski_test.go b/pkg/cms/ski_test.go new file mode 100644 index 0000000..d1a9833 --- /dev/null +++ b/pkg/cms/ski_test.go @@ -0,0 +1,171 @@ +package cms + +import ( + "bytes" + "testing" +) + +// These tests exercise SignerInfo paths that the production signer +// (SignData / SignDataWithoutAttributes / SignDataWithSigner) cannot +// emit — chiefly the SubjectKeyIdentifier (SKI) form of SignerIdentifier +// per RFC 5652 Case B. Mutation-testing baseline before this file +// showed ~60 mutants in matchesSID and the SignerInfo.Version +// cross-check were NOT COVERED, because no test reached that branch. +// All construction goes through buildTestCMS (cms_builder_test.go). + +// TestBuilderHappyPath_IAS sanity-checks the harness: with all defaults +// (IAS-form, v1, id-data eContent) the verifier must accept it. If this +// fails, the builder itself is wrong and every other test in this file +// is meaningless. +func TestBuilderHappyPath_IAS(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("happy-path IAS") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{Data: data}) + certs, err := Verify(sig, data, VerifyOptions{Roots: pool}) + if err != nil { + t.Fatalf("Verify rejected builder default IAS-form CMS: %v", err) + } + if !bytes.Equal(certs[0].Raw, cert.Raw) { + t.Fatalf("Verify returned wrong cert") + } +} + +// TestBuilderHappyPath_SKI is the load-bearing test for the SKI branch: +// build a CMS with [0] IMPLICIT OCTET STRING SignerIdentifier carrying +// the cert's SubjectKeyId, and confirm Verify accepts it. This exercises +// the entire matchesSID SKI path (lines 1011-1022) which the production +// signer never reaches. +func TestBuilderHappyPath_SKI(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("happy-path SKI") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + }) + certs, err := Verify(sig, data, VerifyOptions{Roots: pool}) + if err != nil { + t.Fatalf("Verify rejected SKI-form CMS: %v", err) + } + if !bytes.Equal(certs[0].Raw, cert.Raw) { + t.Fatalf("Verify returned wrong cert for SKI form") + } +} + +// TestSKI_VersionMustBe3 confirms the SignerInfo.Version cross-check +// rejects SKI+v1 (per RFC 5652 §5.3). This kills the +// CONDITIONALS_NEGATION mutants at verifier.go:216:* that survived +// before because no SKI test existed. +func TestSKI_VersionMustBe3(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ski v1 mismatch") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + SIVersion: 1, // explicit wrong version + }) + if _, err := Verify(sig, data, VerifyOptions{Roots: pool}); err == nil { + t.Fatal("Verify accepted SKI-form SignerInfo with Version=1; RFC 5652 §5.3 requires 3") + } +} + +// TestIAS_VersionMustBe1 is the symmetric check for the IAS path. +// Already covered by an existing rfc8419 test, but we restate it via the +// builder so it's exercised through the same code path as the SKI +// variants, ensuring no SID/version cross-check regressions sneak in. +func TestIAS_VersionMustBe1(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ias v3 mismatch") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidIAS, + SIVersion: 3, // explicit wrong version + }) + if _, err := Verify(sig, data, VerifyOptions{Roots: pool}); err == nil { + t.Fatal("Verify accepted IAS-form SignerInfo with Version=3; RFC 5652 §5.3 requires 1") + } +} + +// TestSKI_KeyIdMismatchRejects fills the SKI bytes with 0xff (corrupt) +// so the value cannot match the cert's actual SubjectKeyId. matchesSID +// must reject. Kills the boundary mutants at verifier.go:1019 that +// compare the SKI lengths and the body comparison. +func TestSKI_KeyIdMismatchRejects(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ski corrupted id") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + CorruptSKI: true, + }) + if _, err := Verify(sig, data, VerifyOptions{Roots: pool}); err == nil { + t.Fatal("Verify accepted SKI-form SignerInfo whose key id does not match the cert") + } +} + +// TestSKI_TamperResistance: an SKI-form valid signature must reject any +// single-byte tamper of the detached data, mirroring the IAS-form +// roundtrip invariant. Ensures the SKI path doesn't accidentally skip +// signature verification. +func TestSKI_TamperResistance(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ski tamper test") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + }) + + tampered := append([]byte(nil), data...) + tampered[0] ^= 0xff + if _, err := Verify(sig, tampered, VerifyOptions{Roots: pool}); err == nil { + t.Fatal("Verify accepted SKI-form CMS against tampered data") + } +} + +// TestSKI_RejectsExplicitWrapping proves the verifier rejects the +// non-canonical "EXPLICIT [0]" encoding of SubjectKeyIdentifier +// (A0 04 ). RFC 5652's ASN.1 module uses IMPLICIT +// TAGS by default, so the canonical encoding is 80 ; +// accepting both would be a malleability surface (same logical SID with +// two different DER byte sequences) and would break content-addressing +// guarantees built on CMS-blob hashes. +// +// Before the matchesSID fix in this branch, the verifier ONLY accepted +// the EXPLICIT form — meaning it rejected canonical SKI emitted by +// OpenSSL and github.com/github/ietf-cms. +func TestSKI_RejectsExplicitWrapping(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ski rejects explicit") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + SKIUseExplicit: true, + }) + if _, err := Verify(sig, data, VerifyOptions{Roots: pool}); err == nil { + t.Fatal("Verify accepted non-canonical EXPLICIT [0] SubjectKeyIdentifier; should reject per RFC 5652 IMPLICIT TAGS") + } +} + +// TestSKI_Case2 builds an SKI-form Case 2 (no signed attributes) CMS +// and verifies it. Exercises the intersection of two code paths the +// production signer cannot emit together: SKI SID plus the Case 2 +// signature contract. +func TestSKI_Case2(t *testing.T) { + cert, priv, pool := newBuilderSigner(t) + data := []byte("ski case 2") + + sig := buildTestCMS(t, cert, priv, cmsBuildConfig{ + Data: data, + SIDForm: sidSKI, + OmitAttrs: true, + }) + if _, err := Verify(sig, data, VerifyOptions{Roots: pool}); err != nil { + t.Fatalf("Verify rejected SKI-form Case 2 CMS: %v", err) + } +} diff --git a/pkg/cms/verifier.go b/pkg/cms/verifier.go index 1c05611..64b5d3e 100644 --- a/pkg/cms/verifier.go +++ b/pkg/cms/verifier.go @@ -980,19 +980,29 @@ func constantTimeCompareBigInt(a, b *big.Int) bool { return subtle.ConstantTimeCompare(aPadded, bPadded) == 1 } -// matchesSID verifies that SignerIdentifier matches certificate -// Supports both issuerAndSerialNumber and subjectKeyIdentifier -// Uses constant-time comparison for cryptographic values to prevent timing attacks +// matchesSID verifies that SignerIdentifier matches certificate. +// Supports both issuerAndSerialNumber and subjectKeyIdentifier per RFC 5652 +// §5.3. Uses constant-time comparison for cryptographic values to prevent +// timing attacks. func matchesSID(sidRaw asn1.RawValue, cert *x509.Certificate) bool { - // Check if this is a subjectKeyIdentifier (IMPLICIT [0] OCTET STRING) + // SubjectKeyIdentifier form: [0] IMPLICIT OCTET STRING. + // + // RFC 5652's ASN.1 module declares IMPLICIT TAGS, so the [0] tag + // REPLACES the OCTET STRING tag — the raw SKI bytes live directly + // inside the [0] tag with no nested OCTET STRING wrapping. That makes + // the encoding primitive (IsCompound == false) and sidRaw.Bytes the + // SKI value itself. + // + // An EXPLICIT-wrapped variant (`A0 04 `, + // IsCompound==true) is non-canonical under DER and would create a + // malleability surface, so we reject it. This also matches what + // OpenSSL and github.com/github/ietf-cms emit. if sidRaw.Tag == 0 && sidRaw.Class == asn1ClassContext { - // This is a subjectKeyIdentifier - var keyID []byte - rest, err := asn1.Unmarshal(sidRaw.Bytes, &keyID) - if err != nil || len(rest) > 0 { + if sidRaw.IsCompound { return false } - // Use constant-time comparison for key IDs + keyID := sidRaw.Bytes + // Use constant-time comparison for key IDs. if len(cert.SubjectKeyId) == 0 || len(keyID) != len(cert.SubjectKeyId) { return false } From a50b135447b5bc953ef0ec756b4bd3f637fffced Mon Sep 17 00:00:00 2001 From: jamestexas <18285880+jamestexas@users.noreply.github.com> Date: Wed, 17 Jun 2026 11:32:58 -0600 Subject: [PATCH 4/4] review(pr14): address Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes for issues Copilot flagged on PR #14: 1. Makefile help text: overnight-fuzz claimed "~12h total" but the target list has 16 entries × 1h = ~16h. Update help line to match the code comment. 2. mutation-test target: previously `which gremlins` then bare `gremlins`, which fails on fresh environments where GOBIN isn't on PATH even after `go install` succeeds. Resolve the install path via `go env GOBIN`/`GOPATH/bin` and invoke the binary by absolute path. 3. govulncheck target: same fix as (2). 4. cms_builder_test.go: clarify the SubjectKeyId derivation comment. RFC 5280 §4.2.1.2 method 1 hashes the *contents* of the subjectPublicKey BIT STRING (raw key bytes), not the DER encoding of the BIT STRING itself. The code was already correct; the comment is now accurate. Co-Authored-By: Claude Opus 4.7 --- Makefile | 18 +++++++++++++----- pkg/cms/cms_builder_test.go | 11 +++++++---- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 327482c..7702fa0 100644 --- a/Makefile +++ b/Makefile @@ -28,7 +28,7 @@ help: @echo "Audit-level test targets:" @echo " make long-fuzz - Run every fuzz target for FUZZTIME each" @echo " (default 10m; override with FUZZTIME=30m or 1h)" - @echo " make overnight-fuzz - long-fuzz with FUZZTIME=1h per target (~12h total)" + @echo " make overnight-fuzz - long-fuzz with FUZZTIME=1h per target (~16h total)" @echo " make mutation-test - Run gremlins mutation testing on pkg/cms" @echo " make govulncheck - Surface stdlib/dependency CVEs" @echo "" @@ -102,17 +102,25 @@ long-fuzz: overnight-fuzz: @$(MAKE) long-fuzz FUZZTIME=1h +# go-installed tools live under GOBIN (or GOPATH/bin). Resolve absolute +# paths so these targets work on fresh environments where that directory +# isn't on PATH. +GOBIN_DIR := $(shell $(GOCMD) env GOBIN) +ifeq ($(strip $(GOBIN_DIR)),) +GOBIN_DIR := $(shell $(GOCMD) env GOPATH)/bin +endif + # Mutation testing — verifies that the test suite would actually fail if # someone introduced a logic bug. Surviving mutants point at test gaps. mutation-test: - @which gremlins >/dev/null 2>&1 || $(GOCMD) install github.com/go-gremlins/gremlins/cmd/gremlins@latest + @test -x $(GOBIN_DIR)/gremlins || $(GOCMD) install github.com/go-gremlins/gremlins/cmd/gremlins@latest @echo "Running gremlins on pkg/cms (this can take several minutes)." - @cd pkg/cms && GOWORK=off gremlins unleash --timeout-coefficient 5 + @cd pkg/cms && GOWORK=off $(GOBIN_DIR)/gremlins unleash --timeout-coefficient 5 # govulncheck — surface stdlib and dependency CVEs reachable from our # call graph. Mirrors the CI check. govulncheck: - @which govulncheck >/dev/null 2>&1 || $(GOCMD) install golang.org/x/vuln/cmd/govulncheck@latest - govulncheck ./... + @test -x $(GOBIN_DIR)/govulncheck || $(GOCMD) install golang.org/x/vuln/cmd/govulncheck@latest + $(GOBIN_DIR)/govulncheck ./... .DEFAULT_GOAL := help diff --git a/pkg/cms/cms_builder_test.go b/pkg/cms/cms_builder_test.go index aa0989e..61a658d 100644 --- a/pkg/cms/cms_builder_test.go +++ b/pkg/cms/cms_builder_test.go @@ -88,10 +88,13 @@ func newBuilderSigner(tb testing.TB) (*x509.Certificate, ed25519.PrivateKey, *x5 tb.Fatalf("ed25519.GenerateKey: %v", err) } // Compute a PKIX-standard SubjectKeyId (RFC 5280 §4.2.1.2 method 1: - // 160-bit SHA-1 of the DER-encoded subjectPublicKey BIT STRING value). - // SHA-1 is the IETF-blessed input here — it's an identifier, not a - // security primitive. Go's auto-SKI only triggers for IsCA=true certs, - // so we set it explicitly. + // 160-bit SHA-1 of the *contents* of the subjectPublicKey BIT STRING — + // i.e. the raw public-key bytes, excluding the BIT STRING tag, length, + // and unused-bits prefix. Ed25519 public keys ARE those raw bytes, so + // hashing priv.Public() directly matches the RFC. SHA-1 here is the + // IETF-blessed input — it's an identifier, not a security primitive. + // Go's auto-SKI only triggers for IsCA=true certs, so we set it + // explicitly. skiSum := sha1.Sum(priv.Public().(ed25519.PublicKey)) // #nosec G401 -- see above tmpl := &x509.Certificate{ SerialNumber: big.NewInt(0xc115),