From 03a9db03dcc923cb8b3c4aaf2d91d45ad72b46dd Mon Sep 17 00:00:00 2001 From: Claus Lohmar Date: Wed, 14 Oct 2026 11:29:52 +0000 Subject: [PATCH] =?UTF-8?q?fix(security):=20harden=20httpx=20=E2=80=94=20X?= =?UTF-8?q?FF=20hops,=20=5F=5FHost-=20cookie,=20isolation=20headers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW-001 (M0) perimeter findings: - H1: resolveForwarded walked X-Forwarded-For left→right and returned the leftmost untrusted entry. An append-style proxy (nginx's default) appends the peer it saw to the RIGHT of whatever the client sent, so the leftmost entry is attacker-controlled — a fresh rate-limit bucket per request (A3) and a forged client_ip in the audit trail (A11/D-031). Walk right→left and return the first untrusted entry, falling back to the peer. - L1s: the session cookie now carries the `__Host-` prefix when it is Secure (https), so it is pinned to the exact origin and cannot be shadowed from a sibling subdomain. Conditional on purpose: a browser rejects a `__Host-` cookie without Secure, which would break the http loopback bootstrap. - L4s: add Permissions-Policy, Cross-Origin-Opener-Policy and Cross-Origin-Resource-Policy. - Add httpx.IsLoopbackIP: a RESOLVED client IP test that fails closed on an empty/unparseable value (the setup-mode gate, H2), sharing a classifier with IsLoopbackHost whose "unknown" direction is the opposite, each fail-closed for its own decision. Tests updated: the unit test encoding the vulnerable leftmost expectation is rewritten, plus right→left/forged-entry/host-classification coverage. --- internal/core/httpx/clientip.go | 37 +++++++----- internal/core/httpx/cookie.go | 21 ++++++- internal/core/httpx/headers.go | 11 ++++ internal/core/httpx/httpx_test.go | 98 +++++++++++++++++++++++++++++-- internal/core/httpx/loopback.go | 44 +++++++++++--- 5 files changed, 181 insertions(+), 30 deletions(-) diff --git a/internal/core/httpx/clientip.go b/internal/core/httpx/clientip.go index 6b3fe80..e3290da 100644 --- a/internal/core/httpx/clientip.go +++ b/internal/core/httpx/clientip.go @@ -64,9 +64,11 @@ func (t TrustedProxies) Contains(ip net.IP) bool { // // - the immediate peer (RemoteAddr) is NOT trusted → any X-Forwarded-For is // ignored and the peer is the client (a client cannot spoof past the gate); -// - the peer IS trusted → walk X-Forwarded-For left to right and return the -// first address that is not itself a trusted proxy (the real client); if -// every hop is trusted, the last hop is returned. +// - the peer IS trusted → walk X-Forwarded-For RIGHT to LEFT and return the +// first address that is not itself a trusted proxy (the real client). Each +// trusted hop appended the peer it saw to the right, so the rightmost +// untrusted entry is the one closest to us that a client could not have +// forged (H1); if every hop is trusted, the peer is returned. // // The resolved value is written back with Header.Set (replace), never Add. func ClientIP(r *http.Request, trusted TrustedProxies) string { @@ -85,26 +87,29 @@ func ClientIP(r *http.Request, trusted TrustedProxies) string { return s } -// resolveForwarded returns the leftmost non-trusted address in the header, -// falling back to the last hop, then to the peer. +// resolveForwarded returns the RIGHTMOST address in the header that is not a +// trusted proxy, falling back to the peer. +// +// Walking right→left is the defence (H1): an append-style proxy (nginx's +// default) appends the peer it saw to the RIGHT of whatever the client sent, so +// the leftmost entries are attacker-controlled. The rightmost untrusted entry +// is the closest hop we can trust — it is the first address not appended by a +// trusted proxy. Returning the leftmost entry (the old behaviour) let a client +// forge a fresh rate-limit bucket per request (A3) and a forged client_ip in +// the audit trail (A11/D-031). func resolveForwarded(header string, trusted TrustedProxies, peer net.IP) net.IP { - if strings.TrimSpace(header) == "" { - return peer - } - var last net.IP - for _, part := range strings.Split(header, ",") { - ip := net.ParseIP(strings.TrimSpace(part)) + parts := strings.Split(header, ",") + for i := len(parts) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(parts[i])) if ip == nil { continue } - last = ip if !trusted.Contains(ip) { - return ip // the real client + return ip // the closest untrusted hop — the real client } } - if last != nil { - return last - } + // Every hop (or the whole header) is trusted or unparseable: the peer is + // the only address we can vouch for. return peer } diff --git a/internal/core/httpx/cookie.go b/internal/core/httpx/cookie.go index 8e64a3d..04bb26e 100644 --- a/internal/core/httpx/cookie.go +++ b/internal/core/httpx/cookie.go @@ -8,10 +8,27 @@ package httpx import "net/http" -// SessionCookieName is the name of the session cookie (SPEC-003 §6). The value -// is an opaque random id; the server-side session row is authoritative. +// SessionCookieName is the base name of the session cookie (SPEC-003 §6). The +// value is an opaque random id; the server-side session row is authoritative. const SessionCookieName = "myday_session" +// HostPrefix is the `__Host-` cookie-name prefix (L1s). A browser only accepts +// a `__Host-` cookie when it is Secure · Path=/ · has no Domain attribute, so +// the cookie is pinned to the exact origin — a sibling subdomain cannot shadow +// it (a cookie-fixation / session-shadowing DoS). +const HostPrefix = "__Host-" + +// SessionCookieNameFor returns the session cookie's name for the transport: +// `__Host-myday_session` over https (Secure is set — required by the prefix) +// and the bare name otherwise, because a browser REJECTS a `__Host-` cookie +// sent without Secure over plain http (the loopback bootstrap / human test). +func SessionCookieNameFor(secure bool) string { + if secure { + return HostPrefix + SessionCookieName + } + return SessionCookieName +} + // SetSessionCookie writes the hardened session cookie (SPEC-003 §6): // HttpOnly · Secure · SameSite=Lax · Path=/. The lifetime is server-side, so // no Max-Age is set (a browser-session cookie). diff --git a/internal/core/httpx/headers.go b/internal/core/httpx/headers.go index 70c5f72..1ea4718 100644 --- a/internal/core/httpx/headers.go +++ b/internal/core/httpx/headers.go @@ -22,7 +22,18 @@ func SetSecurityHeaders(w http.ResponseWriter, secure bool) { h.Set("Referrer-Policy", "no-referrer") h.Set("X-Frame-Options", "DENY") h.Set("Content-Security-Policy", ContentSecurityPolicy) + // L4s — the isolation trio: deny the powerful features the app never uses, + // and keep the document and its subresources same-origin (so a compromised + // or mis-hosted sibling cannot open, embed, or read MyDay). + h.Set("Permissions-Policy", PermissionsPolicy) + h.Set("Cross-Origin-Opener-Policy", "same-origin") + h.Set("Cross-Origin-Resource-Policy", "same-origin") if secure { h.Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") } } + +// PermissionsPolicy disables the browser features MyDay does not use (L4s). +// An empty allowlist means "no origin, including self, may use it". +const PermissionsPolicy = "accelerometer=(), camera=(), geolocation=(), " + + "gyroscope=(), magnetometer=(), microphone=(), payment=(), usb=()" diff --git a/internal/core/httpx/httpx_test.go b/internal/core/httpx/httpx_test.go index a6bc992..c31d1c8 100644 --- a/internal/core/httpx/httpx_test.go +++ b/internal/core/httpx/httpx_test.go @@ -80,29 +80,107 @@ func TestClientIPUntrustedPeerIgnoresXFF(t *testing.T) { } } -func TestClientIPTrustedPeerResolvesLeftmostUntrusted(t *testing.T) { +func TestClientIPTrustedPeerResolvesRightmostUntrusted(t *testing.T) { tp, _ := httpx.ParseTrustedProxies([]string{"127.0.0.1", "10.0.0.0/8"}) r := httptest.NewRequest("GET", "/", nil) r.RemoteAddr = "127.0.0.1:5555" - r.Header.Set("X-Forwarded-For", "198.51.100.7, 10.0.0.5") + // An append-style proxy (nginx default) appends the peer it SAW to the + // right of whatever the client sent. Here a client forged "1.2.3.4" and the + // proxy appended the real client "198.51.100.7" (H1). + r.Header.Set("X-Forwarded-For", "1.2.3.4, 198.51.100.7, 10.0.0.5") got := httpx.ClientIP(r, tp) if got != "198.51.100.7" { - t.Errorf("ClientIP = %q, want the leftmost non-trusted 198.51.100.7", got) + t.Errorf("ClientIP = %q, want the rightmost untrusted 198.51.100.7 (H1: the leftmost entry is forgeable)", got) } if xff := r.Header.Get("X-Forwarded-For"); xff != "198.51.100.7" { t.Errorf("X-Forwarded-For = %q, want it SET (replaced), not appended", xff) } } +// TestClientIPIgnoresForgedLeftmostEntry is the H1 regression: the leftmost +// entry must never win, or an append-style proxy gives the client a fresh +// rate-limit bucket and a forged audit client_ip (A3/A11). +func TestClientIPIgnoresForgedLeftmostEntry(t *testing.T) { + tp, _ := httpx.ParseTrustedProxies([]string{"127.0.0.1"}) + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = "127.0.0.1:5555" + r.Header.Set("X-Forwarded-For", "9.9.9.9, 198.51.100.7") + + if got := httpx.ClientIP(r, tp); got != "198.51.100.7" { + t.Errorf("ClientIP = %q, want 198.51.100.7 (the forged 9.9.9.9 must be ignored, H1)", got) + } +} + func TestClientIPAllHopsTrusted(t *testing.T) { tp, _ := httpx.ParseTrustedProxies([]string{"127.0.0.1", "10.0.0.0/8"}) r := httptest.NewRequest("GET", "/", nil) r.RemoteAddr = "127.0.0.1:5555" r.Header.Set("X-Forwarded-For", "10.0.0.1, 10.0.0.2") - if got := httpx.ClientIP(r, tp); got != "10.0.0.2" { - t.Errorf("ClientIP = %q, want the last hop 10.0.0.2", got) + // Every hop is trusted: the peer is the only address we can vouch for. + if got := httpx.ClientIP(r, tp); got != "127.0.0.1" { + t.Errorf("ClientIP = %q, want the peer 127.0.0.1 when every hop is trusted", got) + } +} + +// TestIsLoopbackHost pins the nudge's origin predicate (SPEC-006 §8): a NAME +// is not loopback, and only an empty/unknown host fails closed to loopback. +func TestIsLoopbackHost(t *testing.T) { + cases := []struct { + host string + want bool + }{ + {"127.0.0.1", true}, + {"127.0.0.1:8080", true}, + {"localhost", true}, + {"localhost:8080", true}, + {"[::1]:8080", true}, + {"example.com", false}, + {"myday.example.com:443", false}, + {"203.0.113.9", false}, + {"", true}, // unknown → suppress the nudge (fail closed) + } + for _, tc := range cases { + if got := httpx.IsLoopbackHost(tc.host); got != tc.want { + t.Errorf("IsLoopbackHost(%q) = %v, want %v", tc.host, got, tc.want) + } + } +} + +// TestIsLoopbackIPFailsClosed pins the setup-mode gate's predicate (H2): an +// empty or unparseable resolved client IP must NOT count as loopback. +func TestIsLoopbackIPFailsClosed(t *testing.T) { + cases := []struct { + ip string + want bool + }{ + {"127.0.0.1", true}, + {"127.0.0.5", true}, + {"::1", true}, + {"localhost", true}, + {"[::1]", true}, + {"203.0.113.9", false}, + {"10.0.0.5", false}, + {"", false}, // unparseable: fail closed + {"not-an-ip", false}, + {"0.0.0.0", false}, + } + for _, tc := range cases { + if got := httpx.IsLoopbackIP(tc.ip); got != tc.want { + t.Errorf("IsLoopbackIP(%q) = %v, want %v", tc.ip, got, tc.want) + } + } +} + +// TestSessionCookieNameForPrefix pins L1s: the `__Host-` prefix is used exactly +// when the cookie is Secure (a browser rejects a `__Host-` cookie over http). +func TestSessionCookieNameForPrefix(t *testing.T) { + if got := httpx.SessionCookieNameFor(true); got != "__Host-myday_session" { + t.Errorf("SessionCookieNameFor(true) = %q, want the __Host- prefix", got) + } + if got := httpx.SessionCookieNameFor(false); got != httpx.SessionCookieName { + t.Errorf("SessionCookieNameFor(false) = %q, want the bare name", got) } } @@ -155,6 +233,16 @@ func TestSetSecurityHeaders(t *testing.T) { if !strings.Contains(h.Get("Content-Security-Policy"), "default-src 'self'") { t.Errorf("CSP = %q", h.Get("Content-Security-Policy")) } + // L4s: the isolation trio. + if got := h.Get("Permissions-Policy"); !strings.Contains(got, "geolocation=()") { + t.Errorf("Permissions-Policy = %q, want the feature allowlist", got) + } + if got := h.Get("Cross-Origin-Opener-Policy"); got != "same-origin" { + t.Errorf("Cross-Origin-Opener-Policy = %q, want same-origin", got) + } + if got := h.Get("Cross-Origin-Resource-Policy"); got != "same-origin" { + t.Errorf("Cross-Origin-Resource-Policy = %q, want same-origin", got) + } if h.Get("Strict-Transport-Security") != "" { t.Error("HSTS set for an insecure request") } diff --git a/internal/core/httpx/loopback.go b/internal/core/httpx/loopback.go index 97f5f98..c794ae4 100644 --- a/internal/core/httpx/loopback.go +++ b/internal/core/httpx/loopback.go @@ -11,20 +11,50 @@ import ( // It gates the passkey nudge (SPEC-006 §8): a passkey registered at // `127.0.0.1` binds the Relying Party ID `127.0.0.1` and is UNUSABLE at the // public domain (O-13/D-041), so the nudge is suppressed on loopback. An -// empty/unknown host is treated as loopback — fail closed, so an origin we -// cannot classify never triggers the nudge. +// empty/unknown host counts as loopback — fail closed, so an origin we cannot +// classify never triggers the nudge. func IsLoopbackHost(hostport string) bool { + loopback, known := classifyHost(hostport) + return !known || loopback +} + +// IsLoopbackIP reports whether a RESOLVED client IP is loopback. It gates the +// per-request setup-mode check (SPEC-003 §7, H2) and therefore FAILS CLOSED in +// the other direction: an empty or unparseable value is NOT loopback. +// +// A bind-time check alone is not enough (D-025): a reverse proxy on 127.0.0.1 +// relaying remote clients makes `/setup` reachable past the bind check, so the +// door must re-check every request. +func IsLoopbackIP(ip string) bool { + loopback, known := classifyHost(ip) + return known && loopback +} + +// classifyHost reports whether hostport ("host", "host:port", or a bare IP) is +// loopback, and whether it could be classified at all. The two callers above +// resolve "unknown" in opposite directions on purpose: each is fail-closed for +// its own decision. +// +// "Unknown" is only an empty/blank value: a NAME that is not `localhost` is +// known — and it is not loopback. +func classifyHost(hostport string) (loopback, known bool) { host := strings.TrimSpace(hostport) + if host == "" { + return false, false + } if h, _, err := net.SplitHostPort(host); err == nil { host = h } host = strings.Trim(strings.TrimSpace(host), "[]") if host == "" { - return true + return false, false } - if strings.EqualFold(host, "localhost") { - return true + if strings.EqualFold(host, "localhost") || strings.EqualFold(host, "localhost.") { + return true, true } - ip := net.ParseIP(host) - return ip != nil && ip.IsLoopback() + if ip := net.ParseIP(host); ip != nil { + return ip.IsLoopback(), true + } + // A name we cannot map to loopback: known, and not loopback. + return false, true }