audit 2c: findings of the two reviews
test / test (push) Canceled after 0s

Technical review (Opus, 11 findings, 2 medium) and security review (Fable,
no high or medium) of 3c86280..HEAD. Both confirm the port gives
dkim_mail_parse()'s result on every input it parses, and that no line the
check passes is signed with another domain's key.

- F1 (medium): OpenDKIM's parser takes lines no mail program reads as one
  address - "ceo@bank.com:me@mine.com", "Name <ceo@bank.com:me@mine.com>",
  "<b@evil.com, a@mine.com>", "ceo@bank.com <me@mine.com>",
  "ceo@bank.com@mine.com" - and signs for the last while a lenient reader
  shows the first. The line must now also be one address in RFC 5322's
  grammar (net/mail, display names in any charset passed through), at the
  domain OpenDKIM signs for. A quoted display name may still hold anything.
- F2 (medium): the bindings were asked about OpenDKIM's form of the
  address, quotes and all, so "x"@mine.com passed for an application
  holding @mine.com while x@mine.com belonged to another. They are now
  asked about the reader's address, its local part unquoted, as Postfix
  holds the envelope's.
- F3 / security (low): Bindings.Allowed and DomainOf fold case in ASCII
  only: Postfix folds as Unicode (casefold.c, ICU), but every key and
  login the panel writes is ASCII, and the Unicode fold let the Kelvin sign
  pass for "k" (net/mail then refuses such a line on the domain test).
- F4 (low): a TERM between "postfix start-fg &" and "child=$!" is no longer
  lost: the wrapper stops Postfix as soon as the pid is known.
- F5 (low): for startsecs after a start any failed reload passed as "not
  running". postfix-reload is now build/postfix-reload.sh: exit 75 when
  master -t says no master runs, else exec postfix reload; RunOnce returns
  EndError with the status, and only 75 is taken as done. supervisor.State
  goes.
- F6 (low): crashexit spares postfix-reload too - a reload that could not
  be spawned is reported to whoever asked, not a reason to stop the
  container.
- F7 (low): a certs/ that is itself a link is resolved before the walk
  (filepath.WalkDir does not follow its root).
- F8 (low): at shutdown the tailer reads what is left of the rotated
  file before saving its place. Not changed: a refusal line still waiting
  on the old file at shutdown is not kept - the panel writes refused rows
  itself, only a kill loses one, and that window is the minute's grace.
- F9 (low): pct read as text - empty or unreadable is 100, the rest held
  to 0..100.
- F10 (low): the directory is synced after sasldb2's rename.
- F11 (low): POSTFIX_STARTSECS by expr, so "010" or "08" in
  MILTER_WAIT_TIMEOUT are decimal, not octal or an error.
- Security (low): pagehide also empties defaultValue - the input's value
  attribute and the textarea's text - not only .value. A sasldb2.new left
  by a killed panel is removed at start (entrypoint) and never archived.
  security.md says that links under certs/ are followed and by whom they
  can be placed.

Tests: sender check cases for F1 (four lines), F2 (a bound address
quoted, for its owner and for another), F3; EndError's state and status;
pct cases. architecture.md and CHANGELOG describe the reader rule. Stand
(2026-10-15, make e2e, exit 0); go vet and go test ./... pass on the
server (24 packages).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
mix
2026-10-15 09:17:33 +03:00
co-authored by Claude Opus 5.5
parent ec08c8606b
commit 9eddd49602
20 changed files with 221 additions and 50 deletions
+47 -5
View File
@@ -3,8 +3,11 @@ package milter
import (
"context"
"fmt"
"io"
"log"
"mime"
"net"
"net/mail"
"net/textproto"
"os"
"strings"
@@ -201,13 +204,19 @@ func splitTableLine(line string) (key, value string, ok bool) {
// 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).
//
// Case is folded in ASCII only. Postfix folds as Unicode (src/util/casefold.c
// with ICU, which Debian builds with), but every key and login the panel
// writes is ASCII; a Unicode fold would only let a look-alike pass - the
// Kelvin sign for "k" - for an address OpenDKIM, folding ASCII, does not
// sign (review of stage 2c).
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
}
login, addr = strings.ToLower(login), strings.ToLower(addr)
login, addr = asciiLower(login), asciiLower(addr)
keys := []string{addr}
// recipient_delimiter is "+" (build/postfix-config.sh), so an
// application listed as invoices@ may also send as invoices+march@.
@@ -256,7 +265,7 @@ func (b *Bindings) DomainOf(login string) string {
if b.fresh() != nil {
return ""
}
return b.domainOf[strings.ToLower(login)]
return b.domainOf[asciiLower(login)]
}
// Refusals is where the check leaves the send-log rows of what it refused.
@@ -384,7 +393,8 @@ func (s *senderSession) judge() (shown, why string, err error) {
// OpenDKIM takes the first; a reader may show the others.
return shown, "it must be one address", nil
}
addr := r.user + "@" + asciiLower(r.domain)
signedDomain := asciiLower(r.domain)
addr := r.user + "@" + signedDomain
// OpenDKIM matches its SigningTable against "user@domain" as
// snprintf() leaves it in a buffer of MAXADDRESS + 1 = 257 bytes
// (opendkim.c, dkimf_apply_signtable(); libopendkim/dkim-internal.h).
@@ -394,8 +404,26 @@ func (s *senderSession) judge() (shown, why string, err error) {
if len(addr) > 256 {
return printable(addr), "the address is longer than OpenDKIM reads whole", nil
}
shown = printable(addr)
ok, err := s.bind.Allowed(s.login, asciiLower(addr))
// The address a mail program shows. OpenDKIM's parser reads lines no
// standard reader takes for one address - "ceo@bank.com:me@mine.com",
// "ceo@bank.com <me@mine.com>", "<b@evil.com, a@mine.com>" - and signs
// for the last of them while a lenient program shows the first. So the
// line must also be one address in RFC 5322's grammar (net/mail), at the
// domain OpenDKIM signs for; a quoted display name may say anything.
// And that address, its local part unquoted as Postfix holds the
// envelope's (Postfix 3.7.11, src/global/tok822_parse.c), is the one
// held against the bindings: "x"@mine.com is x@mine.com, which may be
// another application's (review of stage 2c, F1, F2).
list, perr := fromParser.ParseList(unfold(raw))
if perr != nil || len(list) != 1 {
return shown, "it must be one address, as a mail program reads it", nil
}
seen := asciiLower(list[0].Address)
if domainOf(seen) != signedDomain {
return shown, "a mail program would show another address than the one signed", nil
}
shown = printable(seen)
ok, err := s.bind.Allowed(s.login, seen)
if err != nil {
return shown, "", fmt.Errorf("sender bindings: %w", err)
}
@@ -405,6 +433,20 @@ func (s *senderSession) judge() (shown, why string, err error) {
return shown, "", nil
}
// fromParser reads the From: line as a mail program would. A display name
// encoded in a charset Go does not convert (windows-1251, koi8-r, ...) is
// still a display name: its bytes are passed through unread.
var fromParser = mail.AddressParser{WordDecoder: &mime.WordDecoder{
CharsetReader: func(charset string, input io.Reader) (io.Reader, error) { return input, nil },
}}
// unfold joins the lines of a folded header value (RFC 5322 2.2.3): a line
// break is dropped, the white space that follows it stays. Postfix hands a
// milter the line breaks as LF (assumptions B-PM12).
func unfold(v string) string {
return strings.NewReplacer("\r\n", "", "\n", "", "\r", "").Replace(v)
}
// asciiLower lower-cases A-Z only, as OpenDKIM's dkimf_lowercase() and
// Postfix's table lookups do; strings.ToLower would also fold characters
// outside ASCII - the Kelvin sign to "k" - into names they are not.