diff --git a/internal/dmarc/service.go b/internal/dmarc/service.go index f51da0e..373dc0f 100644 --- a/internal/dmarc/service.go +++ b/internal/dmarc/service.go @@ -5,6 +5,7 @@ package dmarc import ( "sort" "strings" + "sync" "github.com/mixeme/selfpost/internal/dnscheck" "github.com/mixeme/selfpost/internal/postfix" @@ -22,6 +23,10 @@ type Service struct { maps Maps hostname string enabled bool + + // resyncMu makes a rebuild's read and write one step, as app.Service's + // (security audit 2026-10-17, W3; the review of stage 1). + resyncMu sync.Mutex } // NewService builds the DMARC ingest service. @@ -34,6 +39,8 @@ func (s *Service) Enabled() bool { return s.enabled } // Resync rebuilds the Postfix allow-list and transport maps from SQLite. func (s *Service) Resync() error { + s.resyncMu.Lock() + defer s.resyncMu.Unlock() if !s.enabled { return s.maps.RebuildDMARCMaps(postfix.DMARCMapsConfig{}) } diff --git a/internal/inbound/service.go b/internal/inbound/service.go index 09248ca..f6a0ec0 100644 --- a/internal/inbound/service.go +++ b/internal/inbound/service.go @@ -7,6 +7,7 @@ package inbound import ( "fmt" "strings" + "sync" "github.com/mixeme/selfpost/internal/postfix" "github.com/mixeme/selfpost/internal/store" @@ -21,6 +22,10 @@ type Maps interface { type Service struct { store *store.Store maps Maps + + // resyncMu makes a rebuild's read and write one step, as app.Service's + // (security audit 2026-10-17, W3; the review of stage 1). + resyncMu sync.Mutex } // NewService builds the inbound service over the shared store and Postfix maps. @@ -161,6 +166,8 @@ func (s *Service) Delete(id int64) error { // Resync rebuilds the inbound Postfix maps from SQLite. func (s *Service) Resync() error { + s.resyncMu.Lock() + defer s.resyncMu.Unlock() list, err := s.store.ListInboundDomains() if err != nil { return err diff --git a/internal/store/domains.go b/internal/store/domains.go index f99b910..8520214 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -144,7 +144,7 @@ func (s *Store) DeleteDomain(id int64) error { return fmt.Errorf("delete domain: begin: %w", err) } defer tx.Rollback() //nolint:errcheck // a no-op after Commit - if _, err := tx.Exec(`DELETE FROM dmarc_reports WHERE domain = (SELECT name FROM domains WHERE id = ?)`, id); err != nil { + if _, err := tx.Exec(`DELETE FROM dmarc_reports WHERE domain = (SELECT name FROM domains WHERE id = ?) COLLATE NOCASE`, id); err != nil { return fmt.Errorf("delete domain: its DMARC reports: %w", err) } res, err := tx.Exec("DELETE FROM domains WHERE id = ?", id) diff --git a/internal/store/inboundspam.go b/internal/store/inboundspam.go index 72d3adc..5d1b4f9 100644 --- a/internal/store/inboundspam.go +++ b/internal/store/inboundspam.go @@ -91,7 +91,7 @@ func (s *Store) countInboundWrites(n int) error { s.inboundWrites.Store(0) // The zero time removes nothing by age: this is the cap alone. if _, err := s.PruneInboundSpam(time.Time{}, inboundCapRows); err != nil { - return fmt.Errorf("cut inbound_spam_log to %d rows: %w", inboundCapRows, err) + return fmt.Errorf("the rows were written; cutting inbound_spam_log to %d rows: %w", inboundCapRows, err) } return nil } diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index c6219c2..10a1523 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -12,6 +12,7 @@ import ( "time" "github.com/mixeme/selfpost/internal/backup" + "github.com/mixeme/selfpost/internal/dmarc" "github.com/mixeme/selfpost/internal/domain" "github.com/mixeme/selfpost/internal/secretfile" "github.com/mixeme/selfpost/internal/store" @@ -450,6 +451,13 @@ func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string { if err := validate.Email(exp.DMARCRua); err != nil { return invalid("report address", err) } + // As on Domain settings: an address on this server's own name is + // only ever the domain's own hosted one (security audit 2026-10-17, + // W1; the review of stage 1 found the import path open). + if dmarc.IsHostedOnHostname(exp.DMARCRua, h.cfg.Hostname) && + !strings.EqualFold(strings.TrimSpace(exp.DMARCRua), dmarc.HostedReportAddress(h.cfg.Hostname, exp.Domain)) { + return "The export's report address is on this server's own name and is not the domain's own hosted address. Edit the file's dmarc_rua, or set it to empty and choose the address on Domain settings after the import." + } } limit := func(rl *domain.RateLimitExport) error { if rl == nil { diff --git a/internal/web/handlers/handlers_import_test.go b/internal/web/handlers/handlers_import_test.go index c65062b..55df510 100644 --- a/internal/web/handlers/handlers_import_test.go +++ b/internal/web/handlers/handlers_import_test.go @@ -38,6 +38,12 @@ func TestDomainImportIsCheckedLikeTheForms(t *testing.T) { if msg := h.checkDomainImport(&exp); msg != "" { t.Fatalf("a good export refused: %s", msg) } + // The domain's own hosted address is its own. + own := good() + own.DMARCRua = "dmarc-reports+example.com@mail.example.org" + if msg := h.checkDomainImport(&own); msg != "" { + t.Fatalf("the domain's own hosted address refused: %s", msg) + } if got := strings.Join(exp.Applications[0].AuthAllowedIPs, " "); got != "192.0.2.1 2001:db8::1" { t.Errorf("client addresses stored as %q, want as the form stores them", got) } @@ -64,6 +70,11 @@ func TestDomainImportIsCheckedLikeTheForms(t *testing.T) { "Invalid DKIM selector in export file: selector must be at most 63 characters"}, {"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" }, "Invalid report address in export file: "}, + // As on Domain settings (security audit 2026-10-17, W1). + {"report address on the hostname", func(e *domain.DomainExport) { e.DMARCRua = "ops@MAIL.example.org" }, + "The export's report address is on this server's own name"}, + {"another domain's hosted address", func(e *domain.DomainExport) { e.DMARCRua = "dmarc-reports+victim.example@mail.example.org" }, + "The export's report address is on this server's own name"}, {"no window", func(e *domain.DomainExport) { e.RateLimit.WindowSeconds = 0 }, "Invalid rate limit for the domain in export file: enter a time window greater than zero seconds"}, {"no messages", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 0 }, diff --git a/test/e2e/limit_reserve_test.go b/test/e2e/limit_reserve_test.go index 2f35e9f..cd13e2c 100644 --- a/test/e2e/limit_reserve_test.go +++ b/test/e2e/limit_reserve_test.go @@ -18,11 +18,15 @@ 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. +// a client with no login naming a domain there used to take one of that +// domain's slots and hold it for as long as its transaction stayed open. +// Here a domain has a limit of one message an hour; a stranger opens the +// submission port, names the domain in MAIL FROM without signing in and +// keeps the transaction open, and meanwhile the domain's own application +// sends its one message - and is refused the second, so the limit is in +// force. (A RSET between the stranger's MAIL FROMs would end the +// transaction, which releases the slot under the old code as well: the +// review of stage 1 found the first version of this step passing on it.) func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) { domainID, err := sc.panel.addDomain(limitDomain) if err != nil { @@ -45,9 +49,11 @@ func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) { 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 { + stranger, err := strangerHoldsMailFrom("x@" + limitDomain) + if err != nil { t.Fatalf("the stranger's MAIL FROM: %v", err) } + defer stranger() send := func(token string) sendResult { return attemptSend(sendAttempt{ @@ -63,35 +69,31 @@ func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) { } } -// 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 { +// strangerHoldsMailFrom opens the implicit-TLS submission port, says EHLO +// and MAIL FROM: without signing in, and leaves the transaction open +// until the returned func is called. The MAIL FROM is answered 250: the +// journal milter continues, and Postfix refuses such a client only at RCPT +// (smtpd_delay_reject). +func strangerHoldsMailFrom(from string) (func(), error) { conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{ InsecureSkipVerify: true, ServerName: selfpostHostname, }) if err != nil { - return err + return nil, err } c, err := smtp.NewClient(conn, selfpostHostname) if err != nil { _ = conn.Close() - return err + return nil, err } - defer c.Close() if err := c.Hello("stranger.e2e.test"); err != nil { - return err + _ = c.Close() + return nil, 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 - } + if err := c.Mail(from); err != nil { + _ = c.Close() + return nil, fmt.Errorf("MAIL FROM: %w", err) } - _ = c.Quit() - return nil + return func() { _ = c.Quit(); _ = c.Close() }, nil }