Repository navigation
Security fixes - #256
Merged
Merged
Security fixes#256
Conversation
pathMatches accepted a prefix match on either the raw or the cleaned request path. Since exceptions use the same function, a request like /public/../admin matched an exception for /public/ while the backend received /admin, bypassing SSO. Only match the cleaned path. Also add SECURITY-TODO.md to track the security review findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ValidateToken accepted a key from any trusted issuer and only checked that the iss claim matched the local issuer. A trusted issuer, or anyone able to tamper with its JWKS, could therefore sign Bearer tokens that were accepted as locally issued, for any user. ValidateToken now only uses local keys. Tokens from trusted issuers are validated with the new ValidateRemoteToken, which requires the signing key to belong to the issuer in the iss claim. Bearer tokens and ID token cookies now require an exp claim, and trusted issuer JWKS URIs must use https. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ttl form value was not checked for negative values or overflow. A negative ttl passed the min() cap and produced a negative ValidBefore that wrapped to a huge uint64, i.e. a certificate that never expires, which OpenSSH accepts. A very large ttl overflowed with the same result. Reject ttl <= 0 and cap ttl at the maximum lifetime before converting it to a time.Duration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QUICConn.HandshakeComplete and NextConnection called themselves instead of the underlying connection. The http3 client calls HandshakeComplete on every request, so any request forwarded to a backend over HTTP/3 crashed the whole process with a stack overflow. Add a test with an HTTP/3 backend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reverse proxy only verified that the Host header matched one of the backend's server names after authentication and local endpoints were handled, and LOCAL/CONSOLE backends never checked it. The Bearer token audience, the ForceReAuth host hash, and URL token URLs are all derived from the Host header, so a client could e.g. replay an ID token issued for one backend against another backend by sending that backend's name in the Host header. Check the Host header first in both handlers and return 421 Misdirected Request when it doesn't match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ocsp.ParseResponseForCert accepts a response signed by any embedded certificate that was issued by the CA, without checking that it has the OCSPSigning extended key usage. Any certificate from the same CA, including the revoked certificate itself, could sign a "Good" stapled response, which was then cached. Fetched responses were parsed with ocsp.ParseResponse, which doesn't check the serial number, and neither path checked ThisUpdate/NextUpdate. Validate both stapled and fetched responses with a common function that checks the serial number, the responder's authorization, and the response's validity period. A stapled response only replaces a cached response if it is newer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Any user with access to the PKI endpoint could request a server certificate for any DNS name, including wildcards and the names of other backends. That is a problem wherever the CA is trusted for server authentication, e.g. in forwardRootCAs. Add a serverCertificates option to the PKI config. Each entry lists the DNS names (or one-label wildcards) that can be requested, and optionally which users or groups can request them. Without this option, only client certificates can be requested. This is a breaking change for configs that rely on users requesting server certificates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The HTTP server that serves ACME challenges and HTTPS redirects had no timeouts and no connection limit. Idle connections were held forever and could exhaust the process's file descriptors, taking down the whole proxy. Use 10s read and write timeouts, and limit the number of concurrent connections. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every request to the local OIDC provider scanned all pending requests under the server's lock, and unauthenticated device authorization requests could add entries without limit, making the cost quadratic. Scan for expired requests at most every 30 seconds, check expiration when looking up a request, and cap the number of pending authorization and device authorization requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
serveLogin and serveLogout redirected to the URL in a URL token without checking its host. URL tokens were built from the Host header, so a client could create a token for an arbitrary host and get a victim to use it, resulting in an open redirect after login. URL tokens also never expired. Require the URL token's host to be one of the backend's server names on login and logout, and make URL tokens expire after 24 hours. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After setting the ID token cookie, the proxy redirected to req.URL.String(), which is a relative URL. For a request like GET //evil.com/x, that produced Location: //evil.com/x, an open redirect. Redirect to an absolute URL with the request's (validated) host. Do the same in the passkeys manager for consistency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SAML assertion consumer service only checked InResponseTo against server-side state. An attacker could start a login, sign in as themselves, and get a victim's browser to post the resulting SAML response, logging the victim into the attacker's account. Set a short-lived cookie with the request ID when sending the AuthnRequest, and require it to match InResponseTo in the callback. The cookie is SameSite=None because the SAML response is a cross-site POST. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ECH accepted/rejected events included the server name from the ClientHello and were recorded before the backend lookup. Any client could send an ECH extension with an arbitrary outer SNI, and each distinct name added a permanent entry to the events map. Record these events only after the server name matched a backend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only the exact names of proxy-owned headers were removed from client requests. Backends that map header names to CGI-style variables treat X_tlsproxy_user_id the same as X-tlsproxy-user-id, so a client could spoof the user identity, or any header set with forwardHttpHeaders. Client-supplied X-Forwarded-Host, X-Forwarded-Proto and Forwarded headers were also forwarded as is. Remove all case and underscore variants of the identity header, the forwardHttpHeaders keys, X-Forwarded-For and X-Forwarded-Client-Cert. Remove client-supplied Forwarded, X-Forwarded-Host and X-Forwarded-Proto, and set X-Forwarded-Host and X-Forwarded-Proto. They can still be overridden with forwardHttpHeaders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
enforceSSOPolicy returned early when no SSO rule matched the request path, without checking the local handler's scopes. A token with limited scope, e.g. an OIDC access token with only the openid scope, could then use PKI or SSH CA endpoints when the backend's SSO rules didn't cover their paths. When no rule matches, still check the handler's scopes for authenticated requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The OIDC client, SAML provider and passkeys manager kept pending login requests in maps that were only pruned when a callback was handled, and each callback scanned the whole map. Unauthenticated clients could add entries, including an arbitrarily long return URL, without limit. Add idp.PendingLogins, which checks expiration on lookup, prunes expired entries periodically when new ones are added, and limits the number of entries. Use it for the OIDC client and SAML states, and for passkey nonces and challenges. Also reject return URLs longer than 4096 bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Passkey registration checked the origin in the client data, but login assertions didn't. A page on another origin that can use the same RP ID, e.g. a subdomain, could get an assertion from the user's authenticator and use it to log in as the user. Require the origin to be the passkey endpoint's origin. Also make the fake authenticator use its configured origin for assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The local OIDC server checked the client ACL and filtered scopes for the user who made the authorization request, but minted the tokens for whichever user approved the request ID. A user denied by the client's ACL could get tokens by approving another user's request. Record the requesting user with the pending request, and reject approval by any other user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The device verification page only showed the user code, so users couldn't tell which client was requesting access, and to what. Neither the device verification page nor the consent page prevented framing. Make device verification two steps: after the user code is entered, the page shows the requesting client and scopes before the user can approve. Send X-Frame-Options and frame-ancestors headers on both pages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For emails without passkeys, AssertionOptions returned a fake 1-byte credential ID, which made it easy to tell which emails are registered. Use a 32-byte ID derived from an HMAC of the email instead, so it looks like a real ID and is the same on every request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The /.sso/ status page is added to every backend, but a GET request on a backend without SSO dereferenced be.SSO and panicked. The panic was recovered, but each request logged a full stack trace. Return 404 when SSO isn't enabled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cached CRL was reused until 30 minutes after its NextUpdate, and the check for newer revocations compared timestamps with one-second resolution, so a revocation in the same second as the CRL's creation was missed. maybeRotateDelegateCert also read m.db without holding m.mu, while the unauthenticated OCSP and CRL handlers could replace it. Count revocations and regenerate the CRL when the count changed, or when the CRL is halfway to its NextUpdate. Check whether the delegate cert needs rotation while holding the lock, and check again when signing the new cert, so concurrent requests don't rotate it twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CSRF check was skipped for any request with an Authorization header, even when the header wasn't a valid bearer token and the user was authenticated with cookies. Browsers send cached Basic credentials automatically, including on cross-site requests. Skip the check only when the request was authenticated with a valid bearer token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The token endpoint only checked that the redirect_uri was registered for the client, not that it was the one used in the authorization request (RFC 6749 section 4.1.3). PKCE parameters were ignored. Require the redirect_uri to match the authorization request. When the client sends a code challenge, require a matching code verifier (RFC 7636), and advertise the supported methods in the discovery document. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The signature counter in passkey assertions was ignored. Store it with each key, and reject assertions whose counter didn't increase, which can indicate a cloned authenticator. Authenticators that don't implement the counter always send 0, and are not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revoking a certificate issued by the built-in PKI only affected new handshakes. Connections that were already open with that certificate stayed open. Re-authorize open connections after a certificate is revoked, and check revocation in addition to the ACL when re-authorizing. Also fix a nil pointer dereference in reAuthorize when a connection's backend no longer exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DSA keys are deprecated and no longer supported by OpenSSH. Stop issuing certificates for them, and return 400 instead of 500 for unsupported key types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ServeCertificate held the CA's lock for the whole request, including while reading the request body. A slow upload from one user blocked certificate issuance for everyone. Only hold the lock while signing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tlsclient -ocsp used the second certificate sent by the server as the issuer, instead of the issuer from the verified chain, and accepted responses signed by any certificate issued by the CA. Use the verified chain, require the responder to be the issuer or to have the OCSPSigning extended key usage, and reject responses that aren't valid yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Command line arguments are visible to all users on the host. Log a warning when --passphrase is used, and recommend $TLSPROXY_PASSPHRASE in the README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SNI and ALPN values from the ClientHello were logged with %s, so a client could inject newlines and other control characters into the logs. Use %q. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Reduce the TLS handshake timeout from 2 minutes to 30 seconds. - Fail instead of queueing forever when the forward rate limiter would delay a connection or request by more than 30 seconds. - Apply the forward rate limit to every QUIC stream. Previously, each stream of a non-h3 QUIC connection could make a request, or dial the backend, without waiting for the limiter. - Add an optional maxOpenPerIP setting to limit the number of open connections from a single client IP address. There is no limit by default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
handleConnection read p.echKeys without holding p.mu, while rotateECH replaces it. Read it, and build the ACME TLS config, while holding the read lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HandleCallback assigned the token endpoint request to its own req parameter, so SetAuthTokenCookie and http.Redirect got the outbound request instead of the user's. Its context doesn't have the user's current claims, so the th session-chain claim was never carried over to the new auth token. Use separate variables for the outbound requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The config shown on the metrics page redacted some secrets, but not static forwardHttpHeaders values, e.g. Authorization headers for the backends, or ECH webhook URLs, which often contain tokens. Redact forwardHttpHeaders values that don't use variables, and keep only the scheme and host of webhook URLs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fetch wrapper added the session ID as x-csrf-token to every request, including requests to other origins. Only add it to same-origin requests. Also set it correctly when the request headers are a Headers object. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An ACL entry was compared directly with the user's email even when it was the name of a group. An identity provider that asserted an email equal to a group name, e.g. "admins", would match every ACL that includes that group. Skip the direct comparison for group names, and never match an empty email. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ephemeral certificate manager, used for testing, generated a new RSA key for every server name and cached the certificates forever, even after they expired. Reuse one key for all the certificates, limit the cache size, and reissue expired certificates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Set-Cookie headers from backends were passed through as is, so a backend could set the proxy's own cookies, e.g. TLSPROXYNONCE or TLSPROXYSAMLNONCE for the whole SSO domain, and log users into another account. Drop Set-Cookie headers for the proxy's cookie names from backend responses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All the items are resolved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
baseTLSConfig reads p.echKeys and requires p.mu, but the backend's tlsConfig function called it without the lock for every HTTP, TLS, and QUIC connection. Take the lock in tlsConfig, and don't hold it when calling tlsConfig from the QUIC GetConfigForClient callback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Non-h3 QUIC connections to non-QUIC backends wait for the forward rate limiter for each stream, but they also waited once for the connection, which doesn't dial or forward anything itself. The first stream cost two tokens. Only wait for the connection with h3 and QUIC backends, where streams aren't rate limited individually. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Previously, the X-Forwarded-For and X-Forwarded-Client-Cert headers were being unconditionally deleted in reverseProxyDirector, which executed after the custom ForwardHTTPHeaders had already been applied. This caused any user-defined overrides for these headers to be ignored. This commit moves the deletion of these headers to reverseProxy, ensuring they are removed before applying any custom overrides.
TestGetCertCache generated 1001 RSA keys to fill the cache, which took over a minute. Make maxCachedCerts a field of CertManager so the test can use a small cache, and remove a stale comment about key reuse. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Scan the pending logins map at most once per second when it is full, so a flood of login requests doesn't scan it on every request. - Remove a dead Header.Del in the forwardHttpHeaders loop. The variants of the header were already removed. - Fix the release note about the ephemeral certificate manager, which generates a new key for every name again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a collection of security fixes resulting from the latest security review. Most of the changes were made directly by claude.
Type of change
How is this change tested ?