findings of the security review of stage 1
The review of d254a99..fd29468 (read-only, a separate agent) found all five fixes sound and nothing above low. Fixed here: - The M1 e2e step passed on the unfixed code: the stranger's RSET between MAIL FROMs ends the transaction, Postfix's mail_reset() calls milter_abort(), and the old code released the slot too. The stranger now keeps one MAIL FROM open (strangerHoldsMailFrom) while the domain's own application sends; under the old code that slot was the domain's one. - A domain import could still store a report address on the server's own name (W1's form rule only): checkDomainImport refuses one that is not the domain's own hosted address. Not a mailbox either way, but the panel would show an address the server refuses. Cases in TestDomainImportIsCheckedLikeTheForms. - DeleteDomain matches the reports' domain without regard to case. - A cut that fails after the journal rows were written says the rows were written. - dmarc.Service.Resync and inbound.Service.Resync are serialised like app.Service.Resync (W3's shape, outside its diff). Left as the review's nit: three ApplicationByLogin lookups for a refused MAIL FROM (a refusal is rare; the store is local). go test ./internal/dmarc ./internal/inbound ./internal/store; the import test; GOOS=linux go vet ./test/e2e. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,6 +5,7 @@ package dmarc
|
|||||||
import (
|
import (
|
||||||
"sort"
|
"sort"
|
||||||
"strings"
|
"strings"
|
||||||
|
"sync"
|
||||||
|
|
||||||
"github.com/mixeme/selfpost/internal/dnscheck"
|
"github.com/mixeme/selfpost/internal/dnscheck"
|
||||||
"github.com/mixeme/selfpost/internal/postfix"
|
"github.com/mixeme/selfpost/internal/postfix"
|
||||||
@@ -22,6 +23,10 @@ type Service struct {
|
|||||||
maps Maps
|
maps Maps
|
||||||
hostname string
|
hostname string
|
||||||
enabled bool
|
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.
|
// 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.
|
// Resync rebuilds the Postfix allow-list and transport maps from SQLite.
|
||||||
func (s *Service) Resync() error {
|
func (s *Service) Resync() error {
|
||||||
|
s.resyncMu.Lock()
|
||||||
|
defer s.resyncMu.Unlock()
|
||||||
if !s.enabled {
|
if !s.enabled {
|
||||||
return s.maps.RebuildDMARCMaps(postfix.DMARCMapsConfig{})
|
return s.maps.RebuildDMARCMaps(postfix.DMARCMapsConfig{})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ package inbound
|
|||||||
import (
|
import (
|
||||||
"fmt"
|
"fmt"
|
||||||
"strings"
|
"strings"
|
||||||
|
"sync"
|
||||||
|
|
||||||
"github.com/mixeme/selfpost/internal/postfix"
|
"github.com/mixeme/selfpost/internal/postfix"
|
||||||
"github.com/mixeme/selfpost/internal/store"
|
"github.com/mixeme/selfpost/internal/store"
|
||||||
@@ -21,6 +22,10 @@ type Maps interface {
|
|||||||
type Service struct {
|
type Service struct {
|
||||||
store *store.Store
|
store *store.Store
|
||||||
maps Maps
|
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.
|
// 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.
|
// Resync rebuilds the inbound Postfix maps from SQLite.
|
||||||
func (s *Service) Resync() error {
|
func (s *Service) Resync() error {
|
||||||
|
s.resyncMu.Lock()
|
||||||
|
defer s.resyncMu.Unlock()
|
||||||
list, err := s.store.ListInboundDomains()
|
list, err := s.store.ListInboundDomains()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
|
|||||||
@@ -144,7 +144,7 @@ func (s *Store) DeleteDomain(id int64) error {
|
|||||||
return fmt.Errorf("delete domain: begin: %w", err)
|
return fmt.Errorf("delete domain: begin: %w", err)
|
||||||
}
|
}
|
||||||
defer tx.Rollback() //nolint:errcheck // a no-op after Commit
|
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)
|
return fmt.Errorf("delete domain: its DMARC reports: %w", err)
|
||||||
}
|
}
|
||||||
res, err := tx.Exec("DELETE FROM domains WHERE id = ?", id)
|
res, err := tx.Exec("DELETE FROM domains WHERE id = ?", id)
|
||||||
|
|||||||
@@ -91,7 +91,7 @@ func (s *Store) countInboundWrites(n int) error {
|
|||||||
s.inboundWrites.Store(0)
|
s.inboundWrites.Store(0)
|
||||||
// The zero time removes nothing by age: this is the cap alone.
|
// The zero time removes nothing by age: this is the cap alone.
|
||||||
if _, err := s.PruneInboundSpam(time.Time{}, inboundCapRows); err != nil {
|
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
|
return nil
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ import (
|
|||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/mixeme/selfpost/internal/backup"
|
"github.com/mixeme/selfpost/internal/backup"
|
||||||
|
"github.com/mixeme/selfpost/internal/dmarc"
|
||||||
"github.com/mixeme/selfpost/internal/domain"
|
"github.com/mixeme/selfpost/internal/domain"
|
||||||
"github.com/mixeme/selfpost/internal/secretfile"
|
"github.com/mixeme/selfpost/internal/secretfile"
|
||||||
"github.com/mixeme/selfpost/internal/store"
|
"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 {
|
if err := validate.Email(exp.DMARCRua); err != nil {
|
||||||
return invalid("report address", err)
|
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 {
|
limit := func(rl *domain.RateLimitExport) error {
|
||||||
if rl == nil {
|
if rl == nil {
|
||||||
|
|||||||
@@ -38,6 +38,12 @@ func TestDomainImportIsCheckedLikeTheForms(t *testing.T) {
|
|||||||
if msg := h.checkDomainImport(&exp); msg != "" {
|
if msg := h.checkDomainImport(&exp); msg != "" {
|
||||||
t.Fatalf("a good export refused: %s", 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" {
|
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)
|
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"},
|
"Invalid DKIM selector in export file: selector must be at most 63 characters"},
|
||||||
{"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" },
|
{"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" },
|
||||||
"Invalid report address in export file: "},
|
"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 },
|
{"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"},
|
"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 },
|
{"no messages", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 0 },
|
||||||
|
|||||||
@@ -18,11 +18,15 @@ const limitDomain = "limit.e2e.test"
|
|||||||
// testDomainLimitNotReservedByAStranger is the "done when" of the security
|
// 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.
|
// 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
|
// 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
|
// a client with no login naming a domain there used to take one of that
|
||||||
// slots. Here a domain has a limit of one message an hour; a stranger opens
|
// domain's slots and hold it for as long as its transaction stayed open.
|
||||||
// the submission port, names the domain in MAIL FROM three times without
|
// Here a domain has a limit of one message an hour; a stranger opens the
|
||||||
// signing in, and the domain's own application still sends its one message -
|
// submission port, names the domain in MAIL FROM without signing in and
|
||||||
// and is refused the second, so the limit is in force.
|
// 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) {
|
func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) {
|
||||||
domainID, err := sc.panel.addDomain(limitDomain)
|
domainID, err := sc.panel.addDomain(limitDomain)
|
||||||
if err != nil {
|
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)
|
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)
|
t.Fatalf("the stranger's MAIL FROM: %v", err)
|
||||||
}
|
}
|
||||||
|
defer stranger()
|
||||||
|
|
||||||
send := func(token string) sendResult {
|
send := func(token string) sendResult {
|
||||||
return attemptSend(sendAttempt{
|
return attemptSend(sendAttempt{
|
||||||
@@ -63,35 +69,31 @@ func testDomainLimitNotReservedByAStranger(t *testing.T, sc *scenario) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// strangerMailFrom opens the implicit-TLS submission port, says EHLO and
|
// strangerHoldsMailFrom opens the implicit-TLS submission port, says EHLO
|
||||||
// sends MAIL FROM:<from> n times with RSET between, never signing in. Each
|
// and MAIL FROM:<from> without signing in, and leaves the transaction open
|
||||||
// MAIL FROM is answered 250: the journal milter continues, and Postfix
|
// until the returned func is called. The MAIL FROM is answered 250: the
|
||||||
// refuses such a client only at RCPT (smtpd_delay_reject).
|
// journal milter continues, and Postfix refuses such a client only at RCPT
|
||||||
func strangerMailFrom(from string, n int) error {
|
// (smtpd_delay_reject).
|
||||||
|
func strangerHoldsMailFrom(from string) (func(), error) {
|
||||||
conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{
|
conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 10 * time.Second}, "tcp", smtpsAddr, &tls.Config{
|
||||||
InsecureSkipVerify: true,
|
InsecureSkipVerify: true,
|
||||||
ServerName: selfpostHostname,
|
ServerName: selfpostHostname,
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return nil, err
|
||||||
}
|
}
|
||||||
c, err := smtp.NewClient(conn, selfpostHostname)
|
c, err := smtp.NewClient(conn, selfpostHostname)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
_ = conn.Close()
|
_ = conn.Close()
|
||||||
return err
|
return nil, err
|
||||||
}
|
}
|
||||||
defer c.Close()
|
|
||||||
if err := c.Hello("stranger.e2e.test"); err != nil {
|
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 {
|
||||||
if err := c.Mail(from); err != nil {
|
_ = c.Close()
|
||||||
return fmt.Errorf("MAIL FROM %d: %w", i+1, err)
|
return nil, fmt.Errorf("MAIL FROM: %w", err)
|
||||||
}
|
|
||||||
if err := c.Reset(); err != nil {
|
|
||||||
return err
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
_ = c.Quit()
|
return func() { _ = c.Quit(); _ = c.Close() }, nil
|
||||||
return nil
|
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user