From c303586734dccfe96cdf1e331f4f0456f62aa11c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 1 May 2026 23:00:58 -0700 Subject: [PATCH] buildkite: enforce freshness window on webhook signatures VerifySignature previously accepted any cryptographically valid "timestamp=,signature=" header regardless of how old the timestamp was. An attacker who captured a single signed delivery could replay it indefinitely, creating duplicate status events and unbounded growth in the events table. Reject signatures whose timestamp is more than MaxSignatureAge (5 minutes) from the local clock in either direction. The symmetric bound also defeats implausibly future-dated stamps that would otherwise mint a long replay window. The clock is read through a package-level timeNow var so tests can pin it deterministically; the existing fixed-timestamp test now stubs the clock and a new stale case covers the rejection path. --- internal/buildkite/buildkite.go | 45 +++++++++++++++++++++------- internal/buildkite/buildkite_test.go | 21 +++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/internal/buildkite/buildkite.go b/internal/buildkite/buildkite.go index 1427574..893a5f6 100644 --- a/internal/buildkite/buildkite.go +++ b/internal/buildkite/buildkite.go @@ -288,15 +288,28 @@ const ( WebhookModeSignature WebhookMode = "signature" ) +// MaxSignatureAge bounds how far the signed timestamp may be from +// the local clock before VerifySignature rejects the request as +// stale (or implausibly future-dated). Without this, an attacker who +// captures one valid signed delivery can replay it indefinitely, +// generating duplicate status events and unbounded growth in the +// events table. Five minutes matches Buildkite's own published +// guidance for verifying their signatures and is the same window +// Stripe et al. use; it absorbs realistic clock skew while keeping +// the replay surface tight. +const MaxSignatureAge = 5 * time.Minute + +// timeNow is the clock VerifySignature consults for freshness. A +// package-level var so tests can pin "now" deterministically without +// touching the system clock or threading a clock argument through +// every caller. +var timeNow = time.Now + // VerifySignature validates the X-Buildkite-Signature header against // secret using the documented "." HMAC-SHA256 -// scheme. Returns nil when the header is well-formed and the digest -// matches; any other condition returns an error. -// -// We deliberately do NOT enforce a freshness window on timestamp: -// callers in practice consume idempotent or storage-deduplicated -// events, so a replayed event is at worst a duplicate publish. -// Callers that need stricter freshness should layer it on top. +// scheme. Returns nil when the header is well-formed, the digest +// matches, and the signed timestamp is within MaxSignatureAge of +// now; any other condition returns an error. // // The header format is "timestamp=,signature=". func VerifySignature(header, secret string, body []byte) error { @@ -325,12 +338,22 @@ func VerifySignature(header, secret string, body []byte) error { if ts == "" || sig == "" { return errors.New("malformed signature header") } - // Sanity-check the timestamp is a parseable int. The value - // itself isn't validated against the clock (see comment above), - // but a non-numeric timestamp is structurally invalid. - if _, err := strconv.ParseInt(ts, 10, 64); err != nil { + tsInt, err := strconv.ParseInt(ts, 10, 64) + if err != nil { return fmt.Errorf("invalid timestamp: %w", err) } + // Freshness check: reject signatures whose timestamp is more + // than MaxSignatureAge away from now in either direction. The + // symmetric bound also rejects implausibly future-dated stamps, + // which would otherwise let an attacker mint a replay window + // well into the future. + skew := timeNow().Sub(time.Unix(tsInt, 0)) + if skew < 0 { + skew = -skew + } + if skew > MaxSignatureAge { + return fmt.Errorf("signature timestamp outside freshness window (skew %s)", skew) + } mac := hmac.New(sha256.New, []byte(secret)) mac.Write([]byte(ts)) diff --git a/internal/buildkite/buildkite_test.go b/internal/buildkite/buildkite_test.go index f92c1b5..81cc397 100644 --- a/internal/buildkite/buildkite_test.go +++ b/internal/buildkite/buildkite_test.go @@ -16,6 +16,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" ) // TestVerifySignature covers the HMAC mode end to end. The reference @@ -26,12 +27,31 @@ func TestVerifySignature(t *testing.T) { body := []byte(`{"event":"build.finished"}`) const ts = "1700000000" + // Pin the clock to "now == ts" so the freshness check is + // satisfied for the in-window cases and we can dial it past the + // max-age boundary for the stale case below. + tsTime := time.Unix(1700000000, 0) + prevNow := timeNow + timeNow = func() time.Time { return tsTime } + defer func() { timeNow = prevNow }() + mac := hmac.New(sha256.New, []byte(secret)) mac.Write([]byte(ts)) mac.Write([]byte(".")) mac.Write(body) good := hex.EncodeToString(mac.Sum(nil)) + // Same body/secret/timestamp, but signed for a moment far in + // the past relative to our pinned clock. A correct verifier + // must reject it even though the HMAC itself is valid, otherwise + // captured deliveries replay forever. + const staleTS = "1600000000" + staleMac := hmac.New(sha256.New, []byte(secret)) + staleMac.Write([]byte(staleTS)) + staleMac.Write([]byte(".")) + staleMac.Write(body) + staleSig := hex.EncodeToString(staleMac.Sum(nil)) + cases := []struct { name string header string @@ -48,6 +68,7 @@ func TestVerifySignature(t *testing.T) { {"non-numeric timestamp", "timestamp=abc,signature=" + good, secret, body, true}, {"wrong signature", "timestamp=" + ts + ",signature=00", secret, body, true}, {"wrong body", "timestamp=" + ts + ",signature=" + good, secret, []byte("nope"), true}, + {"stale timestamp", "timestamp=" + staleTS + ",signature=" + staleSig, secret, body, true}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { -- 2.51.2