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) {