buildkite: enforce freshness window on webhook signatures #11

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.

+55 -11

2 changed files

Diff round #0

expand all

internal

buildkite

buildkite.go

buildkite_test.go

+34 -11

internal/buildkite/buildkite.go

··· 288 288 WebhookModeSignature WebhookMode = "signature" 289 289 ) 290 290 291 + // MaxSignatureAge bounds how far the signed timestamp may be from 292 + // the local clock before VerifySignature rejects the request as 293 + // stale (or implausibly future-dated). Without this, an attacker who 294 + // captures one valid signed delivery can replay it indefinitely, 295 + // generating duplicate status events and unbounded growth in the 296 + // events table. Five minutes matches Buildkite's own published 297 + // guidance for verifying their signatures and is the same window 298 + // Stripe et al. use; it absorbs realistic clock skew while keeping 299 + // the replay surface tight. 300 + const MaxSignatureAge = 5 * time.Minute 301 + 302 + // timeNow is the clock VerifySignature consults for freshness. A 303 + // package-level var so tests can pin "now" deterministically without 304 + // touching the system clock or threading a clock argument through 305 + // every caller. 306 + var timeNow = time.Now 307 + 291 308 // VerifySignature validates the X-Buildkite-Signature header against 292 309 // secret using the documented "<timestamp>.<body>" HMAC-SHA256 293 - // scheme. Returns nil when the header is well-formed and the digest 294 - // matches; any other condition returns an error. 295 - // 296 - // We deliberately do NOT enforce a freshness window on timestamp: 297 - // callers in practice consume idempotent or storage-deduplicated 298 - // events, so a replayed event is at worst a duplicate publish. 299 - // Callers that need stricter freshness should layer it on top. 310 + // scheme. Returns nil when the header is well-formed, the digest 311 + // matches, and the signed timestamp is within MaxSignatureAge of 312 + // now; any other condition returns an error. 300 313 // 301 314 // The header format is "timestamp=<unix>,signature=<hex>". 302 315 func VerifySignature(header, secret string, body []byte) error { ··· 325 338 if ts == "" || sig == "" { 326 339 return errors.New("malformed signature header") 327 340 } 328 - // Sanity-check the timestamp is a parseable int. The value 329 - // itself isn't validated against the clock (see comment above), 330 - // but a non-numeric timestamp is structurally invalid. 331 - if _, err := strconv.ParseInt(ts, 10, 64); err != nil { 341 + tsInt, err := strconv.ParseInt(ts, 10, 64) 342 + if err != nil { 332 343 return fmt.Errorf("invalid timestamp: %w", err) 333 344 } 345 + // Freshness check: reject signatures whose timestamp is more 346 + // than MaxSignatureAge away from now in either direction. The 347 + // symmetric bound also rejects implausibly future-dated stamps, 348 + // which would otherwise let an attacker mint a replay window 349 + // well into the future. 350 + skew := timeNow().Sub(time.Unix(tsInt, 0)) 351 + if skew < 0 { 352 + skew = -skew 353 + } 354 + if skew > MaxSignatureAge { 355 + return fmt.Errorf("signature timestamp outside freshness window (skew %s)", skew) 356 + } 334 357 335 358 mac := hmac.New(sha256.New, []byte(secret)) 336 359 mac.Write([]byte(ts))

+21

internal/buildkite/buildkite_test.go

··· 16 16 "net/http/httptest" 17 17 "strings" 18 18 "testing" 19 + "time" 19 20 ) 20 21 21 22 // TestVerifySignature covers the HMAC mode end to end. The reference ··· 26 27 body := []byte(`{"event":"build.finished"}`) 27 28 const ts = "1700000000" 28 29 30 + // Pin the clock to "now == ts" so the freshness check is 31 + // satisfied for the in-window cases and we can dial it past the 32 + // max-age boundary for the stale case below. 33 + tsTime := time.Unix(1700000000, 0) 34 + prevNow := timeNow 35 + timeNow = func() time.Time { return tsTime } 36 + defer func() { timeNow = prevNow }() 37 + 29 38 mac := hmac.New(sha256.New, []byte(secret)) 30 39 mac.Write([]byte(ts)) 31 40 mac.Write([]byte(".")) 32 41 mac.Write(body) 33 42 good := hex.EncodeToString(mac.Sum(nil)) 34 43 44 + // Same body/secret/timestamp, but signed for a moment far in 45 + // the past relative to our pinned clock. A correct verifier 46 + // must reject it even though the HMAC itself is valid, otherwise 47 + // captured deliveries replay forever. 48 + const staleTS = "1600000000" 49 + staleMac := hmac.New(sha256.New, []byte(secret)) 50 + staleMac.Write([]byte(staleTS)) 51 + staleMac.Write([]byte(".")) 52 + staleMac.Write(body) 53 + staleSig := hex.EncodeToString(staleMac.Sum(nil)) 54 + 35 55 cases := []struct { 36 56 name string 37 57 header string ··· 48 68 {"non-numeric timestamp", "timestamp=abc,signature=" + good, secret, body, true}, 49 69 {"wrong signature", "timestamp=" + ts + ",signature=00", secret, body, true}, 50 70 {"wrong body", "timestamp=" + ts + ",signature=" + good, secret, []byte("nope"), true}, 71 + {"stale timestamp", "timestamp=" + staleTS + ",signature=" + staleSig, secret, body, true}, 51 72 } 52 73 for _, c := range cases { 53 74 t.Run(c.name, func(t *testing.T) {

History

1 round 0 comments

mitchellh.com submitted #0 1d

1 commit

expand

af0daa9b

buildkite: enforce freshness window on webhook signatures

merge conflicts detected

expand

internal/buildkite/buildkite.go:288
internal/buildkite/buildkite_test.go:16

expand 0 comments

Configure Feed

Configure Feed

History