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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user