From e5a1cb71d1f35d8aaf17e2b6f5bcd1502f66cf85 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:45:07 +0100 Subject: [PATCH 01/12] Fix data race on issuer key cache len(m) was read outside the mutex while other goroutines wrote to the map under mu.Lock(), which is a concurrent map read/write. --- storage/storage.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/storage/storage.go b/storage/storage.go index ca46abb3..34ca09a4 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -206,11 +206,12 @@ func cachedStoreIssuers(s IssuerStorage) func(context.Context, []KV) error { return fmt.Errorf("issuerStorage.AddIfNotExist(): error storing issuer data in the underlying IssuerStorage: %v", err) } for _, kv := range req { + mu.Lock() if len(m) >= maxCachedIssuerKeys { + mu.Unlock() logger.DebugExtraContext(ctx, "cachedStoreIssuers wrapper: local issuer cache full, will stop caching issuers.") return nil } - mu.Lock() m[string(kv.K)] = struct{}{} mu.Unlock() } From cee1631b08d3b82934922fddaaedef4a2cdefe1a Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:46:51 +0100 Subject: [PATCH 02/12] Make HTTPFetcher.EnableRetries honour maxRetries The retry limit was hardcoded to 10, so the caller-supplied value was silently ignored. --- internal/client/fetcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/client/fetcher.go b/internal/client/fetcher.go index 0955b891..fb920787 100644 --- a/internal/client/fetcher.go +++ b/internal/client/fetcher.go @@ -75,7 +75,7 @@ func (h *HTTPFetcher) SetUserAgent(ua string) { // EnableRetries causes requests which result in a non-permanent error to be retried with up to maxRetries attempts. func (h *HTTPFetcher) EnableRetries(maxRetries uint) { - h.backOff = []backoff.RetryOption{backoff.WithBackOff(backoff.NewExponentialBackOff()), backoff.WithMaxTries(10)} + h.backOff = []backoff.RetryOption{backoff.WithBackOff(backoff.NewExponentialBackOff()), backoff.WithMaxTries(maxRetries)} } func (h HTTPFetcher) fetch(ctx context.Context, p string) ([]byte, error) { From 99f15c07d3cb84adbc44a15436761cf12eaca9a9 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:47:07 +0100 Subject: [PATCH 03/12] Wrap read error in FileFetcher.ReadEntryBundle Using %v broke the error chain, so PartialOrFullResource never saw os.ErrNotExist and the partial-to-full bundle fallback never fired. --- internal/client/fetcher.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/client/fetcher.go b/internal/client/fetcher.go index fb920787..68a565d8 100644 --- a/internal/client/fetcher.go +++ b/internal/client/fetcher.go @@ -171,7 +171,8 @@ func (f FileFetcher) ReadEntryBundle(ctx context.Context, i uint64, p uint8) ([] return PartialOrFullResource(ctx, p, func(ctx context.Context, p uint8) (r []byte, rErr error) { data, err := os.ReadFile(path.Join(f.Root, ctEntriesPath(i, p))) if err != nil { - return nil, fmt.Errorf("failed to read file: %v", err) + // Must wrap so that callers (e.g. PartialOrFullResource) can detect os.ErrNotExist. + return nil, fmt.Errorf("failed to read file: %w", err) } if f.DecompressBundles { reader, err := gzip.NewReader(bytes.NewReader(data)) From a59018a95f02e696d83d1431301d665d00f09738 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:47:43 +0100 Subject: [PATCH 04/12] Check Unmarshal error before inspecting its output in cpSigner.Sign len(rest) was tested first, so a malformed checkpoint could be reported as trailing data rather than the actual parse error. --- internal/ct/signatures.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/ct/signatures.go b/internal/ct/signatures.go index ec787cf5..0b846f78 100644 --- a/internal/ct/signatures.go +++ b/internal/ct/signatures.go @@ -151,10 +151,10 @@ func (cts *cpSigner) Sign(msg []byte) ([]byte, error) { ckpt := &tfl.Checkpoint{} rest, err := ckpt.Unmarshal(msg) - if len(rest) != 0 { - return nil, fmt.Errorf("checkpoint contains trailing data: %s", string(rest)) - } else if err != nil { + if err != nil { return nil, fmt.Errorf("ckpt.Unmarshal: %v", err) + } else if len(rest) != 0 { + return nil, fmt.Errorf("checkpoint contains trailing data: %s", string(rest)) } else if ckpt.Origin != cts.origin { return nil, fmt.Errorf("checkpoint's origin %s doesn't match signer's origin %s", ckpt.Origin, cts.origin) } From 0261aade81c449f760b08f7fdee9b0352be82705 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:47:55 +0100 Subject: [PATCH 05/12] Check ParseSI error before its unit return value The unit was tested first, so a parse failure could be reported as a bogus unit error. --- cmd/tesseract/posix/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/tesseract/posix/main.go b/cmd/tesseract/posix/main.go index e0b0ad71..2e4915dc 100644 --- a/cmd/tesseract/posix/main.go +++ b/cmd/tesseract/posix/main.go @@ -261,12 +261,12 @@ func newStorage(ctx context.Context, signer note.Signer) (st *storage.CTStorage, } antispamCacheSize, unit, error := humanize.ParseSI(*inMemoryAntispamCacheSize) - if unit != "" { - return nil, fmt.Errorf("invalid antispam cache size, used unit %q, want none", unit) - } if error != nil { return nil, fmt.Errorf("invalid antispam cache size: %v", error) } + if unit != "" { + return nil, fmt.Errorf("invalid antispam cache size, used unit %q, want none", unit) + } var extraSigners []note.Signer for _, as := range additionalSigners { From a4d2eefc764638b4246327055344f9548e6ed2e3 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:48:11 +0100 Subject: [PATCH 06/12] Never return a nil error alongside an empty chain list from Verify The ExtKeyUsageAny short-circuit ran before the empty-chain check, so an unverifiable chain could be reported as success. --- internal/lax509/verify.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/lax509/verify.go b/internal/lax509/verify.go index f0b24da2..aecb3e13 100644 --- a/internal/lax509/verify.go +++ b/internal/lax509/verify.go @@ -201,18 +201,18 @@ func Verify(c *x509.Certificate, opts VerifyOptions) (chains [][]*x509.Certifica opts.KeyUsages = []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth} } - if slices.Contains(opts.KeyUsages, x509.ExtKeyUsageAny) { - // If any key usage is acceptable, no need to check the chain for - // key usages. - return candidateChains, nil - } - if len(candidateChains) == 0 { var details []string err = x509.CertificateInvalidError{Cert: c, Reason: x509.NoValidChains, Detail: strings.Join(details, ", ")} return nil, err } + if slices.Contains(opts.KeyUsages, x509.ExtKeyUsageAny) { + // If any key usage is acceptable, no need to check the chain for + // key usages. + return candidateChains, nil + } + return candidateChains, nil } From e6e1cbcbc74c1585007c975594a9911927e4af9a Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 21:48:24 +0100 Subject: [PATCH 07/12] Declare request duration histogram in seconds The value recorded and the bucket boundaries are both in seconds, but the metric was labelled ms. --- internal/ct/handlers.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/ct/handlers.go b/internal/ct/handlers.go index e8ce8fcd..d71c082b 100644 --- a/internal/ct/handlers.go +++ b/internal/ct/handlers.go @@ -107,10 +107,9 @@ func setupMetrics() { metric.WithDescription("CT HTTP responses"), metric.WithUnit("{response}"))) - // TODO(phboneff): switch back to s, in Tessera as well. reqDuration = mustCreate(meter.Float64Histogram("tesseract.http.request.duration", metric.WithDescription("CT HTTP response duration"), - metric.WithUnit("ms"), + metric.WithUnit("s"), metric.WithExplicitBucketBoundaries(otel.SubSecondLatencyHistogramBuckets...))) notBeforeAgeUnverified = mustCreate(meter.Float64Histogram("tesseract.notbefore.age.unverified", From 5b1e4370dacd25d32ec3fa0eb45b45d1d3836930 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 22:27:43 +0100 Subject: [PATCH 08/12] Check pem.Decode result rather than a stale err in signerFromFlags pem.Decode returns no error, so the check tested the already-nil err from the preceding ReadFile and a non-PEM key file panicked on block.Bytes. --- cmd/tesseract/posix/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/tesseract/posix/main.go b/cmd/tesseract/posix/main.go index 2e4915dc..a14fdf2b 100644 --- a/cmd/tesseract/posix/main.go +++ b/cmd/tesseract/posix/main.go @@ -377,8 +377,8 @@ func signerFromFlags() crypto.Signer { os.Exit(1) } block, _ := pem.Decode(r) - if err != nil { - slog.ErrorContext(context.Background(), "Failed to parse PEM private key", slog.Any("error", err)) + if block == nil { + slog.ErrorContext(context.Background(), "Failed to parse PEM private key", slog.String("path", kf)) os.Exit(1) } k, err := x509.ParseECPrivateKey(block.Bytes) From aadd55030c38982b5d7b66aba92f6374d2e57512 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 22:46:46 +0100 Subject: [PATCH 09/12] Bounds check the source checkpoint in the GCP migration tool A short response from the source log made bits[1]/bits[2] panic with an index out of range. --- cmd/experimental/migrate/gcp/main.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/experimental/migrate/gcp/main.go b/cmd/experimental/migrate/gcp/main.go index ae03a9e9..6106f4c5 100644 --- a/cmd/experimental/migrate/gcp/main.go +++ b/cmd/experimental/migrate/gcp/main.go @@ -81,6 +81,10 @@ func main() { // TODO(AlCutter): We should be properly verifying and opening the checkpoint here with the source log's // public key. bits := strings.Split(string(sourceCP), "\n") + if len(bits) < 3 { + slog.ErrorContext(ctx, "malformed source checkpoint", slog.String("checkpoint", string(sourceCP))) + os.Exit(1) + } sourceSize, err := strconv.ParseUint(bits[1], 10, 64) if err != nil { slog.ErrorContext(ctx, "invalid CP size", slog.Any("arg", bits[1]), slog.Any("error", err)) From 79e10df7f81d2dfabe8ac86ec9a4e9f1af0315c0 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 22:47:02 +0100 Subject: [PATCH 10/12] Bounds check the source checkpoint in the POSIX migration tool A short response from the source log made bits[1]/bits[2] panic with an index out of range. --- cmd/experimental/migrate/posix/main.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/experimental/migrate/posix/main.go b/cmd/experimental/migrate/posix/main.go index 68147d18..3d1ea76f 100644 --- a/cmd/experimental/migrate/posix/main.go +++ b/cmd/experimental/migrate/posix/main.go @@ -90,6 +90,10 @@ func main() { // TODO(AlCutter): We should be properly verifying and opening the checkpoint here with the source log's // public key. bits := strings.Split(string(sourceCP), "\n") + if len(bits) < 3 { + slog.ErrorContext(ctx, "malformed source checkpoint", slog.String("checkpoint", string(sourceCP))) + os.Exit(1) + } sourceSize, err := strconv.ParseUint(bits[1], 10, 64) if err != nil { slog.ErrorContext(ctx, "invalid CP size", slog.Any("arg", bits[1]), slog.Any("error", err)) From cb93940b17df78514f2cf182b8b7723a6c510484 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 22:47:16 +0100 Subject: [PATCH 11/12] Bounds check the issuer fingerprint list in fsck A fingerprint list whose length was not a multiple of 32 made the fpRaw[:32] slice panic on data fetched from a remote log. --- cmd/fsck/main.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cmd/fsck/main.go b/cmd/fsck/main.go index a30fd19d..f24fc9f3 100644 --- a/cmd/fsck/main.go +++ b/cmd/fsck/main.go @@ -181,16 +181,20 @@ func (l *logStateCollector) checkIssuersTask(ctx context.Context, readIssuer fun } // addIssuers adds the issuers in the provided byte string to the set of issuer to be checked. -func (l *logStateCollector) addIssuers(fpRaw cryptobyte.String) { +func (l *logStateCollector) addIssuers(fpRaw cryptobyte.String) error { + if len(fpRaw)%sha256.Size != 0 { + return fmt.Errorf("chain fingerprints are %d bytes, want a multiple of %d", len(fpRaw), sha256.Size) + } var fp []byte for len(fpRaw) > 0 { - fp, fpRaw = fpRaw[:32], fpRaw[32:] + fp, fpRaw = fpRaw[:sha256.Size], fpRaw[sha256.Size:] _, existed := l.issuersSeen.LoadOrStore(string(fp), true) if !existed { logger.DebugExtraContext(context.Background(), "Found issuer", slog.String("fp", fmt.Sprintf("%x", fp))) l.issuersToCheck <- fp } } + return nil } // merkleLeafHasher returns a function which knows how to: @@ -250,7 +254,9 @@ func (l *logStateCollector) merkleLeafHasher() func(bundle []byte) ([][]byte, er if !b.ReadUint16LengthPrefixed(&fpRaw) { return nil, fmt.Errorf("failed to read chain fingerprints at entry index %d of bundle", i) } - l.addIssuers(fpRaw) + if err := l.addIssuers(fpRaw); err != nil { + return nil, fmt.Errorf("invalid chain fingerprints at entry index %d of bundle: %v", i, err) + } h := rfc6962.DefaultHasher.HashLeaf(preimage.BytesOrPanic()) r = append(r, h) From 2c7ed48c94991dc75da516477b2fe6c2e5b08922 Mon Sep 17 00:00:00 2001 From: Rob Stradling Date: Fri, 28 Aug 2026 22:47:34 +0100 Subject: [PATCH 12/12] Decode Uint24 from the offset slice rather than the start of the buffer The bounds check was against rest, but the three bytes were read from data, so any Uint24 field at a non-zero offset decoded the wrong bytes. --- internal/types/tls/tls.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/types/tls/tls.go b/internal/types/tls/tls.go index 15c50817..02f8c311 100644 --- a/internal/types/tls/tls.go +++ b/internal/types/tls/tls.go @@ -340,7 +340,7 @@ func parseField(v reflect.Value, data []byte, initOffset int, info *fieldInfo) ( if len(rest) < 3 { return offset, syntaxError{info.fieldName(), "truncated uint24"} } - v.SetUint(uint64(data[0])<<16 | uint64(data[1])<<8 | uint64(data[2])) + v.SetUint(uint64(rest[0])<<16 | uint64(rest[1])<<8 | uint64(rest[2])) offset += 3 return offset, nil case uint32Type: