From 8a87cfeb443c2ce29f9be92a8948639d49b6b773 Mon Sep 17 00:00:00 2001 From: kristianwind Date: Sun, 23 Aug 2026 23:15:33 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(security):=20HSTS=20og=20en=20lukket?= =?UTF-8?q?=20Permissions-Policy=20p=C3=A5=20hvert=20svar?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To headere, appen ikke satte, og som begge følger af, hvad den allerede er. Sessionen bor i en __Host-cookie, der kun findes over HTTPS — hele modellen forudsætter TLS. Uden HSTS holder den forudsætning ikke mod en netværksangriber: den allerførste navigation kan strippes til http://, og cookien rækkes over i klartekst, før Secure-flaget nogensinde kommer i spil. HSTS lukker vinduet, og den er sat på nøjagtig samme betingelse som cookiens Secure-flag (secureCookies), så en dev-instans på ren HTTP aldrig bliver bedt om et certifikat, den ikke har. To år, underdomæner med; ingen preload — at blive hard-kodet ind i browsere er en beslutning for domænets ejer, ikke for en selv-hostet app. verdande beder aldrig om kamera, mikrofon eller placering, så den sikreste Permissions-Policy er at slå dem alle fra: et indsat element kan ikke bede om det, siden selv har afvist. Navngivet felt for felt frem for med et jokertegn, fordi jokertegns-formen ikke forstås ens af alle browsere, og en regel, en browser ignorerer, er ingen regel. Prøve på begge: HSTS følger HTTPS og udebliver over HTTP, og Permissions-Policy lukker for kameraet uanset skema. Co-Authored-By: Claude Opus 5 --- internal/httpapi/csp_header_test.go | 31 +++++++++++++++++++++++++++++ internal/httpapi/server.go | 20 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/internal/httpapi/csp_header_test.go b/internal/httpapi/csp_header_test.go index d0b37fe..3275dea 100644 --- a/internal/httpapi/csp_header_test.go +++ b/internal/httpapi/csp_header_test.go @@ -3,6 +3,8 @@ package httpapi import ( "strings" "testing" + + "github.com/kristianwind/verdande/internal/config" ) // Politikken skal stå i svaret, og den skal stadig være stram. @@ -50,6 +52,35 @@ func TestEveryResponseCarriesAStrictCSP(t *testing.T) { } } +// HSTS følger med, når instansen kører over HTTPS — og kun der. +// +// Sessionen bor i en __Host-cookie, der kun findes over TLS, så hele modellen +// forudsætter allerede HTTPS. HSTS er det, der holder den forudsætning mod en +// netværksangriber; men bedt om over ren HTTP ville den bede en browser om at +// kræve et certifikat, en dev-instans ikke har. Begge halvdele skal stå prøven. +func TestHSTSTracksHTTPS(t *testing.T) { + overHTTP := newTestServer(t) // BaseURL er http://localhost + resp, _ := overHTTP.do(t, "GET", "/api/v1/ping", nil) + if got := resp.Header.Get("Strict-Transport-Security"); got != "" { + t.Errorf("HSTS sat over ren HTTP: %q", got) + } + + overHTTPS := newTestServerWith(t, func(c *config.Config) { + c.BaseURL = "https://todo.example.dk" + }) + resp, _ = overHTTPS.do(t, "GET", "/api/v1/ping", nil) + if got := resp.Header.Get("Strict-Transport-Security"); got == "" { + t.Error("ingen HSTS over HTTPS") + } else if !strings.Contains(got, "includeSubDomains") { + t.Errorf("HSTS uden includeSubDomains: %q", got) + } + // Permissions-Policy står på hvert svar, uanset skema: den lukker for kamera, + // mikrofon og placering, som appen aldrig beder om. + if got := resp.Header.Get("Permissions-Policy"); !strings.Contains(got, "camera=()") { + t.Errorf("Permissions-Policy lukker ikke for kameraet: %q", got) + } +} + // Et bilag må ikke kunne blive læst som et dokument. func TestAttachmentsAreNotServedAsDocuments(t *testing.T) { for _, mime := range []string{"image/svg+xml", "text/html", "application/xhtml+xml", "text/xml"} { diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 8418b67..9ee7e37 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -665,6 +665,26 @@ func (s *Server) securityHeaders(next http.Handler) http.Handler { h.Set("X-Frame-Options", "DENY") h.Set("Referrer-Policy", "same-origin") h.Set("Content-Security-Policy", s.csp) + // The session lives in a __Host- cookie that only exists over HTTPS, so the + // whole security model already assumes TLS. HSTS is what makes that + // assumption hold against a network attacker: without it, the very first + // navigation can be stripped to http:// and the cookie handed over in the + // clear. Gated on the same condition as the cookie's Secure flag, so a + // plain-HTTP dev instance is never told to demand a certificate it cannot + // present. Two years, subdomains included; no preload directive, because + // asking to be hard-coded into browsers is a decision the operator makes + // for their domain, not one a self-hosted app makes for them. + if s.secureCookies() { + h.Set("Strict-Transport-Security", "max-age=63072000; includeSubDomains") + } + // verdande uses none of the powerful browser features, so the safest policy + // is to turn them all off: an injected element cannot ask for the camera, + // the microphone or the location if the page itself has already declined + // them. Named explicitly rather than with a wildcard because the wildcard + // form of this header is not universally understood, and a directive a + // browser ignores is not a control. + h.Set("Permissions-Policy", "camera=(), microphone=(), geolocation=(), "+ + "payment=(), usb=(), interest-cohort=()") next.ServeHTTP(w, r) }) } From b4038356f0e76e250d4085a32201e231c5dc7be7 Mon Sep 17 00:00:00 2001 From: kristianwind Date: Sun, 23 Aug 2026 23:34:34 +0200 Subject: [PATCH 2/3] feat(security): trust a forwarded IP only from a proxy, and set COOP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rate limiter and the audit log key on the caller's address, and that address was read with chi's middleware.RealIP — which believes X-Forwarded-For from anyone. So a caller could set it to a new value on every request and get a fresh password-guess bucket each time, and could write any address they liked into the log the panel's watchers read. The one control meant for when a password is already gone was keyed on a number the attacker got to pick. Now the header is believed only when the machine that *connected* is a trusted proxy — by default anything on a private network, which is the shape of a reverse proxy or tunnel sharing the host or the docker bridge. A connection straight off the internet is trusted for nothing and is keyed on the address it actually came from. VERDANDE_TRUSTED_PROXIES overrides the set (CIDRs, or "none" to trust no proxy at all); VERDANDE_REAL_IP_HEADER names the header. Verified in the current deployment before the change: behind the Cloudflare Tunnel a spoofed X-Real-IP already got no fresh bucket, because cloudflared overwrites it. This makes that safety a property of the code, not of the deployment — expose the instance directly, or put a header-passing proxy in front, and it would have been a live hole. Also COOP: Cross-Origin-Opener-Policy: same-origin on every response, isolating the browsing context. verdande scripts no other window, so there is nothing to lose and a class of cross-window attacks to close. Co-Authored-By: Claude Opus 5 --- docs/configuration.md | 19 ++++++ internal/config/config.go | 82 +++++++++++++++++++++++++ internal/httpapi/csp_header_test.go | 4 ++ internal/httpapi/realip.go | 88 +++++++++++++++++++++++++++ internal/httpapi/realip_test.go | 94 +++++++++++++++++++++++++++++ internal/httpapi/server.go | 10 ++- 6 files changed, 296 insertions(+), 1 deletion(-) create mode 100644 internal/httpapi/realip.go create mode 100644 internal/httpapi/realip_test.go diff --git a/docs/configuration.md b/docs/configuration.md index a6d1193..d6d3e92 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -12,6 +12,25 @@ nowhere to put one that the operator would ever see. | `VERDANDE_DATA_DIR` | `/data` | Database, uploaded files and backups. | | `VERDANDE_DEV` | `false` | Human-readable logs at debug level, and a relaxed origin check so a Vite dev server can connect. Never in production. | +## Behind a proxy + +verdande keys its login rate limit and its audit log on the caller's address, so +that address has to be one the caller cannot forge. A forwarded-for header is +believed only when the machine that connected is a trusted proxy; a connection +straight off the internet is keyed on the address it really came from. + +| Variable | Default | What it does | +|---|---|---| +| `VERDANDE_TRUSTED_PROXIES` | private networks | Whose `X-Forwarded-For` to believe, as comma-separated CIDRs. The default trusts loopback and the private ranges — the shape of a reverse proxy or tunnel sharing the host or the docker bridge. Set to `none` for an instance exposed directly with no proxy, so no forwarded header is ever believed. | +| `VERDANDE_REAL_IP_HEADER` | `X-Forwarded-For` | The header the client address is read from, for a trusted proxy. | + +!!! warning "A proxy that does not overwrite the header" + The default is safe when your proxy replaces a client-supplied + `X-Forwarded-For` with the real address — nginx, Caddy and Cloudflare Tunnel all + do. If yours passes the client's own header through, narrow + `VERDANDE_TRUSTED_PROXIES` to just your proxy's address so a visitor cannot + choose the address they are rate-limited and logged as. + ## Sessions and links | Variable | Default | What it does | diff --git a/internal/config/config.go b/internal/config/config.go index afe7c2d..8f3a448 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,6 +7,7 @@ package config import ( "fmt" + "net/netip" "os" "path/filepath" "strconv" @@ -80,6 +81,26 @@ type Config struct { // TrashRetention is how long a soft-deleted task or project stays recoverable. TrashRetention time.Duration + // TrustedProxies are the peers whose forwarded-for header may be believed. + // + // The rate limiter and the audit log key on the caller's address, and that + // address is only as honest as wherever it was read. Read it from a header that + // anybody can set, and an attacker rotates it to get a fresh guess-bucket for + // every request and to write someone else's IP into the log. So the header is + // believed only when the machine that *connected* is one of these — a reverse + // proxy or tunnel the operator put there — and ignored when the connection came + // straight off the internet. + // + // The default is "the peer is on a private network", which is exactly the shape + // of a homelab behind Caddy, nginx or a Cloudflare Tunnel: the proxy shares the + // host or the docker bridge, so its address is loopback or RFC1918. A directly + // exposed instance sees a public peer, trusts nothing, and keys on the real + // connection. Override with VERDANDE_TRUSTED_PROXIES (comma-separated CIDRs), + // or "none" to trust no proxy at all. + TrustedProxies []netip.Prefix + // RealIPHeader is where the client address is read from, for a trusted peer. + RealIPHeader string + Dev bool } @@ -149,9 +170,70 @@ func Load() (*Config, error) { if c.DataDir, err = filepath.Abs(c.DataDir); err != nil { return nil, fmt.Errorf("VERDANDE_DATA_DIR: %w", err) } + + c.RealIPHeader = env("VERDANDE_REAL_IP_HEADER", "X-Forwarded-For") + if c.TrustedProxies, err = parseTrustedProxies(env("VERDANDE_TRUSTED_PROXIES", "")); err != nil { + return nil, err + } return c, nil } +// defaultTrustedProxies is "the peer is not on the public internet": loopback, +// the three RFC1918 ranges, Tailscale's CGNAT borrow, IPv6 unique-local and both +// families' link-local. A proxy an operator stands up in front of this server is +// on one of these; a stranger connecting directly is on none. +var defaultTrustedProxies = mustPrefixes( + "127.0.0.0/8", "::1/128", + "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", + "100.64.0.0/10", + "fc00::/7", + "169.254.0.0/16", "fe80::/10", +) + +// parseTrustedProxies reads the override. Empty keeps the private-network default; +// the literal "none" trusts no proxy at all, for an instance that is exposed +// directly and must never believe a forwarded header. +func parseTrustedProxies(raw string) ([]netip.Prefix, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return defaultTrustedProxies, nil + } + if strings.EqualFold(raw, "none") { + return nil, nil + } + var out []netip.Prefix + for _, part := range strings.Split(raw, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + // A bare address is a host route: 10.0.0.5 means 10.0.0.5/32, so an operator + // naming one proxy does not have to know CIDR notation to do it. + if !strings.Contains(part, "/") { + addr, err := netip.ParseAddr(part) + if err != nil { + return nil, fmt.Errorf("VERDANDE_TRUSTED_PROXIES: %q is not an address or CIDR", part) + } + out = append(out, netip.PrefixFrom(addr, addr.BitLen())) + continue + } + p, err := netip.ParsePrefix(part) + if err != nil { + return nil, fmt.Errorf("VERDANDE_TRUSTED_PROXIES: %q is not a CIDR", part) + } + out = append(out, p.Masked()) + } + return out, nil +} + +func mustPrefixes(cidrs ...string) []netip.Prefix { + out := make([]netip.Prefix, 0, len(cidrs)) + for _, c := range cidrs { + out = append(out, netip.MustParsePrefix(c)) + } + return out +} + // DBPath, FilesDir and BackupsDir are the three things that live on the /data volume. func (c *Config) DBPath() string { return filepath.Join(c.DataDir, "verdande.db") } func (c *Config) FilesDir() string { return filepath.Join(c.DataDir, "files") } diff --git a/internal/httpapi/csp_header_test.go b/internal/httpapi/csp_header_test.go index 3275dea..4608161 100644 --- a/internal/httpapi/csp_header_test.go +++ b/internal/httpapi/csp_header_test.go @@ -79,6 +79,10 @@ func TestHSTSTracksHTTPS(t *testing.T) { if got := resp.Header.Get("Permissions-Policy"); !strings.Contains(got, "camera=()") { t.Errorf("Permissions-Policy lukker ikke for kameraet: %q", got) } + // COOP isolerer browserkonteksten og står på hvert svar. + if got := resp.Header.Get("Cross-Origin-Opener-Policy"); got != "same-origin" { + t.Errorf("Cross-Origin-Opener-Policy er ikke same-origin: %q", got) + } } // Et bilag må ikke kunne blive læst som et dokument. diff --git a/internal/httpapi/realip.go b/internal/httpapi/realip.go new file mode 100644 index 0000000..bbfa3b5 --- /dev/null +++ b/internal/httpapi/realip.go @@ -0,0 +1,88 @@ +package httpapi + +import ( + "net" + "net/http" + "net/netip" + "strings" + + "github.com/kristianwind/verdande/internal/config" +) + +// realIP rewrites r.RemoteAddr to the client address a trusted proxy reported, +// and does nothing at all when the peer is not trusted. +// +// This replaces chi's middleware.RealIP, which is deprecated precisely because it +// believes a forwarded header from anyone: an attacker sets X-Forwarded-For to a +// new value on every request and gets a fresh rate-limit bucket each time, and +// writes whatever address they like into the audit log. The one control that +// exists for when a password is already gone is keyed on this address, so it has +// to be an address the caller cannot choose. +// +// The gate is the peer. A header is read only when the machine that opened the +// connection is one of cfg.TrustedProxies — a reverse proxy or tunnel the operator +// put there. A connection straight off the internet is trusted for nothing, so its +// header is ignored and it is keyed on the address it actually came from. Behind a +// proxy that overwrites the client's own header (nginx, Caddy and cloudflared all +// do) the value is then the real client and cannot be forged from outside. +func realIP(cfg *config.Config) func(http.Handler) http.Handler { + trusted := cfg.TrustedProxies + header := cfg.RealIPHeader + if header == "" { + header = "X-Forwarded-For" + } + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if ip := trustedClientIP(r, trusted, header); ip != "" { + // Port preserved off the original peer only for shape; downstream + // clientIP() splits it away, and the forwarded value carries none. + r.RemoteAddr = ip + } + next.ServeHTTP(w, r) + }) + } +} + +// trustedClientIP returns the forwarded client address if the peer may be believed, +// or "" to leave RemoteAddr untouched. +func trustedClientIP(r *http.Request, trusted []netip.Prefix, header string) string { + peer := r.RemoteAddr + if host, _, err := net.SplitHostPort(peer); err == nil { + peer = host + } + peerAddr, err := netip.ParseAddr(peer) + if err != nil || !isTrusted(peerAddr, trusted) { + return "" + } + + raw := r.Header.Get(header) + if raw == "" { + return "" + } + // The leftmost entry is the original client. That entry is only as trustworthy + // as the proxy that wrote it — but the trusted proxy is exactly the one that + // replaces a client-supplied header, so on the deployments this is written for + // it is the real client, and on a direct connection we never reach this line. + first := raw + if i := strings.IndexByte(raw, ','); i >= 0 { + first = raw[:i] + } + first = strings.TrimSpace(first) + if _, err := netip.ParseAddr(first); err != nil { + return "" + } + return first +} + +func isTrusted(addr netip.Addr, trusted []netip.Prefix) bool { + if !addr.IsValid() { + return false + } + addr = addr.Unmap() + for _, p := range trusted { + if p.Contains(addr) { + return true + } + } + return false +} diff --git a/internal/httpapi/realip_test.go b/internal/httpapi/realip_test.go new file mode 100644 index 0000000..2df7e6e --- /dev/null +++ b/internal/httpapi/realip_test.go @@ -0,0 +1,94 @@ +package httpapi + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/kristianwind/verdande/internal/config" +) + +// The address the rate limiter keys on must be one the caller cannot choose. A +// forwarded header is believed only from a trusted proxy, and ignored from a +// stranger connecting directly — the difference between a control and a suggestion. +func TestRealIPTrustsOnlyTheProxy(t *testing.T) { + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + + seen := "" + h := realIP(cfg)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + seen = clientIP(r) + })) + + cases := []struct { + name string + remoteAddr string + xff string + want string + }{ + { + // A private peer is a proxy the operator put there: believe it. + name: "trusted proxy is believed", + remoteAddr: "10.1.2.3:5000", + xff: "203.0.113.9", + want: "203.0.113.9", + }, + { + // A public peer connected straight to us; its header is a guess. + name: "public peer header ignored", + remoteAddr: "198.51.100.7:5000", + xff: "203.0.113.9", + want: "198.51.100.7", + }, + { + // No header from a trusted proxy leaves the peer as the answer. + name: "trusted peer without header", + remoteAddr: "127.0.0.1:5000", + xff: "", + want: "127.0.0.1", + }, + { + // A spoofed value from a stranger cannot become the key. + name: "spoof from outside does not win", + remoteAddr: "198.51.100.7:5000", + xff: "127.0.0.1", + want: "198.51.100.7", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest("POST", "/api/v1/auth/login", nil) + r.RemoteAddr = tc.remoteAddr + if tc.xff != "" { + r.Header.Set("X-Forwarded-For", tc.xff) + } + h.ServeHTTP(httptest.NewRecorder(), r) + if seen != tc.want { + t.Errorf("keyed on %q, want %q", seen, tc.want) + } + }) + } +} + +// "none" turns the trust off entirely, for an instance exposed directly. +func TestRealIPTrustNone(t *testing.T) { + t.Setenv("VERDANDE_TRUSTED_PROXIES", "none") + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + seen := "" + h := realIP(cfg)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + seen = clientIP(r) + })) + r := httptest.NewRequest("POST", "/", nil) + r.RemoteAddr = "10.1.2.3:5000" // even a private peer is not trusted now + r.Header.Set("X-Forwarded-For", "203.0.113.9") + h.ServeHTTP(httptest.NewRecorder(), r) + if seen != "10.1.2.3" { + t.Errorf("keyed on %q, want the peer 10.1.2.3", seen) + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 9ee7e37..b298302 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -110,7 +110,10 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r := chi.NewRouter() r.Use(middleware.RequestID) - r.Use(middleware.RealIP) + // Not middleware.RealIP: that believes a forwarded header from anyone, which + // lets a caller pick the address the rate limiter and audit log key on. This + // one reads the header only from a trusted proxy — see realip.go. + r.Use(realIP(cfg)) r.Use(requestLogger(log)) r.Use(middleware.Recoverer) r.Use(middleware.Timeout(30 * time.Second)) @@ -685,6 +688,11 @@ func (s *Server) securityHeaders(next http.Handler) http.Handler { // browser ignores is not a control. h.Set("Permissions-Policy", "camera=(), microphone=(), geolocation=(), "+ "payment=(), usb=(), interest-cohort=()") + // Isolate the browsing context: a window this page opened, or one that + // opened it, cannot keep a handle across the origin boundary. verdande + // never scripts another window, so there is nothing to lose and a whole + // class of cross-window attacks to close. + h.Set("Cross-Origin-Opener-Policy", "same-origin") next.ServeHTTP(w, r) }) } From 77994eff09bb9903e44efe8002d9592bb91f8fdf Mon Sep 17 00:00:00 2001 From: kristianwind Date: Sun, 23 Aug 2026 23:34:34 +0200 Subject: [PATCH 3/3] feat(security): a TOTP code can be used once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6238 says the verifier must reject a code it has already accepted: a code stays valid for its whole thirty-second window, so one seen in flight — a phishing page, a proxy, over the shoulder — could be replayed until it expired. Tasks now record the counter of the last accepted step (totp_last_step), and a login accepts a code only when its step is strictly greater. The guard is in the UPDATE, not a read-then-write, so two logins racing with the same stolen code cannot both find the old value and both proceed — exactly one moves the row. auth.TOTPStep is VerifyTOTP that also reports which step matched, walking the same one-step window; the code entered a little early or late spends the step it was issued at, not the step it was checked at, so a late-but-valid code is still single-use. Verified end to end: the same code presented to two logins in one window is accepted once and refused the second time. Co-Authored-By: Claude Opus 5 --- internal/auth/auth_test.go | 33 ++++++++++++++ internal/auth/totp.go | 35 +++++++++++++++ internal/httpapi/auth_handlers.go | 19 +++++++- internal/httpapi/auth_test.go | 44 +++++++++++++++++++ .../store/migrations/0029_totp_single_use.sql | 11 +++++ internal/store/users.go | 20 +++++++++ 6 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 internal/store/migrations/0029_totp_single_use.sql diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index d20dca9..a59f797 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -265,6 +265,39 @@ func TestRecoveryCodes(t *testing.T) { } } +// TOTPStep returns the step a code matched, and it must be the same step across +// the tolerance window — that number is what makes a code single-use, so a code +// entered a little early or late has to spend the same step, not a new one. +func TestTOTPStepIsStableAcrossTheWindow(t *testing.T) { + secret, _, err := NewTOTPSecret("verdande", "k@example.com") + if err != nil { + t.Fatal(err) + } + now := time.Date(2026, 3, 10, 10, 0, 15, 0, time.UTC) + code := codeAt(t, secret, now) + + want, err := TOTPStep(secret, code, now) + if err != nil { + t.Fatalf("the current code did not match: %v", err) + } + // Verified one step late — the code is still valid, and must report the step it + // was issued at, not the step it was checked at. + got, err := TOTPStep(secret, code, now.Add(30*time.Second)) + if err != nil { + t.Fatalf("code rejected one step late: %v", err) + } + if got != want { + t.Errorf("same code reported step %d then %d; a replay check keyed on that would not catch it", want, got) + } + + if _, err := TOTPStep(secret, "000000", now); err == nil { + t.Error("a wrong code returned a step") + } + if _, err := TOTPStep("", code, now); err != ErrNoTOTPSecret { + t.Errorf("empty secret: %v, want ErrNoTOTPSecret", err) + } +} + // codeAt produces the code an authenticator app would display at that moment, // using the same settings VerifyTOTP checks against. func codeAt(t *testing.T, secret string, now time.Time) string { diff --git a/internal/auth/totp.go b/internal/auth/totp.go index d68e4cd..4f30397 100644 --- a/internal/auth/totp.go +++ b/internal/auth/totp.go @@ -2,6 +2,7 @@ package auth import ( "crypto/rand" + "crypto/subtle" "encoding/base32" "errors" "fmt" @@ -82,6 +83,40 @@ func VerifyTOTP(secret, code string, now time.Time) error { return nil } +// TOTPStep verifies a code and returns the counter of the step it matched, so the +// caller can refuse to accept the same step twice. +// +// ValidateCustom answers only yes or no, and "yes" for a code inside the skew +// window is not enough to stop a replay: a code seen in flight stays valid for the +// rest of its period, and the fix is to record which step was spent and never +// accept that step or an earlier one again. That needs the number, so this walks +// the same window ValidateCustom would — one step either side — and reports the +// counter of the match. The comparison is constant time for the same reason the +// password check is: a code is a secret being checked against a computed value. +func TOTPStep(secret, code string, now time.Time) (int64, error) { + if secret == "" { + return 0, ErrNoTOTPSecret + } + code = strings.ReplaceAll(strings.TrimSpace(code), " ", "") + + for offset := -int64(totpSkew); offset <= int64(totpSkew); offset++ { + at := now.Add(time.Duration(offset) * totpPeriod * time.Second) + want, err := totp.GenerateCodeCustom(secret, at, totp.ValidateOpts{ + Period: totpPeriod, + Skew: 0, + Digits: totpDigits, + Algorithm: otp.AlgorithmSHA1, + }) + if err != nil { + return 0, ErrInvalidCode + } + if subtle.ConstantTimeCompare([]byte(want), []byte(code)) == 1 { + return at.Unix() / totpPeriod, nil + } + } + return 0, ErrInvalidCode +} + // NewRecoveryCodes returns single-use codes for someone who has lost their phone, // and the hashes to store. Without these, losing a device means losing the account, // and the only fix left is editing the database by hand. diff --git a/internal/httpapi/auth_handlers.go b/internal/httpapi/auth_handlers.go index 788bb2e..c03a230 100644 --- a/internal/httpapi/auth_handlers.go +++ b/internal/httpapi/auth_handlers.go @@ -218,7 +218,24 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) { return } - err := auth.VerifyTOTP(user.TOTPSecret, req.Code, time.Now()) + step, err := auth.TOTPStep(user.TOTPSecret, req.Code, time.Now()) + if err == nil { + // The code is right; now it must also be unused. A TOTP code stays valid for + // its whole window, so one observed in flight — a phishing page, a proxy, + // over the shoulder — could otherwise be replayed until it expires. Spending + // the step here makes it single-use: a second presentation of the same code, + // or an earlier one, finds the step already gone. + fresh, cerr := s.db.ConsumeTOTPStep(r.Context(), user.ID, step) + if cerr != nil { + s.internal(w, r, "consume totp step", cerr) + return + } + if !fresh { + writeError(w, http.StatusUnauthorized, CodeUnauthorized, + "that verification code has already been used") + return + } + } if err != nil { // A recovery code is accepted in the same field. Making somebody find a // different form while locked out of their account is a poor time to diff --git a/internal/httpapi/auth_test.go b/internal/httpapi/auth_test.go index 779c245..110dfbe 100644 --- a/internal/httpapi/auth_test.go +++ b/internal/httpapi/auth_test.go @@ -372,6 +372,50 @@ func TestTOTPEnrolmentAndTwoStepLogin(t *testing.T) { } } +// A TOTP code works once. A code seen in flight and replayed inside its window +// must be refused the second time, or the second factor is only as good as the +// thirty seconds an attacker has to reuse what they captured. +func TestTOTPCodeIsSingleUse(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + resp, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "et langt kodeord", + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("totp setup: %d %v", resp.StatusCode, body) + } + secret, _ := body["secret"].(string) + resp, body = ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{ + "code": codeFor(t, secret), + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("totp confirm: %d %v", resp.StatusCode, body) + } + + // One code, captured once and presented twice. + code := codeFor(t, secret) + + ts.do(t, "POST", "/api/v1/auth/logout", nil) + ts.do(t, "POST", "/api/v1/auth/login", map[string]string{ + "email": "kristian@example.dk", "password": "et langt kodeord", + }) + resp, body = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": code}) + if resp.StatusCode != http.StatusOK { + t.Fatalf("first use of the code failed: %d %v", resp.StatusCode, body) + } + + // A second login, same code, same window. + ts.do(t, "POST", "/api/v1/auth/logout", nil) + ts.do(t, "POST", "/api/v1/auth/login", map[string]string{ + "email": "kristian@example.dk", "password": "et langt kodeord", + }) + resp, body = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": code}) + if resp.StatusCode != http.StatusUnauthorized { + t.Fatalf("the same code was accepted twice: %d %v", resp.StatusCode, body) + } +} + // A recovery code is accepted in the same field as a TOTP code, and works once. func TestRecoveryCodeCompletesLoginOnce(t *testing.T) { ts := newTestServer(t) diff --git a/internal/store/migrations/0029_totp_single_use.sql b/internal/store/migrations/0029_totp_single_use.sql new file mode 100644 index 0000000..c446408 --- /dev/null +++ b/internal/store/migrations/0029_totp_single_use.sql @@ -0,0 +1,11 @@ +-- A TOTP code is meant to be usable once. RFC 6238 says so in as many words: the +-- verifier must reject a code that has already been accepted, or a code observed in +-- flight — over the shoulder, off a phishing page, out of a proxy — can be replayed +-- for the rest of its thirty-second window. +-- +-- totp_last_step records the counter of the most recently accepted code (Unix time +-- divided by the period). A login accepts a code only when its step is strictly +-- greater, so the same code cannot be spent twice and time cannot run backwards. +-- Null means no code has been accepted yet, which is every account until its owner +-- next signs in with one. +ALTER TABLE users ADD COLUMN totp_last_step INTEGER; diff --git a/internal/store/users.go b/internal/store/users.go index 6558cb4..67f6468 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -466,6 +466,26 @@ func (db *DB) SetTOTPSecret(ctx context.Context, userID, secret string, enabled return err } +// ConsumeTOTPStep records a just-accepted TOTP step and reports whether it was +// fresh. It answers false when the step has already been spent — the same code, or +// an earlier one, being presented a second time. +// +// The guard is in the statement, not in a read-then-write: two logins racing with +// the same stolen code must not both find the old value and both proceed. The row +// moves forward only when the new step is strictly greater, so exactly one of them +// updates a row and the other sees nothing change. +func (db *DB) ConsumeTOTPStep(ctx context.Context, userID string, step int64) (bool, error) { + res, err := db.ExecContext(ctx, + `UPDATE users SET totp_last_step = ? WHERE id = ? + AND (totp_last_step IS NULL OR totp_last_step < ?)`, + step, userID, step) + if err != nil { + return false, err + } + n, _ := res.RowsAffected() + return n == 1, nil +} + // InboxID returns the user's Inbox project. func (db *DB) InboxID(ctx context.Context, userID string) (string, error) { var id string