From 55aef6e721bf4ae5aedea3519510a632f6f00664 Mon Sep 17 00:00:00 2001 From: Mix Date: Wed, 14 Oct 2026 20:03:34 +0300 Subject: [PATCH] sender check: the sender map read and looked up as Postfix does it Postfix 3.7.11 takes the owners of the first key mail_addr_find() finds - the address, then without its extension, then @domain, each tried only when the one before is absent (src/global/mail_addr_find.c, mail_addr_find_opt()) - and compares the login with them by strcasecmp_utf8() (src/smtpd/smtpd_check.c, reject_auth_sender_login_mismatch()). The check accepted a From: address when any of the three keys named the application: an application holding @domain could write an address listed for another application of the domain, which Postfix refuses it in the envelope, and have it signed (assumptions B-PS1). The table is now read as dict_thash_open() reads it: continuation lines, owners split on commas and blanks (B-PS4), the key ending at the first blank outside quotes, keys folded; a key given twice makes Postfix refuse the whole table (no DICT_FLAG_DUP_* flag), and the check then defers too. The writer folds keys, so two cases of one address cannot be two lines. e2e on the stand (2026-10-14, make e2e, exit 0): the wildcard application is refused billing@ - listed for app-list - in From: (550 5.7.1), and in the envelope by Postfix at RCPT ("553 5.7.1 : Sender address rejected: not owned by user"). go vet and go test ./... pass on the server (23 packages). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 12 +++ docs/architecture.md | 14 ++- docs/guide.md | 4 + internal/milter/sender.go | 161 ++++++++++++++++++++++--------- internal/milter/sender_test.go | 74 ++++++++++++++ internal/postfix/postfix.go | 13 ++- internal/postfix/postfix_test.go | 16 +++ test/e2e/sender_check_test.go | 21 +++- 8 files changed, 262 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 499f002..432a14e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,18 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version both exceptions off; tried against rspamd 3.11.1 before and after (audit 2026-10-12, stage 2c; assumptions C-R7). Copy the new `antispam/rspamd/` files next to your compose fragment. +- **An address listed for one application could be put in `From:` by + another of its domain.** The sender check accepted a `From:` address when + any of its keys in the sender map - the address, the address without its + extension, the domain - named the application; Postfix takes the first + key it finds and asks only its owners. An application set to any address + of a domain could therefore write in `From:` an address listed for + another application of that domain - which Postfix refuses it in the + envelope - and have it signed. The check now reads the map as Postfix + does: the first key found decides, logins are compared without regard to + case, owners may be separated by blanks or continue on the next line, and + a key written twice makes the check defer, as it makes Postfix refuse the + whole table (assumptions B-PS1, B-PS4). ### Fixed diff --git a/docs/architecture.md b/docs/architecture.md index d168b79..178a0b1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -242,10 +242,16 @@ server itself. its domain may write any address of that domain, and one with a list only the listed ones; everything else is refused - another registered domain, the server's name, a domain the server knows nothing of. One table for - both senders and one lookup: the address, the address without its - `+extension` (`recipient_delimiter`, which `build/postfix-config.sh` sets - for that reason), then the domain. Postfix compares logins without regard - to case and the check as written, which is how the panel writes them. + both senders, read as Postfix reads a texthash table and looked up as + Postfix looks it up: the address, the address without its `+extension` + (`recipient_delimiter`, which `build/postfix-config.sh` sets for that + reason), then `@domain` - and the **first of these keys in the table + decides**, its owners alone: an address listed for one application is not + open to another that holds the whole domain. Logins are compared without + regard to case, as Postfix compares them (Postfix 3.7.11, + `src/smtpd/smtpd_check.c`, `reject_auth_sender_login_mismatch()`, over + `src/global/mail_addr_find.c`, `mail_addr_find_opt()`). A key written + twice makes Postfix refuse the whole table, and the check defers then too. - **How the header is read.** Exactly one `From:` field (none, or more than one, is refused), parsed as an address list that must hold exactly one address. Then a cruder second look at the raw value: OpenDKIM finds the diff --git a/docs/guide.md b/docs/guide.md index ba417be..89f7ec6 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -1122,6 +1122,10 @@ An address with an extension counts as the address: an application listed as `invoices@…` may also send as `invoices+march@…`, in the envelope and in `From:` alike. +An address listed for an application is that application's alone: another +application of the same domain, set to any address, may not send as it - +Postfix asks who owns the address before it asks who owns the domain. + A message whose `From:` is anything else is refused with `550 5.7.1` and a text naming the line: another of your domains, the server's own name, an address at a domain that is not on this server, a line with two addresses, diff --git a/internal/milter/sender.go b/internal/milter/sender.go index 1a54435..bec1851 100644 --- a/internal/milter/sender.go +++ b/internal/milter/sender.go @@ -31,10 +31,9 @@ import ( // answer, Postfix defers mail. It therefore depends on as little as it can. // It reads no database: what an application may send as comes from the file // of sender bindings Postfix itself reads for the envelope, so the two go by -// one table, by one lookup: the address, the address without its -// "+extension", the domain. (Postfix compares logins without regard to case; -// the check compares them as written, which is how the panel writes them.) The send-log row of a refusal is written by another goroutine and -// is not waited for. +// one table, read the way Postfix reads it and looked up the way Postfix looks +// it up (Bindings). The send-log row of a refusal is written by another +// goroutine and is not waited for. // // Postfix 3.7.11, src/smtpd/smtpd_check.c, reject_auth_sender_login_mismatch(): // the SASL login is compared with an owner by strcasecmp_utf8(); @@ -78,8 +77,9 @@ func (f *tableFile) changed() (lines []string, again bool, err error) { type Bindings struct { mu sync.Mutex file tableFile - logins map[string]map[string]bool // sender -> logins + logins map[string]map[string]bool // sender -> logins, both in lower case domainOf map[string]string // login -> the domain it sends as + broken error // why Postfix would not use the table } // NewBindings reads the file at path on first use. @@ -90,63 +90,136 @@ func NewBindings(path string) *Bindings { // fresh makes the table match the file. func (b *Bindings) fresh() error { lines, again, err := b.file.changed() - if err != nil || !again { + if err != nil { return err } - logins := make(map[string]map[string]bool) - domains := make(map[string]string) + if again { + b.logins, b.domainOf, b.broken = parseSenderMap(lines) + } + return b.broken +} + +// parseSenderMap reads the lines of a sender_login_maps file as Postfix 3.7.11 +// reads a texthash table (src/util/dict_thash.c, dict_thash_open(), over +// src/util/readlline.c, readllines()): +// - a line that starts with a blank continues the one before; an empty +// line, or one whose first non-blank is "#", is skipped; +// - the key ends at the first blank outside double quotes, a backslash +// escaping the next character; a line with an unbalanced quote, or +// without a key or a value, is ignored (with a warning, in Postfix); +// - the key is folded to lower case (DICT_FLAG_FOLD_FIX, which +// src/smtpd/smtpd_check.c, pre_jail_init(), gives +// smtpd_sender_login_maps); +// - a key given twice makes the whole table unusable: no DICT_FLAG_DUP_* +// flag is given, so dict_thash_open() returns a surrogate that fails +// every lookup, and smtpd answers 451 (reject_server_error()). The check +// then defers mail as well. +// +// The owners are split on commas and blanks (smtpd_check.c, +// reject_auth_sender_login_mismatch(), mystrtok(CHARS_COMMA_SP)). +func parseSenderMap(lines []string) (logins map[string]map[string]bool, domains map[string]string, broken error) { + var logical []string for _, line := range lines { - // texthash: , the value a comma-separated - // list of logins (internal/postfix renderSenderLoginMaps). - // Postfix 3.7.11, src/util/dict_thash.c, dict_thash_open(): the key - // ends at the first blank; src/util/readlline.c skips a line whose - // first non-blank is "#"; src/util/dict_ht.c folds the key to lower - // case (DICT_FLAG_FOLD_FIX, set for smtpd_sender_login_maps in - // src/smtpd/smtpd_check.c). - fields := strings.Fields(line) - if len(fields) < 2 || strings.HasPrefix(fields[0], "#") { + line = strings.TrimRight(line, "\r") + trimmed := strings.TrimLeft(line, " \t") + switch { + case trimmed == "" || trimmed[0] == '#': + continue + case len(trimmed) < len(line) && len(logical) > 0: + logical[len(logical)-1] += line + default: + logical = append(logical, trimmed) + } + } + logins = make(map[string]map[string]bool) + domains = make(map[string]string) + for _, line := range logical { + key, value, ok := splitTableLine(line) + if !ok { continue } - sender := strings.ToLower(fields[0]) - set := logins[sender] - if set == nil { - set = make(map[string]bool) - logins[sender] = set + sender := strings.ToLower(key) + if _, dup := logins[sender]; dup { + return nil, nil, fmt.Errorf("sender map: duplicate entry %q, which Postfix refuses the whole table for", sender) } - for _, l := range strings.Split(strings.Join(fields[1:], ""), ",") { - if l != "" { - set[l] = true - if _, known := domains[l]; !known { - domains[l] = domainOf(sender) - } + set := make(map[string]bool) + logins[sender] = set + for _, l := range strings.FieldsFunc(value, func(r rune) bool { + return r == ',' || r == ' ' || r == '\t' || r == '\r' || r == '\n' + }) { + l = strings.ToLower(l) + set[l] = true + if _, known := domains[l]; !known { + domains[l] = domainOf(sender) } } } - b.logins, b.domainOf = logins, domains - return nil + return logins, domains, nil } -// Allowed reports whether login may send as addr: the address itself is bound -// to it, or its whole domain is. This is the lookup Postfix makes for the -// envelope sender. +// splitTableLine splits a logical texthash line into its key and its value, +// as dict_thash_open() does: the key ends at the first blank outside double +// quotes (a backslash escaping the character after it), and the value is the +// rest without the blanks around it. ok is false where Postfix ignores the +// line: an unbalanced quote, no key, or no value. +func splitTableLine(line string) (key, value string, ok bool) { + inQuotes := false + i := 0 + for ; i < len(line); i++ { + c := line[i] + if c == '\\' { + i++ + continue + } + if c == '"' { + inQuotes = !inQuotes + continue + } + if (c == ' ' || c == '\t') && !inQuotes { + break + } + } + if inQuotes { + return "", "", false + } + if i > len(line) { + i = len(line) + } + key, value = line[:i], strings.Trim(line[i:], " \t") + return key, value, key != "" && value != "" +} + +// Allowed reports whether login may send as addr - the lookup Postfix makes +// for the envelope sender. Postfix 3.7.11, src/smtpd/smtpd_check.c, +// reject_auth_sender_login_mismatch(), takes the owners of the first key +// mail_addr_find() finds (src/global/mail_addr_find.c, mail_addr_find_opt() +// with MA_FIND_DEFAULT): the address, then the address without its +// "+extension", then "@domain" - each tried only when the one before is not +// in the table - and compares the login with them by strcasecmp_utf8(). So +// an address bound to one application is not open to another that holds its +// whole domain (assumptions B-PS1). The bare local part MA_FIND_DEFAULT also +// tries for a local domain is never a key here: every key the panel writes +// has an "@" (internal/postfix renderSenderLoginMaps). func (b *Bindings) Allowed(login, addr string) (bool, error) { b.mu.Lock() defer b.mu.Unlock() if err := b.fresh(); err != nil { return false, err } - addr = strings.ToLower(addr) - if b.logins[addr][login] { - return true, nil + login, addr = strings.ToLower(login), strings.ToLower(addr) + keys := []string{addr} + // recipient_delimiter is "+" (build/postfix-config.sh), so an + // application listed as invoices@ may also send as invoices+march@. + if bare := withoutExtension(addr); bare != addr { + keys = append(keys, bare) } - // Postfix's second try: the address without its extension, so that an - // application listed as invoices@ may also send as invoices+march@ - // (recipient_delimiter in build/postfix-config.sh). - if bare := withoutExtension(addr); bare != addr && b.logins[bare][login] { - return true, nil + if d := domainOf(addr); d != "" { + keys = append(keys, "@"+d) } - if d := domainOf(addr); d != "" && b.logins["@"+d][login] { - return true, nil + for _, k := range keys { + if owners, found := b.logins[k]; found { + return owners[login], nil + } } return false, nil } @@ -182,7 +255,7 @@ func (b *Bindings) DomainOf(login string) string { if b.fresh() != nil { return "" } - return b.domainOf[login] + return b.domainOf[strings.ToLower(login)] } // Signers is OpenDKIM's SigningTable as the check reads it: the names this diff --git a/internal/milter/sender_test.go b/internal/milter/sender_test.go index 66c7ee8..5ae55e9 100644 --- a/internal/milter/sender_test.go +++ b/internal/milter/sender_test.go @@ -216,6 +216,80 @@ func TestBindingsFollowTheFile(t *testing.T) { } } +// The owners of the first key Postfix finds decide, not any key that lists +// the login: an address bound to one application is not open to another that +// holds its whole domain (Postfix 3.7.11, smtpd_check.c, +// reject_auth_sender_login_mismatch(), over mail_addr_find(); assumptions +// B-PS1). Composed by hand in internal/postfix's format. +func TestBindingsFirstKeyWins(t *testing.T) { + bind := NewBindings(writeMaps(t, `@example.org shop +billing@example.org billing +invoices+march@example.org other +`)) + for _, c := range []struct { + login, addr string + want bool + }{ + {"shop", "a@example.org", true}, + {"billing", "billing@example.org", true}, + {"shop", "billing@example.org", false}, // the address's own key decides + {"billing", "billing+q3@example.org", true}, // without the extension + {"shop", "billing+q3@example.org", false}, // ... before @domain + {"shop", "invoices+march@example.org", false}, // the full key first + {"other", "invoices+march@example.org", true}, + {"shop", "invoices+april@example.org", true}, // no key without "+": @domain + {"billing", "a@example.org", false}, + {"SHOP", "A@Example.ORG", true}, // strcasecmp_utf8, a folded key + } { + if ok, err := bind.Allowed(c.login, c.addr); err != nil || ok != c.want { + t.Errorf("Allowed(%q, %q) = %v, %v; want %v", c.login, c.addr, ok, err, c.want) + } + } +} + +// The table is read the way dict_thash_open() reads it: continuation lines, +// owners split on blanks as well as commas, comments, lines Postfix ignores. +// Composed by hand. +func TestBindingsReadAsPostfixReadsThem(t *testing.T) { + bind := NewBindings(writeMaps(t, "# a comment\n"+ + "@shop.example.org shop,\n"+ + " shop2 shop3\n"+ + "\n"+ + "\"a b\"@x.example.org quoted\n"+ + "nokey-value-only\n"+ + "\"open@x.example.org broken\n"+ + "@news.example.net\tnews\r\n")) + for _, c := range []struct { + login, addr string + want bool + }{ + {"shop", "a@shop.example.org", true}, + {"shop2", "a@shop.example.org", true}, + {"shop3", "a@shop.example.org", true}, + {"news", "a@news.example.net", true}, + {"quoted", "\"a b\"@x.example.org", true}, + {"broken", "\"open@x.example.org", false}, + } { + if ok, err := bind.Allowed(c.login, c.addr); err != nil || ok != c.want { + t.Errorf("Allowed(%q, %q) = %v, %v; want %v", c.login, c.addr, ok, err, c.want) + } + } +} + +// A key given twice - in any case, since Postfix folds keys - makes Postfix +// refuse the whole table (dict_thash_open(), no DICT_FLAG_DUP_* flag), and +// the check defers as smtpd does. Composed by hand. +func TestBindingsDuplicateKeyBreaksTheTable(t *testing.T) { + bind := NewBindings(writeMaps(t, "@shop.example.org shop\nA@Shop.example.org other\n")) + if _, err := bind.Allowed("shop", "b@shop.example.org"); err != nil { + t.Fatalf("two different keys: %v", err) + } + bind = NewBindings(writeMaps(t, "@shop.example.org shop\n@SHOP.example.org other\n")) + if ok, err := bind.Allowed("shop", "b@shop.example.org"); err == nil || ok { + t.Fatalf("duplicate key: %v, %v; want an error", ok, err) + } +} + // Without the signing table the second look cannot be taken, and a line it // would have to judge is deferred; a line that names nobody else is not held // up by it. diff --git a/internal/postfix/postfix.go b/internal/postfix/postfix.go index 8742d3f..68470bf 100644 --- a/internal/postfix/postfix.go +++ b/internal/postfix/postfix.go @@ -100,10 +100,17 @@ func renderSenderLoginMaps(bindings []Binding) ([]byte, error) { if err := assertMapSafe(b.Address, b.Login); err != nil { return nil, err } - if _, seen := byAddr[b.Address]; !seen { - order = append(order, b.Address) + // One line per key as Postfix folds it: smtpd opens the map with + // DICT_FLAG_FOLD_FIX and no DICT_FLAG_DUP_* flag, so two keys that + // differ only in case would make the whole table unusable (Postfix + // 3.7.11, src/util/dict_thash.c, dict_thash_open()). The panel stores + // addresses in lower case already; this keeps the file safe if a row + // ever is not. + addr := strings.ToLower(b.Address) + if _, seen := byAddr[addr]; !seen { + order = append(order, addr) } - byAddr[b.Address] = appendUnique(byAddr[b.Address], b.Login) + byAddr[addr] = appendUnique(byAddr[addr], b.Login) } sort.Strings(order) diff --git a/internal/postfix/postfix_test.go b/internal/postfix/postfix_test.go index 04e5c88..6b0f747 100644 --- a/internal/postfix/postfix_test.go +++ b/internal/postfix/postfix_test.go @@ -50,6 +50,22 @@ func TestRenderSenderLoginMapsDedupesLogin(t *testing.T) { } } +// Keys differing only in case are one key to Postfix, which folds them, and +// two of them would make it refuse the whole table (dict_thash_open() with no +// DICT_FLAG_DUP_* flag): they are written as one line. +func TestRenderSenderLoginMapsFoldsKeys(t *testing.T) { + got, err := renderSenderLoginMaps([]Binding{ + {"Billing@Example.org", "billing"}, + {"billing@example.org", "billing2"}, + }) + if err != nil { + t.Fatal(err) + } + if string(got) != "billing@example.org billing,billing2\n" { + t.Errorf("map = %q, want one folded key", got) + } +} + func TestAssertMapSafeRejectsInjection(t *testing.T) { bad := []struct{ addr, login string }{ {"@exa mple.com", "log"}, diff --git a/test/e2e/sender_check_test.go b/test/e2e/sender_check_test.go index f08f128..3a14844 100644 --- a/test/e2e/sender_check_test.go +++ b/test/e2e/sender_check_test.go @@ -62,6 +62,10 @@ func testSenderCheck(t *testing.T, sc *scenario) { {"the server's own name", wildcard, "postmaster@" + selfpostHostname, false}, {"a domain the server knows nothing of", wildcard, "someone@elsewhere.e2e.test", false}, {"an address off the list", list, "director@" + senderDomain, false}, + // The first key Postfix finds decides: billing@ is listed for + // app-list, so the wildcard application may not have it, in the + // header as in the envelope (assumptions B-PS1; the envelope below). + {"the address listed for another application", wildcard, "billing@" + senderDomain, false}, {"another sender in the display name", wildcard, `"director@` + otherDomain + `" `, false}, } { token := uniqueToken("from") @@ -98,14 +102,27 @@ func testSenderCheck(t *testing.T, sc *scenario) { } } + // What the header case above holds to: Postfix itself refuses the + // wildcard application billing@ as envelope sender, at RCPT, because the + // address's own key names app-list only (Postfix 3.7.11, + // src/smtpd/smtpd_check.c, reject_auth_sender_login_mismatch(): "553 + // 5.7.1 <%s>: Sender address rejected: not owned by user %s"). + if res := attemptSend(sendAttempt{ + authLogin: sc.appLogin, authPassword: sc.appPassword, + from: "billing@" + senderDomain, headerFrom: "alerts@" + senderDomain, to: recipient, + subject: "e2e " + uniqueToken("envelope"), body: "sender check: another application's address in the envelope", + }); res.refusedAt("RCPT", 553, "not owned by user") != nil { + t.Errorf("the address listed for another application, in the envelope: %v", res.refusedAt("RCPT", 553, "not owned by user")) + } + // A refusal is a row of the Outbound log whose tag says why. if err := waitFor("the refusals to appear in the Outbound log", 15*time.Second, 500*time.Millisecond, func() (bool, error) { rows, err := sc.panel.sendLogRows(senderDomain, "") if err != nil { return false, err } - if n := strings.Count(rows, `">wrong From`); n < 5 { - return false, fmt.Errorf("%d rows tagged wrong From, want 5", n) + if n := strings.Count(rows, `">wrong From`); n < 6 { + return false, fmt.Errorf("%d rows tagged wrong From, want 6", n) } return true, nil }); err != nil {