diff --git a/CHANGELOG.md b/CHANGELOG.md index cc348e8..6d6d451 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version and name, and an IPv6 client is one per /64. With every request coming from one private address with `X-Forwarded-For` and `TRUSTED_PROXY_CIDR` empty, the log says what to set. Refused sign-ins are logged. +- **Reset links pause for a day after five** (audit A3): anyone who knew a + username could keep a live reset link arriving in that mailbox five times + an hour. After five in a day the owner is told once that further requests + are ignored for 24 hours. - **The TLS private key no longer has to be readable by Postfix** (audit I1): the guide gave it to group 10998 for the panel's backup, and `postfix` is in that group, so every Postfix daemon - an smtpd talking to diff --git a/docs/guide.md b/docs/guide.md index 413c7b9..5b2b287 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -605,7 +605,8 @@ Notifications go to the address either way. Only a confirmed address can be sent a reset link: *Forgot your password?* on the sign-in page asks for the username and always answers the same - whether the name exists, and whether its address is confirmed, is not said; another link is not sent within ten -minutes of the last - and the link +minutes of the last, and after five in a day the owner is mailed once that +further requests are ignored for 24 hours - and the link (`https:///login/reset/…`) works once, for 30 minutes, and only while the account's address is still the one it was sent to. Setting the new password signs every session of the account out. The links name the diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 8404f3f..b1078c8 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -787,8 +787,9 @@ the argument. key. — **Sonnet**, 0.5 h. *Done 2026-10-17* (`refusalLog`). - [x] **A9.** IPv6 keyed by /64. — **Sonnet**, 0.5 h. *Done 2026-10-17:* TestIPv6IsLimitedPerSlash64. -- [ ] **A3.** After five unsolicited reset links in a day, one mail saying +- [x] **A3.** After five unsolicited reset links in a day, one mail saying further requests are ignored for 24 h, then silence. — **Opus**, 1.5 h. + *Done 2026-10-17:* TestResetLinksPauseForADayAfterFive. - [ ] **I6.** `/healthz` answered only to loopback and trusted-proxy peers; the guide's "Container health" says the proxy may expose it on purpose with the fragment line. — **Sonnet**, 0.5 h. diff --git a/docs/security.md b/docs/security.md index db7bd18..e557326 100644 --- a/docs/security.md +++ b/docs/security.md @@ -176,7 +176,10 @@ The panel is exposed to the internet — the items below are **not optional**. after answering, so neither the words nor the time tell whether a name exists or is confirmed; requests are limited to five in 15 minutes per client address and username (fifty per address alone), mails to five an - hour per account, and a link's page to 30 requests in 15 minutes per link + 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 (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/web/auth/auth.go b/internal/web/auth/auth.go index 0efa808..633a3ac 100644 --- a/internal/web/auth/auth.go +++ b/internal/web/auth/auth.go @@ -49,6 +49,11 @@ type Module struct { linkLimiter *rateLimiter linkAddrLimiter *rateLimiter mailLimiter *rateLimiter + // resetDayLimiter counts the reset links mailed to one account in a + // day, resetNoticeLimiter the one notice that further requests are + // ignored (links.go, sendReset). + resetDayLimiter *rateLimiter + resetNoticeLimiter *rateLimiter proxyWatch proxyWatch refusals *refusalLog @@ -88,8 +93,12 @@ func New(st *store.Store, cfg Config, v *view.Engine, setupTokenPath string) *Mo // Five links an hour to one address, of either kind: three resets // asked for and a couple of confirmations is ordinary use, a mailbox // full of them is not. - mailLimiter: newRateLimiter(5, time.Hour), - trustedProxies: cfg.TrustedProxyCIDRs, + mailLimiter: newRateLimiter(5, time.Hour), + // Five reset links a day to one account, then one notice and + // silence for the rest of the day (security audit 2026-10-17, A3). + resetDayLimiter: newRateLimiter(resetsPerDay, 24*time.Hour), + resetNoticeLimiter: newRateLimiter(1, 24*time.Hour), + trustedProxies: cfg.TrustedProxyCIDRs, } m.setupLimiter.startSweeper() m.loginLimiter.startSweeper() @@ -101,6 +110,8 @@ func New(st *store.Store, cfg Config, v *view.Engine, setupTokenPath string) *Mo m.resetLimiter.startSweeper() m.linkLimiter.startSweeper() m.mailLimiter.startSweeper() + m.resetDayLimiter.startSweeper() + m.resetNoticeLimiter.startSweeper() // Made now rather than at the first unknown name, which would otherwise // pay for two bcrypt runs (review of audit stage 4). go noSuchUserHash() diff --git a/internal/web/auth/links.go b/internal/web/auth/links.go index 48ad296..08cbae2 100644 --- a/internal/web/auth/links.go +++ b/internal/web/auth/links.go @@ -29,6 +29,12 @@ const ( // spend the account's links an hour in a minute (security review, // 2026-10-17). resetSpacing = 10 * time.Minute + // resetsPerDay is how many reset links one account is mailed in a day. + // Anyone who knows a username can ask for them; past this the owner is + // told once that further requests are ignored for a day, and then nothing + // more is sent - rather than a live link in the mailbox five times an + // hour for ever (security audit 2026-10-17, A3). + resetsPerDay = 5 ) // later runs f after the answer, counted for Wait. @@ -182,7 +188,22 @@ func (m *Module) sendReset(username string) { logf("panel: user %d: a reset link went less than %s ago; none was sent", u.ID, resetSpacing) return } - if !m.mailLimiter.Allow("reset:" + strconv.FormatInt(u.ID, 10)) { + key := "reset:" + strconv.FormatInt(u.ID, 10) + if !m.resetDayLimiter.Allow(key) { + if !m.resetNoticeLimiter.Allow(key) { + logf("panel: user %d: reset links asked for again after today's %d; nothing was sent", u.ID, resetsPerDay) + return + } + logf("panel: user %d: reset links asked for more than %d times today; the owner is told further requests are ignored for 24 hours", u.ID, resetsPerDay) + body := "Reset links for the SelfPost account " + u.Username + " at " + m.cfg.Hostname + " have been asked for more than " + + strconv.Itoa(resetsPerDay) + " times today. Further requests are ignored for 24 hours, and no more mail of this kind will come before then.\n\n" + + "If it was not you, nothing needs doing: the password stays as it is. A global administrator can set a new password on Server > Users at any time.\n" + if err := m.mailer.Send(u.Email, "[SelfPost] Reset links paused for a day", body); err != nil { + logf("panel: user %d: notice not sent: %v", u.ID, err) + } + return + } + if !m.mailLimiter.Allow(key) { logf("panel: user %d: reset links asked for too often; none was sent", u.ID) return } diff --git a/internal/web/auth/links_test.go b/internal/web/auth/links_test.go index 1519e36..66e574a 100644 --- a/internal/web/auth/links_test.go +++ b/internal/web/auth/links_test.go @@ -1,6 +1,7 @@ package auth import ( + "github.com/mixeme/selfpost/internal/store" "net/http" "net/http/httptest" "net/url" @@ -299,3 +300,33 @@ func TestLinkPathsWithoutAToken(t *testing.T) { } } } + +// Five reset links a day to one account, then one notice that further +// requests are ignored for a day, then nothing (security audit 2026-10-17, +// A3). The ten-minute spacing is stepped over by removing the last link. +func TestResetLinksPauseForADayAfterFive(t *testing.T) { + m, ml := linkModule(t, true) + u, _ := m.store.GetUserByUsername("admin") + for i := 0; i < resetsPerDay+3; i++ { + if err := m.store.DeleteUserLinks(u.ID, store.LinkReset); err != nil { + t.Fatal(err) + } + m.sendReset("admin") + got, sent := ml.next(t) + switch { + case i < resetsPerDay: + if !sent || !resetLinkRe.MatchString(got.body) { + t.Fatalf("link %d: sent %v, %+v", i+1, sent, got) + } + case i == resetsPerDay: + if !sent || got.subject != "[SelfPost] Reset links paused for a day" || resetLinkRe.MatchString(got.body) || + !strings.Contains(got.body, "ignored for 24 hours") { + t.Fatalf("the notice: sent %v, %+v", sent, got) + } + default: + if sent { + t.Fatalf("request %d after the notice sent %+v", i+1, got) + } + } + } +}