diff --git a/CHANGELOG.md b/CHANGELOG.md index 6694a7c..d54c08d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version check. Adding a user, setting a password, changing an e-mail or a role on the user form, and deleting a user now ask for your own account password, as a backup does, under the same limit on wrong guesses. +- **A domain's rate limit is taken only by its own applications** (audit + M1): the level-2 domain limit was read from `MAIL FROM`, which reaches the + milter before Postfix holds it to the login, so a client that never + signed in, or another tenant's application, could use up a domain's + limit. It is now the signed-in application's domain. - **A stranger's recipient no longer writes an Inbound › Log row** (audit M2): Postfix quotes a refused recipient in its reply with the quotes removed, so `RCPT TO:<"; from= to= …"@domain>` spelled a second diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 82a12fd..d42fb6e 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -719,12 +719,14 @@ the argument. ### Stage 1 — the tenant boundary in the mail path (~7 h) → patch release -- [ ] **M1.** `overLimit` and `recordRejected` take the domain from the login +- [x] **M1.** `overLimit` and `recordRejected` take the domain from the login (`ApplicationByLogin`), return early on an empty login; the reader's two tests (stranger; other tenant) become the regression tests; an e2e sends an unauthenticated `MAIL FROM` for a domain with a limit of 1 and then the domain's own message, which must pass. security.md § Mail path records that an unauthenticated session reserves nothing. — **Opus**, 2 h. + *Done 2026-10-17:* TestRateLimitIsNotReservedByAnotherSendersMailFrom; + e2e `domain_limit_not_reserved_by_a_stranger`. - [ ] **M3.** De-duplicate recipients in `inboundSession.RcptTo` (case-folded, as the outbound session does); the milter's inserts count toward the prune trigger (a counter in the store beside `InsertInboundSpam`, the prune diff --git a/docs/security.md b/docs/security.md index 81f02f9..9c7fd6b 100644 --- a/docs/security.md +++ b/docs/security.md @@ -399,6 +399,16 @@ operator can act on it. ask for it at all, an application with a client-address list is refused when no address is known, and neither milter replies anything but "continue" to a message it lets through. `test/e2e` sends two `MAIL FROM` on one connection. +- **A level-2 limit is reserved only by its own senders.** The domain limit + is the domain of the signed-in application, never the `MAIL FROM` + domain, and a session with no login reserves nothing. `MAIL FROM` reaches + the journal milter before Postfix holds it to the login (with + `smtpd_delay_reject` at its default no sender restriction runs at MAIL), + so until `2.14.4` a stranger on 465/587 who never signed in, or another + tenant's application, could name a domain there and take its slots for + the in-flight TTL - and have a `limit` refusal written under it (security + audit 2026-10-17, M1). `test/e2e` names a limited domain in a stranger's + `MAIL FROM` and then sends the domain's own message. - **The upstream's answer is an update of a row, not a row** (inbound-upstream-result, `2.10.0`). A delivery line of mail accepted on port 25 changes the result of that message's journal rows and adds diff --git a/internal/milter/milter_test.go b/internal/milter/milter_test.go index a7deba5..b5d8855 100644 --- a/internal/milter/milter_test.go +++ b/internal/milter/milter_test.go @@ -221,6 +221,11 @@ func TestBracedMacros(t *testing.T) { } } +// app1AtExample is the application the rate-limit tests sign in as: the +// domain limit is its login's domain's, whatever MAIL FROM says (audit +// 2026-10-17, M1). +var app1AtExample = map[string]store.Application{"app1": {Login: "app1", DomainName: "example.com"}} + // limitIP is the client IP rate-limit tests connect from. const limitIP = "203.0.113.7" @@ -249,6 +254,7 @@ func mailFrom(t *testing.T, rec Store, ip, from, login string) milter.Response { func TestRateLimitRefusesWhenDomainOverLimit(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -276,6 +282,7 @@ func TestRateLimitRefusesWhenAppOverLimit(t *testing.T) { func TestRateLimitAllowsUnderLimit(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -291,6 +298,7 @@ func TestRateLimitAllowsUnderLimit(t *testing.T) { func TestRateLimitDomainAppliesToAnyIP(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -304,6 +312,7 @@ func TestRateLimitDomainAppliesToAnyIP(t *testing.T) { func TestRateLimitAppSkipsDomainWhenActive(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": {MaxMessages: 1, WindowSeconds: 3600}, store.RateLimitScopeApp + "|app1": appLimit(), @@ -364,6 +373,7 @@ func TestAuthIPRestrictOffAllowsAnyIP(t *testing.T) { func TestRateLimitInactiveWithoutCeiling(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": {}, // no max/window }, @@ -383,6 +393,7 @@ func TestRateLimitFailsOpenOnLookupError(t *testing.T) { func TestRateLimitFailsOpenOnCountError(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -398,6 +409,7 @@ func TestRateLimitFailsOpenOnCountError(t *testing.T) { // through, which is how the second MAIL FROM of a connection got past a limit. func TestRateLimitAppliesWithoutAClientIP(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -484,6 +496,7 @@ func mailFromIn(t *testing.T, rec Store, fl *inflight, ip, from, login string) ( func limitedRecorder(count int64) *fakeRecorder { return &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), }, @@ -603,6 +616,7 @@ func TestReservationReleasedOnAbort(t *testing.T) { // no domain reservation should linger after the refusal. func TestRefusalDoesNotLeaveDomainReservation(t *testing.T) { rec := &fakeRecorder{ + apps: app1AtExample, limits: map[string]store.RateLimit{ store.RateLimitScopeDomain + "|example.com": domainLimit(), store.RateLimitScopeApp + "|app1": appLimit(), @@ -717,3 +731,46 @@ func TestRateLimitHoldsTheNullSenderToItsLoginsDomain(t *testing.T) { t.Fatalf("a null sender over its domain's limit = %v, want TempFail (4xx)", resp) } } + +// The domain limit is the login's domain's. MAIL FROM reaches the milter +// before Postfix holds it to the login, so a stranger with no login, or +// another tenant's application, naming a victim's domain there must take +// none of its slots, and write no refusal under it (security audit +// 2026-10-17, M1). +func TestRateLimitIsNotReservedByAnotherSendersMailFrom(t *testing.T) { + victim := func() *fakeRecorder { + return &fakeRecorder{ + apps: map[string]store.Application{ + "victim-app": {Login: "victim-app", DomainName: "victim.example"}, + "tenant-a": {Login: "tenant-a", DomainName: "tenant-a.example"}, + }, + limits: map[string]store.RateLimit{ + store.RateLimitScopeDomain + "|victim.example": {MaxMessages: 1, WindowSeconds: 3600}, + }, + counts: map[string]int64{}, + } + } + for name, login := range map[string]string{"a stranger with no login": "", "another tenant's application": "tenant-a"} { + t.Run(name, func(t *testing.T) { + rec := victim() + fl := &inflight{} + for i := 0; i < 5; i++ { + if _, resp := mailFromIn(t, rec, fl, limitIP, "x@victim.example", login); resp != milter.RespContinue { + t.Fatalf("MAIL FROM %d = %v, want Continue", i, resp) + } + } + if n := fl.count(store.RateLimitScopeDomain+"|victim.example", time.Now().Add(-time.Hour)); n != 0 { + t.Errorf("%d slots of victim.example taken", n) + } + // The victim's own application still has its one message. + if _, resp := mailFromIn(t, rec, fl, limitIP, "news@victim.example", "victim-app"); resp != milter.RespContinue { + t.Errorf("the victim's own message = %v, want Continue", resp) + } + for _, r := range rec.rejected { + if r.Domain == "victim.example" { + t.Errorf("a refusal was written under the victim's domain: %+v", r) + } + } + }) + } +} diff --git a/internal/milter/ratelimit.go b/internal/milter/ratelimit.go index f6cc0be..24d0c47 100644 --- a/internal/milter/ratelimit.go +++ b/internal/milter/ratelimit.go @@ -1,7 +1,9 @@ package milter import ( + "errors" "log" + "strings" "time" "github.com/mixeme/selfpost/internal/store" @@ -38,15 +40,19 @@ func (s *session) overLimit() bool { } } - domain := domainOf(s.from) - if domain == "" && s.login != "" { - // MAIL FROM:<> has no domain to read; the login has one. A null - // sender is held to its application's domain like any other message - // of it (review of audit stage 2). - if a, err := s.rec.ApplicationByLogin(s.login); err == nil { - domain = a.DomainName - } + // The domain is the login's, never the envelope's. MAIL FROM reaches + // this milter before Postfix has held it to the login - with + // smtpd_delay_reject at its default no sender restriction runs at MAIL + // (Postfix 3.10.13, src/smtpd/smtpd.c, mail_cmd()) - so a domain read + // from it let a stranger on 465/587, with no login at all, or another + // tenant's application reserve a victim domain's slots (security audit + // 2026-10-17, M1). A session without a login can deliver nothing on + // those ports and reserves nothing; the send log counts a domain's + // messages by the login's domain too (store.CountMessages). + if s.login == "" { + return false } + domain := s.loginDomain() if domain == "" { return false } @@ -99,7 +105,7 @@ func (s *session) releaseReservations() { // and never affects the response. func (s *session) recordRejected(reason string) { err := s.rec.InsertRejected(store.SendLogEntry{ - Domain: domainOf(s.from), + Domain: s.loginDomain(), AppLogin: s.login, From: s.from, Reason: reason, @@ -109,3 +115,19 @@ func (s *session) recordRejected(reason string) { log.Printf("journal-milter: record rejected %s: %v", s.from, err) } } + +// loginDomain is the domain of the session's application, "" without a login +// or when the store cannot say (fail-open, as overLimit is). +func (s *session) loginDomain() string { + if s.login == "" { + return "" + } + a, err := s.rec.ApplicationByLogin(s.login) + if err != nil { + if !errors.Is(err, store.ErrApplicationNotFound) { + log.Printf("journal-milter: domain of application %q: %v (fail-open)", s.login, err) + } + return "" + } + return strings.ToLower(a.DomainName) +} diff --git a/test/e2e/limit_reserve_test.go b/test/e2e/limit_reserve_test.go new file mode 100644 index 0000000..2f35e9f --- /dev/null +++ b/test/e2e/limit_reserve_test.go @@ -0,0 +1,97 @@ +package e2e + +import ( + "crypto/tls" + "fmt" + "net" + "net/http" + "net/smtp" + "net/url" + "testing" + "time" +) + +// limitDomain is a sending domain of its own for the domain-limit case, so +// that its ceiling of one touches no other step. +const limitDomain = "limit.e2e.test" + +// testDomainLimitNotReservedByAStranger is the "done when" of the security +// audit's M1 (2026-10-17): the level-2 domain limit is the login's domain's. +// MAIL FROM reaches the journal milter before Postfix holds it to a login, so +// a client with no login naming a domain there used to take that domain's +// slots. Here a domain has a limit of one message an hour; a stranger opens +// the submission port, names the domain in MAIL FROM three times without +// signing in, and the domain's own application still sends its one message - +// and is refused the second, so the limit is in force. +func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) { + domainID, err := sc.panel.addDomain(limitDomain) + if err != nil { + t.Fatalf("add %s: %v", limitDomain, err) + } + login, password, err := sc.panel.addApplication(domainID, "limit-app", "wildcard", "") + if err == nil { + err = waitForSenderMap(login, password, "alerts@"+limitDomain) + } + if err != nil { + t.Fatalf("add the application: %v", err) + } + resp, body, err := sc.panel.postForm("/outbound/domains/"+domainID+"/settings/ratelimit", url.Values{ + "mode": {"manual"}, "max_messages": {"1"}, "window_seconds": {"3600"}, + }) + if err != nil { + t.Fatal(err) + } + if resp.StatusCode != http.StatusOK || resp.Request.URL.Path != "/outbound/domains/"+domainID { + t.Fatalf("save the domain limit: status %d, landed on %s:\n%.800s", resp.StatusCode, resp.Request.URL.Path, body) + } + + if err := strangerMailFrom("x@"+limitDomain, 3); err != nil { + t.Fatalf("the stranger's MAIL FROM: %v", err) + } + + send := func(token string) sendResult { + return attemptSend(sendAttempt{ + authLogin: login, authPassword: password, + from: "alerts@" + limitDomain, to: recipient, subject: "e2e " + uniqueToken(token), body: "ok", + }) + } + if res := send("m1-own"); !res.ok() { + t.Fatalf("the domain's own message after a stranger's MAIL FROM: %v", res.firstErr()) + } + if err := send("m1-second").refusedAt("MAIL", 451, "4.7.1 Service unavailable"); err != nil { + t.Fatalf("the second message over the domain's limit of one: %v", err) + } +} + +// strangerMailFrom opens the implicit-TLS submission port, says EHLO and +// sends MAIL FROM: n times with RSET between, never signing in. Each +// MAIL FROM is answered 250: the journal milter continues, and Postfix +// refuses such a client only at RCPT (smtpd_delay_reject). +func strangerMailFrom(from string, n int) error { + conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{ + InsecureSkipVerify: true, + ServerName: selfpostHostname, + }) + if err != nil { + return err + } + c, err := smtp.NewClient(conn, selfpostHostname) + if err != nil { + _ = conn.Close() + return err + } + defer c.Close() + if err := c.Hello("stranger.e2e.test"); err != nil { + return err + } + for i := 0; i < n; i++ { + if err := c.Mail(from); err != nil { + return fmt.Errorf("MAIL FROM %d: %w", i+1, err) + } + if err := c.Reset(); err != nil { + return err + } + } + _ = c.Quit() + return nil +} diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 686233f..3d63993 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -315,6 +315,9 @@ func TestE2E(t *testing.T) { run("rate_limit_refusal_is_notified", func(t *testing.T) { testRateLimitRefusalIsNotified(t, sc) }) + run("domain_limit_not_reserved_by_a_stranger", func(t *testing.T) { + testDomainLimitNotReservedByAStranger(t, sc) + }) run("negative_sender_login_mismatch", func(t *testing.T) { testSenderLoginMismatch(t, sc) })