findings of the security review of stage 0
The review of 2c27275..2c8e981 (read-only, a separate agent) found no high or medium; three small ones, fixed here: - reauth took a limiter attempt before the compare, right password included: with A2 asking on every user save, ten saves in fifteen minutes locked the administrator out of Account's password change, the backup and the export. A right password now gives its attempt back (rateLimiter.Refund, Module.PasswordCheckPassed; a bucket already at its limit stays blocked). TestRightAccountPasswordsDoNotUseUpTheLimit. - LoggedPath hid the tokens of /login/reset/ and /confirm/ but not of the first administrator's /setup/<token>; now it does (test case added). - The entrypoint's own `[ -e ] || : >` lines (sender_login_maps, the four inbound maps, mail.log) get the `[ -L ]` guard postfix-config.sh got for I2. Safe before too - drop_links runs first on those paths and the panel is not running yet - this is the belt and braces. The review also confirmed, with a probe of the regex against Postfix's formats, that a port-25 client can place only its own envelope in an Inbound log row, and that every path to a password, e-mail or role asks for the signed-in password. go test ./internal/web/handlers ./internal/web/auth; sh -n entrypoint.sh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+2
-1
@@ -12,7 +12,8 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
|
|||||||
password without the current one, which then passed the backup's password
|
password without the current one, which then passed the backup's password
|
||||||
check. Adding a user, setting a password, changing an e-mail or a role on
|
check. Adding a user, setting a password, changing an e-mail or a role on
|
||||||
the user form, and deleting a user now ask for your own account password,
|
the user form, and deleting a user now ask for your own account password,
|
||||||
as a backup does, under the same limit on wrong guesses.
|
as a backup does, under the same limit on wrong guesses - which now
|
||||||
|
counts wrong passwords only.
|
||||||
- **A domain's rate limit is taken only by its own applications** (audit
|
- **A domain's rate limit is taken only by its own applications** (audit
|
||||||
M1): the level-2 domain limit was read from `MAIL FROM`, which reaches the
|
M1): the level-2 domain limit was read from `MAIL FROM`, which reaches the
|
||||||
milter before Postfix holds it to the login, so a client that never
|
milter before Postfix holds it to the login, so a client that never
|
||||||
|
|||||||
+3
-3
@@ -150,11 +150,11 @@ rm -f /data/sasl/sasldb2.new
|
|||||||
# Postfix state under /data (architecture.md § Persistence). The panel writes
|
# Postfix state under /data (architecture.md § Persistence). The panel writes
|
||||||
# sender_login_maps; Postfix owns the on-disk queue tree under queue/.
|
# sender_login_maps; Postfix owns the on-disk queue tree under queue/.
|
||||||
mkdir -p /data/postfix/queue
|
mkdir -p /data/postfix/queue
|
||||||
[ -e /data/postfix/sender_login_maps ] || : > /data/postfix/sender_login_maps
|
[ -e /data/postfix/sender_login_maps ] || [ -L /data/postfix/sender_login_maps ] || : > /data/postfix/sender_login_maps
|
||||||
# Inbound relay maps (written by the panel when INBOUND_RELAY_ENABLE=true).
|
# Inbound relay maps (written by the panel when INBOUND_RELAY_ENABLE=true).
|
||||||
# Empty files keep postfix check happy if the flag is on before any domain exists.
|
# Empty files keep postfix check happy if the flag is on before any domain exists.
|
||||||
for f in relay_domains transport relay_recipients tls_policy; do
|
for f in relay_domains transport relay_recipients tls_policy; do
|
||||||
[ -e "/data/postfix/$f" ] || : > "/data/postfix/$f"
|
[ -e "/data/postfix/$f" ] || [ -L "/data/postfix/$f" ] || : > "/data/postfix/$f"
|
||||||
done
|
done
|
||||||
# Every map here is the panel's to write and Postfix's to read - the sender
|
# Every map here is the panel's to write and Postfix's to read - the sender
|
||||||
# map, the inbound and the DMARC maps alike - so each file beside queue/ is
|
# map, the inbound and the DMARC maps alike - so each file beside queue/ is
|
||||||
@@ -196,7 +196,7 @@ chmod 0755 /data/antispam
|
|||||||
# because parent directory has insecure permissions" when the directory's
|
# because parent directory has insecure permissions" when the directory's
|
||||||
# group is not root and has write permission.
|
# group is not root and has write permission.
|
||||||
mkdir -p /data/log
|
mkdir -p /data/log
|
||||||
[ -e /data/log/mail.log ] || : > /data/log/mail.log
|
[ -e /data/log/mail.log ] || [ -L /data/log/mail.log ] || : > /data/log/mail.log
|
||||||
chown -R postfix:selfpost /data/log
|
chown -R postfix:selfpost /data/log
|
||||||
chmod 2750 /data/log
|
chmod 2750 /data/log
|
||||||
find /data/log -type f -exec chmod 0640 {} +
|
find /data/log -type f -exec chmod 0640 {} +
|
||||||
|
|||||||
+4
-1
@@ -503,7 +503,10 @@ operator can act on it.
|
|||||||
user and when a save sets a password, changes the e-mail (where reset
|
user and when a save sets a password, changes the e-mail (where reset
|
||||||
links go) or changes the role - the signed-in user's own account included -
|
links go) or changes the role - the signed-in user's own account included -
|
||||||
and deleting a user asks for it too. A save of the name and the domain
|
and deleting a user asks for it too. A save of the name and the domain
|
||||||
assignments alone is not asked: neither reaches an account's secret.
|
assignments alone is not asked: neither reaches an account's secret. Only
|
||||||
|
wrong passwords count against the limiter; a right one gives its attempt
|
||||||
|
back, so an administrator adding users is not locked out of the next
|
||||||
|
backup.
|
||||||
- Both files are secrets: a full backup carries DKIM keys, `sasldb2`, the
|
- Both files are secrets: a full backup carries DKIM keys, `sasldb2`, the
|
||||||
panel users' password hashes, `docker-compose.yml`, `.env`, and the TLS
|
panel users' password hashes, `docker-compose.yml`, `.env`, and the TLS
|
||||||
private key from `certs/` when present; a domain export carries the DKIM key
|
private key from `certs/` when present; a domain export carries the DKIM key
|
||||||
|
|||||||
@@ -100,6 +100,14 @@ func (m *Module) AllowPasswordChange(userID int64) bool {
|
|||||||
return m.passwordLimiter.Allow("user:" + strconv.FormatInt(userID, 10))
|
return m.passwordLimiter.Allow("user:" + strconv.FormatInt(userID, 10))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// PasswordCheckPassed gives back the attempt AllowPasswordChange took for a
|
||||||
|
// check that found the right password: only wrong guesses count, so an
|
||||||
|
// administrator who adds ten users in a quarter of an hour is not locked out
|
||||||
|
// of the next backup (review of audit 2026-10-17 stage 0).
|
||||||
|
func (m *Module) PasswordCheckPassed(userID int64) {
|
||||||
|
m.passwordLimiter.Refund("user:" + strconv.FormatInt(userID, 10))
|
||||||
|
}
|
||||||
|
|
||||||
// SessionToken returns the session token the request carries, if exactly one
|
// SessionToken returns the session token the request carries, if exactly one
|
||||||
// cookie of that name is present.
|
// cookie of that name is present.
|
||||||
func (m *Module) SessionToken(r *http.Request) (string, bool) {
|
func (m *Module) SessionToken(r *http.Request) (string, bool) {
|
||||||
|
|||||||
@@ -407,7 +407,7 @@ func loginInfo(r *http.Request) string {
|
|||||||
// Every log line that names a request's path goes through it: the origin
|
// Every log line that names a request's path goes through it: the origin
|
||||||
// check's refusal and the duplicate-cookie line (audit 2026-10-17, A4).
|
// check's refusal and the duplicate-cookie line (audit 2026-10-17, A4).
|
||||||
func LoggedPath(p string) string {
|
func LoggedPath(p string) string {
|
||||||
for _, prefix := range []string{"/login/reset/", "/confirm/"} {
|
for _, prefix := range []string{"/login/reset/", "/confirm/", "/setup/"} {
|
||||||
if strings.HasPrefix(p, prefix) && len(p) > len(prefix) {
|
if strings.HasPrefix(p, prefix) && len(p) > len(prefix) {
|
||||||
return prefix + "…"
|
return prefix + "…"
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ func TestLoggedPathHidesTokens(t *testing.T) {
|
|||||||
for in, want := range map[string]string{
|
for in, want := range map[string]string{
|
||||||
"/login/reset/abcDEF123": "/login/reset/…",
|
"/login/reset/abcDEF123": "/login/reset/…",
|
||||||
"/confirm/abcDEF123": "/confirm/…",
|
"/confirm/abcDEF123": "/confirm/…",
|
||||||
|
"/setup/abcDEF123": "/setup/…",
|
||||||
"/login/reset": "/login/reset",
|
"/login/reset": "/login/reset",
|
||||||
"/confirm/": "/confirm/",
|
"/confirm/": "/confirm/",
|
||||||
"/account/profile": "/account/profile",
|
"/account/profile": "/account/profile",
|
||||||
|
|||||||
@@ -74,6 +74,17 @@ func (r *rateLimiter) Allow(key string) bool {
|
|||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// 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.
|
||||||
|
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 {
|
||||||
|
b.count--
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func (r *rateLimiter) makeRoom(now time.Time) {
|
func (r *rateLimiter) makeRoom(now time.Time) {
|
||||||
if r.maxBuckets <= 0 || len(r.buckets) < r.maxBuckets {
|
if r.maxBuckets <= 0 || len(r.buckets) < r.maxBuckets {
|
||||||
return
|
return
|
||||||
|
|||||||
@@ -353,6 +353,7 @@ func (h *Handlers) reauth(r *http.Request, what string) (int, string) {
|
|||||||
logf("panel: %s: user %d gave a wrong account password", what, p.ID)
|
logf("panel: %s: user %d gave a wrong account password", what, p.ID)
|
||||||
return http.StatusUnauthorized, "Your account password is incorrect."
|
return http.StatusUnauthorized, "Your account password is incorrect."
|
||||||
}
|
}
|
||||||
|
h.auth.PasswordCheckPassed(p.ID)
|
||||||
return 0, ""
|
return 0, ""
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -134,3 +134,18 @@ func TestUserFormAsksForTheAccountPassword(t *testing.T) {
|
|||||||
t.Fatalf("delete with the right password = %d:\n%s", rec.Code, rec.Body.String())
|
t.Fatalf("delete with the right password = %d:\n%s", rec.Code, rec.Body.String())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Only wrong guesses count against the account-password limit: a dozen
|
||||||
|
// right ones in a row - an administrator adding users - leave the next
|
||||||
|
// backup open (review of audit 2026-10-17 stage 0).
|
||||||
|
func TestRightAccountPasswordsDoNotUseUpTheLimit(t *testing.T) {
|
||||||
|
h, st := inboundHandlers(t)
|
||||||
|
root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
|
||||||
|
withAccountPassword(t, h, st, root)
|
||||||
|
for i := 0; i < 12; i++ {
|
||||||
|
form := url.Values{"username": {"new-user-" + idStr(int64(i))}, "password": {"a-long-enough-password-1"}, "role": {"global"}, "account_password": {testAccountPassword}}
|
||||||
|
if rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, form); rec.Code != http.StatusSeeOther {
|
||||||
|
t.Fatalf("user %d = %d:\n%.600s", i, rec.Code, rec.Body.String())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user