dmarc: a report is bounded, kept per domain, and ingested one at a time

Security review of the DMARC report path (Fable, 2026-10-11), which ran for the first time in 2.0.0. No Critical or High finding. Fixed: - Medium: a report was bounded only as 8 MiB of XML - some 160 000 records or one name of megabytes, all of it rows and table cells. At most 5 000 records (more is ErrRejected, dropped), text fields cut to 255 bytes on a character boundary, a record's count capped. - Medium: the cap of 500 reports was for the whole server, so a stranger filing under one domain's address evicted every other domain's reports. PruneDMARCReports counts per domain. - Low: the pipe service ran up to default_process_limit (100) commands at once against the panel's database; maxproc is 1. - Low: the reason logged for a dropped message carried the sender's bytes (header line, zip entry name) as they came; cut to 200 bytes and reduced to printable ASCII, zip entry names quoted. - Low: SELFPOST_DATA_DIR / SELFPOST_DB_PATH with whitespace or "$" would reach the command through export_environment as another path; refused at start when DMARC ingest is on. Left as they are, told to the owner: the parse-failure count is a lifetime figure shown under "This week"; a report larger than the command reads is dropped as unreadable; nested multipart is not searched. The hub's Retention line follows the design commit before this one. security.md: the review, and the accepted-risk entry says what a stranger can and cannot do now. The roadmap no longer calls the review pending. go vet ./cmd/... ./internal/dmarc/ ./internal/store/: ok go test ./cmd/... ./internal/dmarc/ ./internal/store/ ./internal/web/...: ok make e2e on the test server (with the port-25 scenarios of the next commit): ok Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-11 17:40:31 +03:00
parent 505260e227
commit 4dd46ea355
15 changed files with 239 additions and 34 deletions
@@ -0,0 +1,109 @@
+package dmarc
+
+import (
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/mixeme/selfpost/internal/store"
+)
+
+func reportXML(orgName, domain, records string) string {
+	return `<?xml version="1.0"?><feedback><report_metadata><org_name>` + orgName +
+		`</org_name><email>a@example.net</email><report_id>1</report_id>` +
+		`<date_range><begin>1723593600</begin><end>1723680000</end></date_range></report_metadata>` +
+		`<policy_published><domain>` + domain + `</domain><p>none</p></policy_published>` + records + `</feedback>`
+}
+
+const oneRecord = `<record><row><source_ip>203.0.113.10</source_ip><count>3</count>` +
+	`<policy_evaluated><disposition>none</disposition><dkim>pass</dkim><spf>pass</spf></policy_evaluated></row>` +
+	`<identifiers><header_from>example.com</header_from></identifiers></record>`
+
+// The XML is a stranger's: what it may make the relay store is bounded.
+func TestParseAggregateBoundsWhatAReportHolds(t *testing.T) {
+	long := strings.Repeat("é", 4000)
+	got, err := ParseAggregate([]byte(reportXML(long, "example.com", oneRecord)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(got.Reporter) > maxFieldLen || !strings.HasPrefix(long, got.Reporter) || got.Reporter == "" {
+		t.Fatalf("reporter name is %d bytes, want the start of the name in at most %d", len(got.Reporter), maxFieldLen)
+	}
+	if strings.ContainsRune(got.Reporter, '�') {
+		t.Fatal("the name was cut in the middle of a character")
+	}
+
+	huge := `<record><row><source_ip>203.0.113.10</source_ip><count>9223372036854775807</count>` +
+		`<policy_evaluated><dkim>pass</dkim></policy_evaluated></row></record>`
+	got, err = ParseAggregate([]byte(reportXML("r", "example.com", huge+huge)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.PassCount != 2*maxRecordCount {
+		t.Fatalf("pass count = %d, want two records clamped to %d each", got.PassCount, maxRecordCount)
+	}
+
+	if _, err := ParseAggregate([]byte(reportXML("r", "example.com", strings.Repeat(oneRecord, maxReportRecords)))); err != nil {
+		t.Fatalf("a report of %d records was refused: %v", maxReportRecords, err)
+	}
+	if _, err := ParseAggregate([]byte(reportXML("r", "example.com", strings.Repeat(oneRecord, maxReportRecords+1)))); err == nil {
+		t.Fatalf("a report of %d records was accepted", maxReportRecords+1)
+	}
+}
+
+// A report over the bound is one the relay does not keep: dropped, not retried.
+func TestIngestMessageRejectsAReportWithTooManyRecords(t *testing.T) {
+	st, err := store.Open(t.TempDir() + "/test.db")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer st.Close()
+	if _, err := st.AddDomain("example.com", "selfpost"); err != nil {
+		t.Fatal(err)
+	}
+	xml := reportXML("r", "example.com", strings.Repeat(oneRecord, maxReportRecords+1))
+	err = IngestMessage(st, strings.NewReader(gzipMessage(t, xml)), "dmarc-reports+example.com@mail.example.com", time.Now().UTC())
+	if !errors.Is(err, ErrRejected) {
+		t.Fatalf("err = %v, want ErrRejected", err)
+	}
+}
+
+// Reports filed under one domain do not push another domain's reports out.
+func TestIngestKeepsEachDomainsReportsApart(t *testing.T) {
+	st, err := store.Open(t.TempDir() + "/test.db")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer st.Close()
+	now := time.Now().UTC()
+	if _, err := st.InsertDMARCReport(store.DMARCReport{
+		Domain: "quiet.example", Reporter: "genuine", ReportID: "1", PeriodBegin: now, PeriodEnd: now, ReceivedAt: now.Add(-time.Hour),
+	}); err != nil {
+		t.Fatal(err)
+	}
+	for i := 0; i < store.DMARCReportsMaxKeep+20; i++ {
+		if _, err := st.InsertDMARCReport(store.DMARCReport{
+			Domain: "flooded.example", Reporter: "stranger", ReportID: strings.Repeat("x", 1+i), PeriodBegin: now, PeriodEnd: now, ReceivedAt: now,
+		}); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := st.PruneDMARCReports("flooded.example"); err != nil {
+		t.Fatal(err)
+	}
+	quiet, err := st.ListDMARCReports(store.DMARCReportScope{Domains: []string{"quiet.example"}}, 10)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(quiet) != 1 {
+		t.Fatalf("the other domain has %d reports after the flood, want its 1", len(quiet))
+	}
+	flooded, err := st.ListDMARCReports(store.DMARCReportScope{Domains: []string{"flooded.example"}}, 10000)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(flooded) > store.DMARCReportsMaxKeep {
+		t.Fatalf("the flooded domain keeps %d reports, want at most %d", len(flooded), store.DMARCReportsMaxKeep)
+	}
+}
@@ -84,7 +84,7 @@ func IngestMessage(st *store.Store, r io.Reader, recipient string, receivedAt ti
 	if _, err := st.InsertDMARCReport(rep); err != nil {
 		return err
 	}
-	return st.PruneDMARCReports()
+	return st.PruneDMARCReports(rep.Domain)
 }

 // checkReportDomain refuses a report whose policy domain is not one of this
@@ -9,6 +9,7 @@ import (
 	"io"
 	"strings"
 	"time"
+	"unicode/utf8"
 )

 // ParsedReport is the structured form of a DMARC aggregate XML payload.
@@ -74,6 +75,31 @@ type feedbackXML struct {
 	} `xml:"record"`
 }

+// What a report may hold. The XML comes from anyone who knows a report address,
+// and eight megabytes of it (maxAggregateXML) fit some 160 000 empty records or
+// one name of eight megabytes - every one of them a row or a table cell later.
+// A real report has tens of records, a few thousand from the largest
+// receivers, and no field longer than a line.
+const (
+	maxReportRecords = 5000
+	maxFieldLen      = 255
+	maxRecordCount   = 1_000_000_000
+)
+
+// field trims a text value of the report and cuts it to maxFieldLen bytes, on
+// a character boundary.
+func field(s string) string {
+	s = strings.TrimSpace(s)
+	if len(s) <= maxFieldLen {
+		return s
+	}
+	s = s[:maxFieldLen]
+	for len(s) > 0 && !utf8.ValidString(s) {
+		s = s[:len(s)-1]
+	}
+	return s
+}
+
 // ParseAggregate decodes gzip- or zip-compressed, or raw, DMARC aggregate XML.
 func ParseAggregate(raw []byte) (ParsedReport, error) {
 	data, err := decompress(raw)
@@ -87,34 +113,43 @@ func ParseAggregate(raw []byte) (ParsedReport, error) {
 	if doc.Metadata.ReportID == "" || doc.Policy.Domain == "" {
 		return ParsedReport{}, fmt.Errorf("dmarc xml: missing report_id or domain")
 	}
+	if len(doc.Records) > maxReportRecords {
+		return ParsedReport{}, fmt.Errorf("dmarc xml: %d records, more than the %d a report may hold", len(doc.Records), maxReportRecords)
+	}
+	// Cut, not refused: a domain name is at most 253 bytes, so a longer one
+	// names nothing this relay sends for and is turned away as such.
 	out := ParsedReport{
-		Reporter:     strings.TrimSpace(doc.Metadata.OrgName),
-		ReportID:     strings.TrimSpace(doc.Metadata.ReportID),
-		ContactEmail: strings.TrimSpace(doc.Metadata.Email),
+		Reporter:     field(doc.Metadata.OrgName),
+		ReportID:     field(doc.Metadata.ReportID),
+		ContactEmail: field(doc.Metadata.Email),
 		PeriodBegin:  time.Unix(doc.Metadata.DateRange.Begin, 0).UTC(),
 		PeriodEnd:    time.Unix(doc.Metadata.DateRange.End, 0).UTC(),
-		Domain:       strings.ToLower(strings.TrimSpace(doc.Policy.Domain)),
-		PolicyP:      strings.TrimSpace(doc.Policy.P),
-		PolicySP:     strings.TrimSpace(doc.Policy.SP),
+		Domain:       strings.ToLower(field(doc.Policy.Domain)),
+		PolicyP:      field(doc.Policy.P),
+		PolicySP:     field(doc.Policy.SP),
 		PolicyPct:    doc.Policy.Pct,
-		PolicyADKIM:  strings.TrimSpace(doc.Policy.ADKIM),
-		PolicyASPF:   strings.TrimSpace(doc.Policy.ASPF),
+		PolicyADKIM:  field(doc.Policy.ADKIM),
+		PolicyASPF:   field(doc.Policy.ASPF),
 	}
 	if out.PolicyPct == 0 {
 		out.PolicyPct = 100
 	}
 	for _, rec := range doc.Records {
 		row := ParsedRecord{
-			SourceIP:    strings.TrimSpace(rec.Row.SourceIP),
+			SourceIP:    field(rec.Row.SourceIP),
 			Count:       rec.Row.Count,
-			Disposition: strings.TrimSpace(rec.Row.Policy.Disposition),
-			SPFResult:   strings.TrimSpace(rec.Row.Policy.SPF),
-			DKIMResult:  strings.TrimSpace(rec.Row.Policy.DKIM),
-			HeaderFrom:  strings.TrimSpace(rec.Identifiers.HeaderFrom),
+			Disposition: field(rec.Row.Policy.Disposition),
+			SPFResult:   field(rec.Row.Policy.SPF),
+			DKIMResult:  field(rec.Row.Policy.DKIM),
+			HeaderFrom:  field(rec.Identifiers.HeaderFrom),
 		}
 		if row.Count <= 0 {
 			row.Count = 1
 		}
+		// Bounded so that the sums of a report cannot wrap around.
+		if row.Count > maxRecordCount {
+			row.Count = maxRecordCount
+		}
 		if dmarcRecordPasses(row) {
 			out.PassCount += row.Count
 		} else {
@@ -172,12 +207,12 @@ func unzip(raw []byte) ([]byte, error) {
 		}
 		rc, err := f.Open()
 		if err != nil {
-			return nil, fmt.Errorf("dmarc zip open %s: %w", f.Name, err)
+			return nil, fmt.Errorf("dmarc zip open %q: %w", field(f.Name), err)
 		}
 		data, err := io.ReadAll(io.LimitReader(rc, maxAggregateXML))
 		rc.Close()
 		if err != nil {
-			return nil, fmt.Errorf("dmarc zip read %s: %w", f.Name, err)
+			return nil, fmt.Errorf("dmarc zip read %q: %w", field(f.Name), err)
 		}
 		return data, nil
 	}