diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml index 0098a5e23..901656a06 100644 --- a/.github/codeql/codeql-config.yml +++ b/.github/codeql/codeql-config.yml @@ -3,9 +3,9 @@ name: cortex CodeQL config queries: - uses: security-extended -# Path-scoped query suppressions. Each entry must explain WHY the -# query doesn't apply — future readers should be able to evaluate -# whether the suppression is still warranted as code evolves. +# Query suppressions. Each entry must explain WHY the query doesn't +# apply — future readers should be able to evaluate whether the +# suppression is still warranted as code evolves. query-filters: # go/disabled-certificate-check fires on InsecureSkipVerify: true # in core/tlsconfig/client.go. The flag is required: it lets crypto/tls diff --git a/core/listener/extproc/server.go b/core/listener/extproc/server.go index d4fa680d7..741cf9836 100644 --- a/core/listener/extproc/server.go +++ b/core/listener/extproc/server.go @@ -637,7 +637,7 @@ func (s *Server) handleOutboundBody(stream extprocv3.ExternalProcessor_ProcessSe // run the pipeline on the body phase. if pat, matched := s.SkipHosts.MatchPattern(pctx.Host); matched { slog.Info("ext_proc: skip_hosts match (body phase) — bypassing pipeline + session recording", - "host", pctx.Host, "pattern", pat, "path", pctx.Path) + "host", pipeline.SanitizeLogValue(pctx.Host), "pattern", pat, "path", pipeline.SanitizeLogValue(pctx.Path)) return allowBodyResponse(), nil } diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index 2380ac90a..e1d6392bf 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -513,9 +513,9 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // and a control rune forged into any of them would forge a log entry — the same // CWE-150 shape #1539's egress guard was flagged for. slog.Info("forward-proxy: destination refused by egress allow list", - "host", pipeline.SanitizeControlRunes(host), - "method", pipeline.SanitizeControlRunes(r.Method), - "path", pipeline.SanitizeControlRunes(r.URL.Path)) + "host", pipeline.SanitizeLogValue(host), + "method", pipeline.SanitizeLogValue(r.Method), + "path", pipeline.SanitizeLogValue(r.URL.Path)) httpx.WriteRejection(w, action) return } @@ -542,7 +542,7 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne pat, skipped := s.SkipHosts.MatchPattern(pctx.Host) if skipped { slog.Info("forward-proxy: skip_hosts match — bypassing pipeline + session recording", - "host", pctx.Host, "pattern", pat, "method", r.Method, "path", r.URL.Path) + "host", pipeline.SanitizeLogValue(pctx.Host), "pattern", pat, "method", r.Method, "path", pipeline.SanitizeLogValue(r.URL.Path)) } // The client's process, looked up once per connection; nil when process attribution @@ -581,7 +581,7 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne } r.Body = io.NopCloser(bytes.NewReader(body)) pctx.Body = body - slog.Debug("forward-proxy: buffered request body", "host", r.Host, "bodyLen", len(body)) + slog.Debug("forward-proxy: buffered request body", "host", pipeline.SanitizeLogValue(r.Host), "bodyLen", len(body)) } // Establish the session identity ONCE, here, and keep it in a local: the @@ -745,8 +745,8 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // Sanitized like the gate at the top of this function: both hosts are // client-controlled (pctx.Host is the one the client named, pre-redirect). slog.Info("forward-proxy: destination refused by egress allow list (redirect re-check)", - "host", pipeline.SanitizeControlRunes(host), - "requested", pipeline.SanitizeControlRunes(pctx.Host)) + "host", pipeline.SanitizeLogValue(host), + "requested", pipeline.SanitizeLogValue(pctx.Host)) httpx.WriteRejection(w, action) return } @@ -789,7 +789,7 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne slog.Debug("forward-proxy: destination refused by egress guard", // Sanitized: Host and Method are client-controlled, and a newline // forged into either would forge a log entry (#1539's CodeQL finding). - "host", pipeline.SanitizeControlRunes(r.Host), + "host", pipeline.SanitizeLogValue(r.Host), "method", pipeline.SanitizeControlRunes(r.Method), "addr", ref.Addr.String()) // The listener's standard rejection shape, code verbatim from the MOCA // egress contract so the shared conformance suite matches. @@ -812,10 +812,10 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // See pipeline.ExchangeFailure. status, fail := pipeline.ExchangeFailure(r.Context(), err) if r.Context().Err() != nil { - slog.Debug("forward-proxy: client went away before the upstream answered", "host", r.Host, "method", r.Method) + slog.Debug("forward-proxy: client went away before the upstream answered", "host", pipeline.SanitizeLogValue(r.Host), "method", r.Method) } else { // fail.Message, not err: err's text quotes the request URL, query and all. - slog.Warn("forward-proxy: upstream request failed", "host", r.Host, "method", r.Method, "kind", fail.Kind, "error", fail.Message) + slog.Warn("forward-proxy: upstream request failed", "host", pipeline.SanitizeLogValue(r.Host), "method", r.Method, "kind", fail.Kind, "error", fail.Message) } http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) // Without this the request event is the only trace, and a row with no @@ -905,7 +905,7 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // supported configuration (cpex, sparc), not a misconfiguration, so // warning every request is log spam at request rate. s.bufferedFallbackOnce.Do(func() { - slog.Info("forward-proxy: text/event-stream responses will use the buffered path — a WritesResponseBody plugin is in the chain", "host", r.Host) + slog.Info("forward-proxy: text/event-stream responses will use the buffered path — a WritesResponseBody plugin is in the chain", "host", pipeline.SanitizeLogValue(r.Host)) }) } else if pinned.HasStreamingResponders() { // Streaming-aware plugins (inference-parser, a2a-parser) read each @@ -933,7 +933,7 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // so ask about the response side specifically rather than asserting // what NeedsBody implies. if pinned.NeedsResponseBody() { - slog.Warn("forward-proxy: text/event-stream response with a ReadsBody plugin that is not a StreamingResponder — streaming byte-for-byte; its OnResponse will see an empty body (implement StreamingResponder to inspect a streamed body)", "host", r.Host) + slog.Warn("forward-proxy: text/event-stream response with a ReadsBody plugin that is not a StreamingResponder — streaming byte-for-byte; its OnResponse will see an empty body (implement StreamingResponder to inspect a streamed body)", "host", pipeline.SanitizeLogValue(r.Host)) } s.streamPassthrough(w, r, resp, pctx) return @@ -1158,13 +1158,13 @@ func (s *Server) bridgeServe(client net.Conn, authority, host string, tl *tunnel defer cancel() req, err := http.NewRequestWithContext(ctx, http.MethodHead, "https://"+authority, nil) if err != nil { - slog.Info("tls-bridge passthrough", "host", host, "reason", "upstream-verify", "error", err) + slog.Info("tls-bridge passthrough", "host", pipeline.SanitizeLogValue(host), "reason", "upstream-verify", "error", err) tl.open(pipeline.TunnelOriginUnverified) return false } resp, err := s.TLSBridge.Upstream.Do(req) if err != nil { - slog.Info("tls-bridge passthrough", "host", host, "reason", "upstream-verify", "error", err) + slog.Info("tls-bridge passthrough", "host", pipeline.SanitizeLogValue(host), "reason", "upstream-verify", "error", err) tl.open(pipeline.TunnelOriginUnverified) return false // fall back to plain tunnel — agent's own e2e TLS still reaches origin } @@ -1214,7 +1214,7 @@ func (s *Server) bridgeServe(client net.Conn, authority, host string, tl *tunnel // HERE, because the connection is gone by the time anyone reads the log and no // later process listing can attribute it. args := []any{ - "host", host, + "host", pipeline.SanitizeLogValue(host), "reason", reason, "client", clientAddr(client), "error", err, @@ -1403,7 +1403,7 @@ func (s *Server) refuseMisdirected(w http.ResponseWriter, r *http.Request, pctx claimed = claimed[:255] } slog.Warn("forward-proxy: refused a bridged request whose Host is not the tunnel's", - "tunnel", r.URL.Host, "host", claimed, "method", r.Method) + "tunnel", pipeline.SanitizeLogValue(r.URL.Host), "host", pipeline.SanitizeLogValue(claimed), "method", r.Method) w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusMisdirectedRequest) _, _ = io.WriteString(w, misdirectedBody) @@ -1545,7 +1545,7 @@ func headerListContainsToken(h http.Header, name, token string) bool { // does for the 421. func (s *Server) declineResponsesWebSocket(w http.ResponseWriter, r *http.Request, pctx *pipeline.Context, tl *tunnelLog, sid string, chain []attribution.Proc) { slog.Debug("forward-proxy: declined a WebSocket upgrade on a Responses path; the client falls back to HTTPS", - "host", r.Host, "path", r.URL.Path) + "host", pipeline.SanitizeLogValue(r.Host), "path", pipeline.SanitizeLogValue(r.URL.Path)) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusUpgradeRequired) _, _ = io.WriteString(w, declinedUpgradeBody) @@ -1598,18 +1598,8 @@ func (s *Server) declineResponsesWebSocket(w http.ResponseWriter, r *http.Reques // forwarded as a transparent proxy or not at all — but no row is appended, which // is what skip_hosts promises. func (s *Server) relayUpgradedWebSocket(w http.ResponseWriter, r *http.Request, pctx *pipeline.Context, resp *http.Response, record bool) { - // The host as this function logs it. r.Host is client-controlled, so it goes - // through SanitizeControlRunes — every control rune replaced — and then - // through a strings.ReplaceAll that is a NO-OP by construction, because the - // sanitizer already took every control rune, "\n" included. The ReplaceAll - // is not ornament: CodeQL's go/log-injection (this PR's finding) cannot see - // through a custom function however thorough, and recognizes exactly two - // sanitizers — this ReplaceAll shape and %q formatting — so the one it knows - // has to sit in THIS function, on the value's last step before a log call - // (the query's own test suite proves the shape only in the sink's function; - // fencing inside SanitizeControlRunes was tried at 6cae5d14 and changed - // nothing). A helper would put it back inside a callee — do not lift it. - host := strings.ReplaceAll(pipeline.SanitizeControlRunes(r.Host), "\n", "") + // The host as this function logs it: r.Host is client-controlled. + host := pipeline.SanitizeLogValue(r.Host) // BOTH early returns below must close the origin themselves. This function // owns resp.Body (serveOutbound skipped its Close defer, see the doc above), // and the goroutine that closes it on context-done is only armed further @@ -1628,7 +1618,7 @@ func (s *Server) relayUpgradedWebSocket(w http.ResponseWriter, r *http.Request, // committed its connection. Refuse before the hijack, while a 502 is still // writable (#1549 review). if !upgradesToWebSocket(resp.Header) { - got := strings.ReplaceAll(pipeline.SanitizeControlRunes(resp.Header.Get("Upgrade")), "\n", "") + got := pipeline.SanitizeLogValue(resp.Header.Get("Upgrade")) slog.Error("forward-proxy: origin switched to another protocol than the WebSocket requested — not relaying", "host", host, "upgrade", got) _ = resp.Body.Close() @@ -2521,11 +2511,11 @@ func (s *Server) bufferResponseBody(w http.ResponseWriter, r *http.Request, resp if err != nil { status, fail = pipeline.ExchangeFailure(r.Context(), err) if r.Context().Err() == nil { - slog.Warn("forward-proxy: response body read error", "host", r.Host, "kind", fail.Kind, "error", fail.Message) + slog.Warn("forward-proxy: response body read error", "host", pipeline.SanitizeLogValue(r.Host), "kind", fail.Kind, "error", fail.Message) } http.Error(w, `{"error":"response body read error"}`, http.StatusBadGateway) } else { - slog.Warn("forward-proxy: response body too large", "host", r.Host, "limit", maxBodySize) + slog.Warn("forward-proxy: response body too large", "host", pipeline.SanitizeLogValue(r.Host), "limit", maxBodySize) http.Error(w, `{"error":"response body too large"}`, http.StatusBadGateway) status, fail = http.StatusBadGateway, &pipeline.EventError{ Kind: "proxy_error", @@ -2575,11 +2565,11 @@ func classifyBrokenStream(pctx *pipeline.Context, r *http.Request, resp *http.Re fail.Code = strconv.Itoa(resp.StatusCode) pctx.StatusCode = 0 if r.Context().Err() != nil { - slog.Debug("forward-proxy: streaming response ended by client hangup", "host", r.Host, "error", err) + slog.Debug("forward-proxy: streaming response ended by client hangup", "host", pipeline.SanitizeLogValue(r.Host), "error", err) return fail, false } slog.Warn("forward-proxy: streaming response read error — aborting the client connection", - "host", r.Host, "kind", fail.Kind, "error", fail.Message) + "host", pipeline.SanitizeLogValue(r.Host), "kind", fail.Kind, "error", fail.Message) return fail, true } @@ -2624,7 +2614,7 @@ func (s *Server) handleStreamingResponse(w http.ResponseWriter, r *http.Request, // is supported by net/http's default ResponseWriter, so this // is a defensive guard for exotic wrappers (httptest with a // custom recorder, embedded servers). - slog.Warn("forward-proxy: ResponseWriter does not support flushing — falling back to buffered for streaming response", "host", r.Host) + slog.Warn("forward-proxy: ResponseWriter does not support flushing — falling back to buffered for streaming response", "host", pipeline.SanitizeLogValue(r.Host)) s.streamFallbackBuffered(w, r, resp, pctx) return } @@ -2657,7 +2647,7 @@ func (s *Server) handleStreamingResponse(w http.ResponseWriter, r *http.Request, // Headers already sent; we can't promote to 502, but // surface the policy violation so operators see it. slog.Warn("forward-proxy: streaming response rejected on finalization (headers already sent)", - "host", r.Host, "violation", finalAction.Violation) + "host", pipeline.SanitizeLogValue(r.Host), "violation", finalAction.Violation) } s.recordOutboundResponseEvent(pctx, resp.StatusCode, streamFail) }() @@ -2707,7 +2697,7 @@ func (s *Server) handleStreamingResponse(w http.ResponseWriter, r *http.Request, // stream, which is the best we can do without inspect- // before-forward semantics. slog.Warn("forward-proxy: streaming response rejected mid-stream by plugin", - "host", r.Host, "violation", respAction.Violation) + "host", pipeline.SanitizeLogValue(r.Host), "violation", respAction.Violation) break } } @@ -2718,7 +2708,7 @@ func (s *Server) handleStreamingResponse(w http.ResponseWriter, r *http.Request, // used to drop event:, id:, retry: and comment lines, which clients // type, resume and keep alive by. if _, err := w.Write(block.Raw); err != nil { - slog.Debug("forward-proxy: streaming write error", "host", r.Host, "error", err) + slog.Debug("forward-proxy: streaming write error", "host", pipeline.SanitizeLogValue(r.Host), "error", err) break } flusher.Flush() @@ -2749,7 +2739,7 @@ func (s *Server) streamPassthrough(w http.ResponseWriter, r *http.Request, resp // No incremental delivery possible on this ResponseWriter — buffer. // http.Flusher is implemented by net/http's default writer; this is a // defensive guard for test recorders and exotic wrappers. - slog.Warn("forward-proxy: ResponseWriter does not support flushing — falling back to buffered for streaming response", "host", r.Host) + slog.Warn("forward-proxy: ResponseWriter does not support flushing — falling back to buffered for streaming response", "host", pipeline.SanitizeLogValue(r.Host)) s.streamFallbackBuffered(w, r, resp, pctx) return } @@ -2793,7 +2783,7 @@ func (s *Server) streamPassthrough(w http.ResponseWriter, r *http.Request, resp n, readErr := body.Read(buf) if n > 0 { if _, writeErr := w.Write(buf[:n]); writeErr != nil { - slog.Debug("forward-proxy: streaming write error", "host", r.Host, "error", writeErr) + slog.Debug("forward-proxy: streaming write error", "host", pipeline.SanitizeLogValue(r.Host), "error", writeErr) return } flusher.Flush() @@ -2871,7 +2861,7 @@ func (s *Server) streamFallbackBuffered(w http.ResponseWriter, r *http.Request, break } if ferr != nil { - slog.Warn("forward-proxy: streaming response read error in fallback", "host", r.Host, "error", ferr) + slog.Warn("forward-proxy: streaming response read error in fallback", "host", pipeline.SanitizeLogValue(r.Host), "error", ferr) break } frameAction := s.OutboundPipeline.RunResponseFrame(foldCtx, pctx, frame, false) @@ -2895,7 +2885,7 @@ func (s *Server) streamFallbackBuffered(w http.ResponseWriter, r *http.Request, copyResponseHeaders(w, resp) w.WriteHeader(resp.StatusCode) if _, err := io.Copy(w, resp.Body); err != nil { - slog.Debug("response copy error", "host", r.Host, "error", err) + slog.Debug("response copy error", "host", pipeline.SanitizeLogValue(r.Host), "error", err) } } @@ -3021,7 +3011,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { // Sanitized like the serveOutbound refusal: host is client-controlled and // lands in a log file. slog.Info("forward-proxy: CONNECT destination refused by egress allow list", - "host", pipeline.SanitizeControlRunes(host)) + "host", pipeline.SanitizeLogValue(host)) httpx.WriteRejection(w, action) return } @@ -3044,7 +3034,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { pat, skipped := s.SkipHosts.MatchPattern(pctx.Host) if skipped { slog.Info("forward-proxy: skip_hosts match (CONNECT) — opening tunnel without pipeline + recording", - "host", pctx.Host, "pattern", pat) + "host", pipeline.SanitizeLogValue(pctx.Host), "pattern", pat) } var sessionID string @@ -3112,7 +3102,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { // Hijack() call happens after dial succeeds — http.Error needs an // un-hijacked ResponseWriter to deliver the dial-failure 502. if _, ok := w.(http.Hijacker); !ok { - slog.Error("forward-proxy: ResponseWriter does not support hijacking", "host", r.Host) + slog.Error("forward-proxy: ResponseWriter does not support hijacking", "host", pipeline.SanitizeLogValue(r.Host)) http.Error(w, `{"error":"connect not supported by listener"}`, http.StatusInternalServerError) return } @@ -3149,7 +3139,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { slog.Warn("forward-proxy: CONNECT destination refused by egress guard", // Sanitized like the Debug line in serveOutbound: r.Host is // client-controlled and lands in a log file. - "host", pipeline.SanitizeControlRunes(r.Host), "addr", ref.Addr.String()) + "host", pipeline.SanitizeLogValue(r.Host), "addr", ref.Addr.String()) // A policy answer, not a failed exchange. skip_hosts destinations // are refused too — a bypass must not become an SSRF bypass — and // tl's rows are no-ops for them, exactly as the dial failure's are. @@ -3175,7 +3165,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { // embeds it (net.DNSError quotes the name it failed to resolve). if !bridgeable { slog.Warn("forward-proxy: CONNECT destination resolution failed", - "host", pipeline.SanitizeControlRunes(r.Host), + "host", pipeline.SanitizeLogValue(r.Host), "error", pipeline.SanitizeControlRunes(cerr.Error())) http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) tl.open(pipeline.TunnelDialFailed) @@ -3207,7 +3197,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { var dialFail, freshDialFail error if err != nil { if !bridgeable { - slog.Warn("forward-proxy: CONNECT upstream dial failed", "host", r.Host, "error", err) + slog.Warn("forward-proxy: CONNECT upstream dial failed", "host", pipeline.SanitizeLogValue(r.Host), "error", err) http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) // Recorded rather than dropped: an unreachable destination used to leave no // trace in the timeline at all. @@ -3216,7 +3206,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { return } slog.Warn("forward-proxy: CONNECT upstream dial failed; trying the TLS bridge", - "host", r.Host, "error", err) + "host", pipeline.SanitizeLogValue(r.Host), "error", err) dialFail = err if !memoized { freshDialFail = err @@ -3228,7 +3218,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { if upstream != nil { _ = upstream.Close() } - slog.Error("forward-proxy: CONNECT hijack failed", "host", r.Host, "error", err) + slog.Error("forward-proxy: CONNECT hijack failed", "host", pipeline.SanitizeLogValue(r.Host), "error", err) return } @@ -3251,7 +3241,7 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { if upstream != nil { _ = upstream.Close() } - slog.Debug("forward-proxy: CONNECT 200 write failed", "host", r.Host, "error", err) + slog.Debug("forward-proxy: CONNECT 200 write failed", "host", pipeline.SanitizeLogValue(r.Host), "error", err) return } diff --git a/core/listener/forwardproxy/transparent.go b/core/listener/forwardproxy/transparent.go index e82c12f80..a407b2cc7 100644 --- a/core/listener/forwardproxy/transparent.go +++ b/core/listener/forwardproxy/transparent.go @@ -82,7 +82,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { host = net.JoinHostPort(name, port) } slog.Debug("transparent-proxy: recovered destination host for policy", - "host", name, "dst", dst) + "host", pipeline.SanitizeLogValue(name), "dst", dst) } } @@ -128,7 +128,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { // Carry the identity this connection was gated under, so the denial lands // in the session whose traffic it was. s.recordOutboundReject(pctx, action, s.tunnelSessionID(sessionID, nil, nil)) - slog.Warn("transparent-proxy: outbound rejected by policy", "host", host) + slog.Warn("transparent-proxy: outbound rejected by policy", "host", pipeline.SanitizeLogValue(host)) return } @@ -159,7 +159,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { // so it gets the sanitizer; dst is the kernel's SO_ORIGINAL_DST // literal and ref.Addr the guard's parsed address — neither is // client-controlled. - "host", pipeline.SanitizeControlRunes(host), + "host", pipeline.SanitizeLogValue(host), "dst", dst, "addr", ref.Addr.String()) // The 426 rule, as on both CONNECT paths: a non-zero StatusCode is // what OutcomeFromContext reads as an allow, and the guard did what @@ -195,7 +195,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { var dialFail, freshDialFail error if err != nil { if !s.bridgeable(dst) { - slog.Warn("transparent-proxy: upstream dial failed", "host", host, "dst", dst, "error", err) + slog.Warn("transparent-proxy: upstream dial failed", "host", pipeline.SanitizeLogValue(host), "dst", dst, "error", err) // The 502 is synthetic — this path sends the client no status at all — but the // failure is recorded as it is on CONNECT rather than leaving no trace. tl.open(pipeline.TunnelDialFailed) @@ -203,7 +203,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { return } slog.Warn("transparent-proxy: upstream dial failed; trying the TLS bridge", - "host", host, "dst", dst, "error", err) + "host", pipeline.SanitizeLogValue(host), "dst", dst, "error", err) dialFail = err if !memoized { freshDialFail = err @@ -253,7 +253,7 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { s.tunnelOrDial(clientConn, dialTarget, tl, freshDialFail) return } - slog.Info("tls-bridge passthrough", "host", key, "reason", reason) + slog.Info("tls-bridge passthrough", "host", pipeline.SanitizeLogValue(key), "reason", reason) } } // The pre-dial failed (or was skipped as memoized-dead) and the bridge diff --git a/core/listener/internal/bodyread/bodyread.go b/core/listener/internal/bodyread/bodyread.go index 7f64cf04e..44ea46ebb 100644 --- a/core/listener/internal/bodyread/bodyread.go +++ b/core/listener/internal/bodyread/bodyread.go @@ -25,9 +25,9 @@ import ( // (chunked), where the true size is unknowable here. func LogError(listener string, r *http.Request, read int, limit int64, err error) { attrs := []any{ - "host", r.Host, + "host", pipeline.SanitizeLogValue(r.Host), "method", r.Method, - "path", r.URL.Path, + "path", pipeline.SanitizeLogValue(r.URL.Path), "error", err, "read", read, "limit", limit, diff --git a/core/listener/reverseproxy/server.go b/core/listener/reverseproxy/server.go index 40941b372..7d54eef30 100644 --- a/core/listener/reverseproxy/server.go +++ b/core/listener/reverseproxy/server.go @@ -334,7 +334,7 @@ func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) { if s.transparentInbound { if _, ok := transparentproxy.OrigDstFromContext(r.Context()); !ok { slog.Warn("reverse-proxy: rejecting request with no recovered destination", - "remote", r.RemoteAddr, "host", r.Host, "path", r.URL.Path) + "remote", r.RemoteAddr, "host", pipeline.SanitizeLogValue(r.Host), "path", pipeline.SanitizeLogValue(r.URL.Path)) http.Error(w, "no original destination recovered", http.StatusBadGateway) return } @@ -395,7 +395,7 @@ func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) { } r.Body = io.NopCloser(bytes.NewReader(body)) pctx.Body = body - slog.Debug("reverse-proxy: buffered request body", "host", r.Host, "bodyLen", len(body)) + slog.Debug("reverse-proxy: buffered request body", "host", pipeline.SanitizeLogValue(r.Host), "bodyLen", len(body)) } action := s.InboundPipeline.Run(r.Context(), pctx) @@ -515,7 +515,7 @@ func (s *Server) modifyResponse(resp *http.Response) error { pinned.HasStreamingResponders() && resp.Body != nil { if pinned.WritesResponseBody() { - slog.Warn("reverse-proxy: text/event-stream response with WritesResponseBody plugin — falling back to buffered path", "host", pctx.Host) + slog.Warn("reverse-proxy: text/event-stream response with WritesResponseBody plugin — falling back to buffered path", "host", pipeline.SanitizeLogValue(pctx.Host)) } else { s.installStreamingResponseBody(resp, pctx) // Strip Content-Length — the framing reader doesn't know @@ -707,7 +707,7 @@ func (s *Server) errorHandler(w http.ResponseWriter, r *http.Request, err error) // that is handleUpgradeResponse failing a 101 (a protocol mismatch, a // failed hijack), and a second row would break the one-response-per- // request pairing /v1/usage counts on. - slog.Warn("reverse-proxy: backend upgrade failed", "host", r.Host, "status", pctx.StatusCode, "error", err) + slog.Warn("reverse-proxy: backend upgrade failed", "host", pipeline.SanitizeLogValue(r.Host), "status", pctx.StatusCode, "error", err) return } @@ -725,7 +725,7 @@ func (s *Server) errorHandler(w http.ResponseWriter, r *http.Request, err error) status, fail = pipeline.ExchangeFailure(r.Context(), err) } if r.Context().Err() == nil { - slog.Warn("reverse-proxy: backend request failed", "host", r.Host, "method", r.Method, "kind", fail.Kind, "error", fail.Message) + slog.Warn("reverse-proxy: backend request failed", "host", pipeline.SanitizeLogValue(r.Host), "method", r.Method, "kind", fail.Kind, "error", fail.Message) } if pctx == nil { return diff --git a/core/pipeline/client.go b/core/pipeline/client.go index ff6b59477..cf2da0a54 100644 --- a/core/pipeline/client.go +++ b/core/pipeline/client.go @@ -163,6 +163,17 @@ func SanitizeControlRunes(s string) string { return b.String() } +// SanitizeLogValue is SanitizeControlRunes plus the two strings.ReplaceAll +// calls CodeQL's go/log-injection query recognizes as sanitizers: the calls +// are markers the scanner reads, and the bytes they target are already gone +// by the time they run. For any client-controlled value headed into slog, +// this rather than SanitizeControlRunes directly. +func SanitizeLogValue(s string) string { + s = SanitizeControlRunes(s) + s = strings.ReplaceAll(s, "\n", "") + return strings.ReplaceAll(s, "\r", "") +} + // hasControlRunes reports whether s carries anything SanitizeControlRunes would replace. // // A RUNE scan rather than a byte scan, because the C1 block cannot be seen from the diff --git a/core/pipeline/client_test.go b/core/pipeline/client_test.go index 42a2b07a3..f7f79d2a4 100644 --- a/core/pipeline/client_test.go +++ b/core/pipeline/client_test.go @@ -954,3 +954,19 @@ func TestFoldAgentNames(t *testing.T) { t.Errorf("FoldAgentNames(nil) = %v, want empty", got) } } + +// SanitizeLogValue is what every client-controlled host value passes through +// on its way into slog: control runes — C0, DEL, C1 and CR/LF — become U+FFFD. +func TestSanitizeLogValue(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + {"api.example.com", "api.example.com"}, + {"evil\nhost", "evil\uFFFDhost"}, + {"evil\rhost", "evil\uFFFDhost"}, + {"esc\x1b[2J", "esc\uFFFD[2J"}, + {"c1\u009b", "c1\uFFFD"}, + } { + if got := SanitizeLogValue(tc.in); got != tc.want { + t.Errorf("SanitizeLogValue(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/core/session/archive/archive.go b/core/session/archive/archive.go index aa9ddd996..f2c84fd52 100644 --- a/core/session/archive/archive.go +++ b/core/session/archive/archive.go @@ -354,6 +354,9 @@ func (a *Archive) Rekeyed(oldID, newID string) { case a.ops <- op{kind: opRename, id: oldID, to: newID, after: st.after}: default: a.droppedRenames.Add(1) + // Session ids are attribution keys the operator needs to find the + // history this rename left behind. + // codeql[go/clear-text-logging] slog.Error("session archive: a rename was lost to a full queue; the session's history stays under its old id", "from", oldID, "to", newID) } diff --git a/core/session/idheader.go b/core/session/idheader.go index 4f4b7f94f..3c6bd89de 100644 --- a/core/session/idheader.go +++ b/core/session/idheader.go @@ -3,6 +3,7 @@ package session import ( "log/slog" "net/http" + "strings" "unicode" "unicode/utf8" ) @@ -172,6 +173,11 @@ func IDHeaderFrom(h http.Header, names []string) (id, header string) { // TestIDFromHeaders_InvalidWinnerFallsThroughToNextHeader. continue } + // Sanitizer markers for CodeQL's go/log-injection query: they cut the + // taint at its source, where the validation above has already refused + // every control rune. + id = strings.ReplaceAll(id, "\n", "") + id = strings.ReplaceAll(id, "\r", "") return id, name } return "", "" diff --git a/core/session/store.go b/core/session/store.go index b25fc14fb..658b9015a 100644 --- a/core/session/store.go +++ b/core/session/store.go @@ -1406,12 +1406,16 @@ func (s *Store) publishLocked(event pipeline.SessionEvent) { // since the parsers already log them. func logAppended(sessionID string, e *pipeline.SessionEvent) { attrs := []any{ + // A session id is an attribution key the operator needs in the log to + // answer "which session was this bucket?" — validated at the source + // before IDHeaderFrom returns it, and the headers the query targets + // (Authorization, Cookie) are read elsewhere. "sessionId", sessionID, "direction", e.Direction.String(), "phase", e.Phase.String(), } if e.Host != "" { - attrs = append(attrs, "host", e.Host) + attrs = append(attrs, "host", pipeline.SanitizeLogValue(e.Host)) } if e.StatusCode != 0 { attrs = append(attrs, "status", e.StatusCode) @@ -1444,6 +1448,9 @@ func logAppended(sessionID string, e *pipeline.SessionEvent) { if e.Error != nil { attrs = append(attrs, "errorKind", e.Error.Kind) } + // The session id in attrs is an attribution key — see the comment on it + // where attrs is built. + // codeql[go/clear-text-logging] slog.Debug("session: event appended", attrs...) }