From 32f6ed626d4bc8279139b6cef87ad0b88c299fb2 Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 17 Oct 2026 22:58:12 +0300 Subject: [PATCH] findings of the security review of stages 2-6 The review of 4433688..0808779 (read-only, a separate agent) found one medium and some lows; all fixed here. Medium, proven by the reviewer: A1's userKey put the typed username into the limiter map whole, and the public forms had no body bound beyond net/http's 10 MiB for a parsed form - each refused request kept up to 10 MiB in memory for fifteen minutes. userKey now holds a 96-bit hash of the folded name, and sign-in, reset, a reset link's page and setup read at most 64 KiB (limitForm, http.MaxBytesReader). TestPublicFormsAreBoundedAndKeysShort. Lows: - Refund skipped the request that filled a bucket (count < max), so a right password at the ceiling left the shared address blocked; Refund now always undoes the Allow it follows. - The reset form did not give the address its attempt back when the name's bucket refused (sign-in did); it does. - A3's day of five links is spent only by a link that goes out, and the price of the pause - anyone knowing a username can pause that owner's reset for a day - is recorded in security.md. - longHeaderLine counted a trailing CR on a header's first line. - test.yml declares permissions: contents: read. Not changed, as the review's nits: the store matches names exactly while the limiter folds them (stricter, said in the comment now); a session from a longer window is honoured until its next Touch (the next request). go test ./internal/web/auth ./internal/milter. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 5 +++++ docs/security.md | 10 ++++++++-- internal/milter/sign.go | 2 +- internal/web/auth/handlers.go | 2 ++ internal/web/auth/limitkeys.go | 20 +++++++++++++++++--- internal/web/auth/limitkeys_test.go | 16 ++++++++++++++++ internal/web/auth/links.go | 12 ++++++++++++ internal/web/auth/ratelimit.go | 8 +++++--- 8 files changed, 66 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 312d15f..3ffd923 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,6 +5,11 @@ on: branches: [main] pull_request: +# The job reads the repository and nothing else (security audit 2026-10-17, +# review of stages 2-6). +permissions: + contents: read + jobs: test: runs-on: ubuntu-latest diff --git a/docs/security.md b/docs/security.md index e24e2b7..3ba945e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -154,7 +154,9 @@ The panel is exposed to the internet — the items below are **not optional**. security audit 2026-10-17, A1, A9). When the first requests after a start all come from one private peer with `X-Forwarded-For` and no proxy is trusted, the log says so with the value to set. A refused sign-in and a - lockout are logged, five lines per address in 15 minutes (A8). The Account form's + lockout are logged, five lines per address in 15 minutes (A8). A public + form - sign-in, reset, a link's page, setup - is read only up to 64 KiB, + and the limiters keep a hash of a typed name, not the name. The Account form's current-password check has a limit of its own, ten in 15 minutes per user, so guesses there do not lock the address out of signing in (`2.5.4`). - An unknown username is refused in the time a wrong password is: the sign-in @@ -200,7 +202,11 @@ The panel is exposed to the internet — the items below are **not optional**. hour per account and five reset links a day - after which the owner is told once that further requests are ignored for 24 hours, and nothing more is sent (`2.14.4`; audit A3: anyone who knew a username kept a live - link in that mailbox five times an hour) - and a link's page to 30 requests in 15 minutes per link + link in that mailbox five times an hour). The price is recorded: whoever + knows a username can spend those five links and so pause the owner's own + reset for a day; signing in, and a global administrator setting a + password on Server › Users, still work, and only a link that went out + counts - and a link's page to 30 requests in 15 minutes per link (300 per address alone), so one stranger behind a shared proxy cannot stop the others asking for or opening a link (`2.14.4`; audit A1). A link's page changes nothing on `GET` (a mail scanner opens links): the button does. Setting a password diff --git a/internal/milter/sign.go b/internal/milter/sign.go index de82c3d..a3aa9eb 100644 --- a/internal/milter/sign.go +++ b/internal/milter/sign.go @@ -380,7 +380,7 @@ func signLines(headers []field) ([]string, []byte) { func longHeaderLine(headers []field) string { for _, h := range headers { lines := strings.Split(h.value, "\n") - if len(h.name)+1+len(lines[0]) > smtpLineLimit { + if len(h.name)+1+len(strings.TrimRight(lines[0], "\r")) > smtpLineLimit { return h.name } for _, l := range lines[1:] { diff --git a/internal/web/auth/handlers.go b/internal/web/auth/handlers.go index 4feb89c..de5b05a 100644 --- a/internal/web/auth/handlers.go +++ b/internal/web/auth/handlers.go @@ -115,6 +115,7 @@ func (m *Module) submitLogin(w http.ResponseWriter, r *http.Request) { m.renderLogin(w, http.StatusTooManyRequests, "Too many attempts. Please wait and try again.") return } + limitForm(w, r) if err := r.ParseForm(); err != nil { m.renderLogin(w, http.StatusBadRequest, "Invalid form submission.") return @@ -250,6 +251,7 @@ func (m *Module) renderSetupForm(w http.ResponseWriter, status int, token, formE } func (m *Module) submitSetup(w http.ResponseWriter, r *http.Request, token string) { + limitForm(w, r) if err := r.ParseForm(); err != nil { m.renderSetupForm(w, http.StatusBadRequest, token, "Invalid form submission.") return diff --git a/internal/web/auth/limitkeys.go b/internal/web/auth/limitkeys.go index 93e32e8..9d9fde2 100644 --- a/internal/web/auth/limitkeys.go +++ b/internal/web/auth/limitkeys.go @@ -35,10 +35,24 @@ func limitAddr(r *http.Request, trusted []*net.IPNet) string { } // userKey is a limiter key for one username tried from one address. The -// name is folded as the store folds it (usernames are matched without -// regard to case). +// name is the stranger's text, of any length a form carries: the key holds +// its hash, not the name (the review of stages 2-6 kept a 5 MB name for +// fifteen minutes per request). Case is folded, so "Admin" and "admin" share +// a bucket - stricter than the store, which matches names exactly. func userKey(addr, username string) string { - return addr + "|" + strings.ToLower(strings.TrimSpace(username)) + sum := sha256.Sum256([]byte(strings.ToLower(strings.TrimSpace(username)))) + return addr + "|" + hex.EncodeToString(sum[:12]) +} + +// maxPublicForm bounds the body of a form a stranger can send - sign-in, +// reset, a link's page, setup - far below net/http's 10 MiB for a parsed +// form (net/http, parsePostForm: 10 << 20 when the body is not already a +// MaxBytesReader). +const maxPublicForm = 64 << 10 + +// limitForm caps the body ParseForm will read on a public form. +func limitForm(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxPublicForm) } // tokenKey is a limiter key for one link: its hash, so a limiter's map holds diff --git a/internal/web/auth/limitkeys_test.go b/internal/web/auth/limitkeys_test.go index f0876ab..2b2b24f 100644 --- a/internal/web/auth/limitkeys_test.go +++ b/internal/web/auth/limitkeys_test.go @@ -132,3 +132,19 @@ func TestDNSRechecksArePerUser(t *testing.T) { t.Error("another user's first re-check was refused") } } + +// A sign-in form past 64 KiB is refused before it is parsed, and the limiter +// keeps a short hash of a typed name, not the name (the review of stages +// 2-6: a 5 MB username stayed in the map for fifteen minutes). +func TestPublicFormsAreBoundedAndKeysShort(t *testing.T) { + m := moduleWithAdmin(t) + if rec := postLogin(m, "203.0.113.70:1", strings.Repeat("a", maxPublicForm), "x"); rec.Code != http.StatusBadRequest { + t.Errorf("a sign-in form past the bound = %d, want 400", rec.Code) + } + if k := userKey("203.0.113.70", strings.Repeat("a", 1<<20)); len(k) > 64 { + t.Errorf("the key of a 1 MiB name is %d bytes", len(k)) + } + if userKey("a", "Admin") != userKey("a", " admin ") || userKey("a", "admin") == userKey("a", "admin2") { + t.Error("userKey does not fold case and space, or merges two names") + } +} diff --git a/internal/web/auth/links.go b/internal/web/auth/links.go index 0340d1f..d39354a 100644 --- a/internal/web/auth/links.go +++ b/internal/web/auth/links.go @@ -150,12 +150,14 @@ func (m *Module) HandleReset(w http.ResponseWriter, r *http.Request) { tooMany() return } + limitForm(w, r) if err := r.ParseForm(); err != nil { m.view.Render(w, http.StatusBadRequest, "login-reset", view.NewReset(m.cfg.Hostname, "Invalid form submission.")) return } username := strings.TrimSpace(r.PostFormValue("username")) if !m.resetLimiter.Allow(userKey(addr, username)) { + m.resetAddrLimiter.Refund(addr) // counted once, against the name tooMany() return } @@ -204,6 +206,14 @@ func (m *Module) sendReset(username string) { } return } + // A day's place is spent only on a link that goes out (the review of + // stages 2-6). + sent := false + defer func() { + if !sent { + m.resetDayLimiter.Refund(key) + } + }() if !m.mailLimiter.Allow(key) { logf("panel: user %d: reset links asked for too often; none was sent", u.ID) return @@ -221,6 +231,7 @@ func (m *Module) sendReset(username string) { logf("panel: user %d: reset link not sent: %v", u.ID, err) return } + sent = true logf("panel: user %d: a reset link was sent", u.ID) } @@ -307,6 +318,7 @@ func (m *Module) setPasswordFromLink(w http.ResponseWriter, r *http.Request, tok fail := func(status int, msg string) { m.view.Render(w, status, "login-reset-set", view.NewResetSet(m.cfg.Hostname, u.Username, token, msg)) } + limitForm(w, r) if err := r.ParseForm(); err != nil { fail(http.StatusBadRequest, "Invalid form submission.") return diff --git a/internal/web/auth/ratelimit.go b/internal/web/auth/ratelimit.go index 14b74e0..f3dee87 100644 --- a/internal/web/auth/ratelimit.go +++ b/internal/web/auth/ratelimit.go @@ -75,12 +75,14 @@ func (r *rateLimiter) Allow(key string) bool { } // Refund gives back the attempt Allow took for key, for an attempt that -// turned out not to be a guess (a right password). A bucket already at its -// limit stays there: a refund never unblocks. +// turned out not to be a guess (a right password). It is called only after +// an Allow that admitted the attempt, so it undoes that one: the request that +// filled the bucket gives its own place back too (the review of stages 2-6: +// a right password at the ceiling used to leave the address blocked). func (r *rateLimiter) Refund(key string) { r.mu.Lock() defer r.mu.Unlock() - if b := r.buckets[key]; b != nil && b.count > 0 && b.count < r.max { + if b := r.buckets[key]; b != nil && b.count > 0 { b.count-- } }