| index ef9b56ec0..74653fbd8 100644 |
| --- a/internal/console/sesevents.go |
| +++ b/internal/console/sesevents.go |
| @@ -16,6 +16,16 @@ |
| // host their own certificate, sign their own payload, and have it verify. |
| // The consequence here is not merely bad data: forged bounce events trip |
| // the automatic pause, so it is a remote off-switch for any tenant's email. |
| +// |
| +// A host allowlist is NOT a signer identity by itself (post-review fix): the |
| +// original check here accepted any *.amazonaws.com host, which is every AWS |
| +// customer's identity, not Amazon's — a free-tier S3 bucket or API Gateway |
| +// stage can serve a genuine, publicly-trusted certificate from a host that |
| +// satisfies "ends in amazonaws.com". validSigningCertURL now pins to SNS's |
| +// own regional endpoint shape, and verifiedCertKey separately verifies the |
| +// downloaded certificate's chain (against system roots) and hostname — |
| +// two independent checks, both required, because either alone still admits |
| +// a forgery the other closes. |
| package console |
| |
| import ( |
| @@ -34,7 +44,9 @@ import ( |
| "log" |
| "net/http" |
| "net/url" |
| + "regexp" |
| "strings" |
| + "sync" |
| "time" |
| |
| "github.com/carlosframework/platform/internal/mail" |
| @@ -45,9 +57,10 @@ import ( |
| // unauthenticated endpoint is memory exhaustion anyone can trigger. |
| const sesEventMaxBody = 256 << 10 |
| |
| -// sesCertMaxBody bounds the signing certificate this door fetches. AWS's |
| -// SNS certificates are a few KB; anything vastly larger than that is not a |
| -// certificate this code needs to finish reading. |
| +// sesCertMaxBody bounds the signing certificate this door fetches (and the |
| +// SubscribeURL response body it discards). AWS's SNS certificates are a few |
| +// KB; anything vastly larger than that is not a certificate this code needs |
| +// to finish reading. |
| const sesCertMaxBody = 64 << 10 |
| |
| // sesFetchTimeout bounds both outbound calls this door makes — fetching the |
| @@ -56,18 +69,34 @@ const sesCertMaxBody = 64 << 10 |
| // far end costs one request this long, not a goroutine forever. |
| const sesFetchTimeout = 10 * time.Second |
| |
| -// snsCertHost is the only host family a signing certificate may come from. |
| -const snsCertHost = "amazonaws.com" |
| +// snsHostPattern is the ONLY host shape a signing certificate — or a |
| +// SubscribeURL — may come from: an SNS regional endpoint, |
| +// "sns.<region>.amazonaws.com". This is deliberately narrower than "any |
| +// amazonaws.com host": that wider family is not Amazon's identity, it is |
| +// every AWS customer's — an S3 bucket (evil-bucket.s3.amazonaws.com), an API |
| +// Gateway stage (xyz.execute-api.eu-west-1.amazonaws.com), or any of the |
| +// hundreds of other services that mint a genuine, publicly-trusted |
| +// certificate on an *.amazonaws.com host that a free-tier account can stand |
| +// up in minutes. Accepting any of those reduces this door's whole security |
| +// model to the secrecy of the topic ARN, which is not a secret — it appears |
| +// in IaC and logs. |
| +var snsHostPattern = regexp.MustCompile(`^sns\.[a-z0-9-]+\.amazonaws\.com$`) |
| |
| -// validSigningCertURL refuses anything that is not an https URL on an |
| -// amazonaws.com host. |
| +// validSigningCertURL refuses anything that is not an https URL on an SNS |
| +// regional host. It is also reused, unchanged, to validate SubscribeURL |
| +// before that URL is ever fetched (IMPORTANT-1 fix): a verified signature |
| +// only proves AWS put some string in that field, not that the string points |
| +// somewhere safe to GET, and a real SubscribeURL is itself always an SNS |
| +// regional URL, so the same pattern is the right check for both. |
| // |
| -// The suffix check is LABEL-AWARE. A bare strings.HasSuffix would accept |
| -// "evilamazonaws.com" — it genuinely ends with the string "amazonaws.com", |
| -// with no dot separating the two, so it is a completely different |
| -// registered domain that merely happens to end the right way. Requiring |
| -// either an exact match or a "."-prefixed suffix is what tells a real AWS |
| -// subdomain apart from that lookalike. |
| +// The host check is LABEL-AWARE. A bare strings.HasSuffix(host, |
| +// "amazonaws.com") would accept "evilamazonaws.com" — it genuinely ends |
| +// with the string "amazonaws.com", with no dot separating the two, so it is |
| +// a completely different registered domain that merely happens to end the |
| +// right way. Anchoring the pattern on both ends (and requiring the "sns." |
| +// label specifically) is what tells a real AWS subdomain apart from that |
| +// lookalike, and from names like "sns.eu-west-1.amazonaws.com.evil.com" |
| +// that merely CONTAIN the right substring in the middle. |
| func validSigningCertURL(raw string) error { |
| u, err := url.Parse(raw) |
| if err != nil { |
| @@ -77,8 +106,8 @@ func validSigningCertURL(raw string) error { |
| return fmt.Errorf("sns: SigningCertURL is not https") |
| } |
| host := strings.ToLower(u.Hostname()) |
| - if host != snsCertHost && !strings.HasSuffix(host, "."+snsCertHost) { |
| - return fmt.Errorf("sns: SigningCertURL host %q is not an AWS host", host) |
| + if !snsHostPattern.MatchString(host) { |
| + return fmt.Errorf("sns: SigningCertURL host %q is not an SNS host", host) |
| } |
| // A traversal in the path can walk off the certificate route on a host |
| // that serves more than certificates. |
| @@ -155,16 +184,18 @@ func (e snsEnvelope) canonicalString() string { |
| // GET in dispatch) — otherwise a stranger's POST decides who this |
| // console makes outbound requests to, which is SSRF wearing an |
| // AWS-shaped alibi. |
| -// 3. Validate SigningCertURL's shape — https, an amazonaws.com host, |
| +// 3. Validate SigningCertURL's shape — https, an SNS regional host, |
| // label-aware, no path traversal — before it is ever fetched. |
| -// 4. Verify the signature over SNS's canonical string. Nothing past this |
| -// point trusts a single field of the message, including SubscribeURL, |
| -// until this has succeeded. |
| -// 5. Dispatch: confirm a subscription, or parse a notification. Anything |
| -// this deployment deliberately does not act on still answers 200 — |
| -// SNS retries non-2xx, so refusing an unknown case with a failure |
| -// status would turn one unrecognised field into an unbounded retry |
| -// flood against the console. |
| +// 4. Verify the signature over SNS's canonical string, which includes |
| +// fetching the certificate and checking ITS chain and hostname too. |
| +// Nothing past this point trusts a single field of the message, |
| +// including SubscribeURL, until this has succeeded. |
| +// 5. Dispatch: confirm a subscription (after running its SubscribeURL |
| +// through the SAME host check as step 3), or parse a notification. |
| +// Anything this deployment deliberately does not act on still answers |
| +// 200 — SNS retries non-2xx, so refusing an unknown case with a |
| +// failure status would turn one unrecognised field into an unbounded |
| +// retry flood against the console. |
| func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) { |
| // 1. Bound the body first. |
| r.Body = http.MaxBytesReader(w, r.Body, sesEventMaxBody) |
| @@ -205,9 +236,15 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) { |
| // verification" (verifySNSSignature, which fetches the certificate over |
| // the network). The seam is signature verification ONLY: both checks |
| // above already ran unconditionally, for real, regardless of the seam. |
| + // |
| + // The real path is wrapped in a closure so it can carry the request's |
| + // own context (matching confirmSNSSubscription below) rather than |
| + // context.Background() — without changing the seam's own type, which |
| + // stays exactly func(snsEnvelope) error so a test's assignment to |
| + // s.snsVerify is unaffected. |
| verify := s.snsVerify |
| if verify == nil { |
| - verify = s.verifySNSSignature |
| + verify = func(e snsEnvelope) error { return s.verifySNSSignature(r.Context(), e) } |
| } |
| if err := verify(env); err != nil { |
| log.Printf("console: ses events: signature verification failed: %v", err) |
| @@ -218,8 +255,27 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) { |
| // 5. Dispatch. |
| switch env.Type { |
| case "SubscriptionConfirmation", "UnsubscribeConfirmation": |
| - // Handled, not ignored: without confirming, SNS never delivers |
| - // another event to this endpoint. |
| + // IMPORTANT-1 fix: SubscribeURL gets the SAME pinned-host check as |
| + // SigningCertURL before it is fetched. A verified signature only |
| + // proves AWS put some string in this field — it says nothing about |
| + // where that string points — so without this an authentic message |
| + // can still carry a SubscribeURL aimed at an arbitrary host, |
| + // including link-local metadata endpoints. |
| + if err := validSigningCertURL(env.SubscribeURL); err != nil { |
| + log.Printf("console: ses events: refusing SubscribeURL: %v", err) |
| + jsonError(w, http.StatusBadRequest, "invalid SubscribeURL") |
| + return |
| + } |
| + // UnsubscribeConfirmation's SubscribeURL is, by SNS's own naming, |
| + // actually a RE-subscribe link: fetching it here means an operator |
| + // who unsubscribes this topic from the SNS console finds the |
| + // endpoint re-subscribed within seconds, the next time SNS sends an |
| + // UnsubscribeConfirmation. Defensible — this deployment always |
| + // wants to be subscribed to its own mail-events topic, so an |
| + // unsubscribe is far more likely to be SNS housekeeping or an |
| + // operator mistake than an intentional retirement — but it is not |
| + // what "Unsubscribe" reads as, so it is named here rather than left |
| + // to be discovered. |
| ctx, cancel := context.WithTimeout(r.Context(), sesFetchTimeout) |
| defer cancel() |
| if err := s.confirmSNSSubscription(ctx, env.SubscribeURL); err != nil { |
| @@ -252,11 +308,12 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) { |
| } |
| } |
| |
| -// confirmSNSSubscription GETs the verified SubscribeURL. It is reached only |
| -// after the enclosing message's signature has verified, which is what makes |
| -// trusting this URL safe: SubscribeURL is itself one of the fields the |
| -// signature covers, so an authentic signature over it means AWS is the one |
| -// who put it there. |
| +// confirmSNSSubscription GETs the verified, host-checked SubscribeURL. It is |
| +// reached only after the enclosing message's signature has verified AND |
| +// SubscribeURL has passed validSigningCertURL — the signature proves AWS |
| +// put the string there, the host check proves the string points at SNS |
| +// itself rather than somewhere this console should never fetch on a |
| +// stranger's behalf. |
| func (s *Server) confirmSNSSubscription(ctx context.Context, subscribeURL string) error { |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, subscribeURL, nil) |
| if err != nil { |
| @@ -274,52 +331,146 @@ func (s *Server) confirmSNSSubscription(ctx context.Context, subscribeURL string |
| return nil |
| } |
| |
| -// verifySNSSignature is the real (non-test-seam) verification path: fetch |
| -// the certificate SigningCertURL names — already validated as an |
| -// amazonaws.com https URL by the caller — parse its RSA public key, and |
| -// check the signature over the message's own canonical string. |
| +// snsCertCache caches a signing certificate's verified RSA public key, |
| +// keyed on its (already pinned-host-validated) URL — IMPORTANT-2 fix. |
| +// Without it, every request bearing the deployment's TopicArn — which is |
| +// not a secret, it appears in IaC and logs — triggers a fresh outbound |
| +// HTTPS fetch and a full certificate-chain verification: unauthenticated |
| +// outbound amplification, one connection out per request in. The AWS SDKs |
| +// cache verification certs by URL for the same reason. |
| // |
| -// SNS signs with SHA1withRSA under SignatureVersion "1" (the historical |
| -// default, and what SNS still sends unless a topic opts into version "2", |
| -// SHA256withRSA). Both are verified here; an unrecognised version is |
| -// refused rather than guessed at. |
| -func (s *Server) verifySNSSignature(env snsEnvelope) error { |
| - ctx, cancel := context.WithTimeout(context.Background(), sesFetchTimeout) |
| +// A value type with a nil-safe zero value on purpose: Server embeds one |
| +// directly (no constructor call needed), the same way ceClientOnce |
| +// (server.go) needs no init. |
| +type snsCertCache struct { |
| + mu sync.Mutex |
| + keys map[string]*rsa.PublicKey |
| +} |
| + |
| +func (c *snsCertCache) get(certURL string) (*rsa.PublicKey, bool) { |
| + c.mu.Lock() |
| + defer c.mu.Unlock() |
| + pub, ok := c.keys[certURL] |
| + return pub, ok |
| +} |
| + |
| +func (c *snsCertCache) put(certURL string, pub *rsa.PublicKey) { |
| + c.mu.Lock() |
| + defer c.mu.Unlock() |
| + if c.keys == nil { |
| + c.keys = map[string]*rsa.PublicKey{} |
| + } |
| + c.keys[certURL] = pub |
| +} |
| + |
| +// verifiedCertKey returns the RSA public key for a signing certificate URL |
| +// — already pinned to an SNS regional host by the caller (validSigningCertURL) |
| +// — using the cache on a hit, and fetching plus fully verifying the |
| +// certificate on a miss. |
| +func (s *Server) verifiedCertKey(ctx context.Context, certURL string) (*rsa.PublicKey, error) { |
| + if pub, ok := s.snsCerts.get(certURL); ok { |
| + return pub, nil |
| + } |
| + pub, err := fetchVerifiedCertKey(ctx, certURL) |
| + if err != nil { |
| + return nil, err |
| + } |
| + s.snsCerts.put(certURL, pub) |
| + return pub, nil |
| +} |
| + |
| +// fetchVerifiedCertKey downloads the certificate at certURL and returns its |
| +// RSA public key, but only once the certificate has passed BOTH of two |
| +// independent checks (CRITICAL-2 fix) — neither is sufficient alone: |
| +// |
| +// - Its chain verifies against the system root pool. Without this, a |
| +// certificate that merely PARSES is trusted — a self-signed certificate |
| +// generated on an attacker's laptop, or any other certificate not |
| +// actually issued by a CA anyone trusts, would verify a forged |
| +// signature. |
| +// - It is valid for the EXACT hostname certURL named. Without this, a |
| +// validly-issued certificate for a completely different (but still |
| +// amazonaws.com-hosted) service could be swapped in for SNS's own — |
| +// chain verification alone says "some CA vouches for this cert", not |
| +// "this cert speaks for SNS". |
| +// |
| +// certURL is assumed already checked by validSigningCertURL — this function |
| +// re-derives the hostname from it rather than trusting a caller-supplied |
| +// one, so there is exactly one source of truth for "which host this |
| +// certificate must be valid for". |
| +func fetchVerifiedCertKey(ctx context.Context, certURL string) (*rsa.PublicKey, error) { |
| + u, err := url.Parse(certURL) |
| + if err != nil { |
| + return nil, fmt.Errorf("sns: unparseable SigningCertURL: %w", err) |
| + } |
| + hostname := u.Hostname() |
| + |
| + fctx, cancel := context.WithTimeout(ctx, sesFetchTimeout) |
| defer cancel() |
| - req, err := http.NewRequestWithContext(ctx, http.MethodGet, env.SigningCertURL, nil) |
| + req, err := http.NewRequestWithContext(fctx, http.MethodGet, certURL, nil) |
| if err != nil { |
| - return fmt.Errorf("sns: building cert request: %w", err) |
| + return nil, fmt.Errorf("sns: building cert request: %w", err) |
| } |
| resp, err := http.DefaultClient.Do(req) |
| if err != nil { |
| - return fmt.Errorf("sns: fetching signing cert: %w", err) |
| + return nil, fmt.Errorf("sns: fetching signing cert: %w", err) |
| } |
| defer resp.Body.Close() |
| if resp.StatusCode != http.StatusOK { |
| - return fmt.Errorf("sns: signing cert fetch status %d", resp.StatusCode) |
| + return nil, fmt.Errorf("sns: signing cert fetch status %d", resp.StatusCode) |
| } |
| certPEM, err := io.ReadAll(io.LimitReader(resp.Body, sesCertMaxBody)) |
| if err != nil { |
| - return fmt.Errorf("sns: reading signing cert: %w", err) |
| + return nil, fmt.Errorf("sns: reading signing cert: %w", err) |
| } |
| block, _ := pem.Decode(certPEM) |
| if block == nil { |
| - return fmt.Errorf("sns: signing cert is not PEM") |
| + return nil, fmt.Errorf("sns: signing cert is not PEM") |
| } |
| cert, err := x509.ParseCertificate(block.Bytes) |
| if err != nil { |
| - return fmt.Errorf("sns: parsing signing cert: %w", err) |
| + return nil, fmt.Errorf("sns: parsing signing cert: %w", err) |
| + } |
| + // Chain (system roots) and hostname, in one call: nil Roots defaults to |
| + // the system pool, and DNSName additionally requires the cert to be |
| + // valid for this exact host. |
| + if _, err := cert.Verify(x509.VerifyOptions{DNSName: hostname}); err != nil { |
| + return nil, fmt.Errorf("sns: signing cert failed chain/hostname verification: %w", err) |
| } |
| pub, ok := cert.PublicKey.(*rsa.PublicKey) |
| if !ok { |
| - return fmt.Errorf("sns: signing cert public key is not RSA") |
| + return nil, fmt.Errorf("sns: signing cert public key is not RSA") |
| + } |
| + return pub, nil |
| +} |
| + |
| +// verifySNSSignature is the real (non-test-seam) verification path: resolve |
| +// the certificate's verified public key (cached; fetched and chain-checked |
| +// on a miss) and check the signature over the message's own canonical |
| +// string. |
| +func (s *Server) verifySNSSignature(ctx context.Context, env snsEnvelope) error { |
| + pub, err := s.verifiedCertKey(ctx, env.SigningCertURL) |
| + if err != nil { |
| + return err |
| } |
| - sig, err := base64.StdEncoding.DecodeString(env.Signature) |
| + return verifySNSSignatureBytes(pub, []byte(env.canonicalString()), env.SignatureVersion, env.Signature) |
| +} |
| + |
| +// verifySNSSignatureBytes checks a base64 signature against canon using pub, |
| +// per SignatureVersion. Split out from verifySNSSignature so this — the |
| +// actual cryptographic check — can be tested with a plain in-memory RSA |
| +// keypair: no network, no certificate, no trust store involved. |
| +// |
| +// SNS signs with SHA1withRSA under SignatureVersion "1" (the historical |
| +// default, and what SNS still sends unless a topic opts into version "2", |
| +// SHA256withRSA). Both are verified here; an unrecognised version is |
| +// refused rather than guessed at. |
| +func verifySNSSignatureBytes(pub *rsa.PublicKey, canon []byte, sigVersion, sigB64 string) error { |
| + sig, err := base64.StdEncoding.DecodeString(sigB64) |
| if err != nil { |
| return fmt.Errorf("sns: signature is not base64: %w", err) |
| } |
| - canon := []byte(env.canonicalString()) |
| - switch env.SignatureVersion { |
| + switch sigVersion { |
| case "", "1": |
| sum := sha1.Sum(canon) |
| if err := rsa.VerifyPKCS1v15(pub, crypto.SHA1, sum[:], sig); err != nil { |
| @@ -331,7 +482,7 @@ func (s *Server) verifySNSSignature(env snsEnvelope) error { |
| return fmt.Errorf("sns: signature verification failed: %w", err) |
| } |
| default: |
| - return fmt.Errorf("sns: unsupported SignatureVersion %q", env.SignatureVersion) |
| + return fmt.Errorf("sns: unsupported SignatureVersion %q", sigVersion) |
| } |
| return nil |
| } |
amadan