From b0fc06731715cdda24933160a9010b30610444b9 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 4 May 2026 05:29:35 +0000 Subject: [PATCH] security: close CodeQL #17 (log injection) + #23 (SSRF false-positive reopen) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeQL alerts in one sweep — both medium-impact follow-ups on already-merged guards. Alert #17 — go/log-injection (CWE-117) at internal/api/middleware/middleware.go:58: log.Printf("[%s] %s %s %d %v", requestID, r.Method, r.URL.Path, ...) r.Method and r.URL.Path are attacker-controllable (Go's net/http percent-decodes path segments before they reach handlers, so r.URL.Path can contain CR/LF in the decoded form even though raw HTTP request lines cannot). An attacker who controls a URL can forge new log entries by embedding %0A%0Afake-log-line. Fix: introduce scrubLogValue helper that replaces CR/LF/NUL with spaces. Apply to both r.Method and r.URL.Path. Replacement is structural (collapse to space) not destructive (drop) so an operator scanning the log still sees the field was present, just neutralized. Cheap fast path when the value contains no control chars (the common case). The deprecation comment on this function recommends NewLogging (slog with structured fields) where the logger escapes per-field natively. The Logging function is preserved for back-compat callers; the scrubber is the load-bearing CWE-117 defense for the legacy path. Alert #23 — go/request-forgery (CWE-918) at scep_probe.go:271: CodeQL reopened the alert after commit e6919cd. The commit's in-function validator dispatch went through a function-pointer override hook: validateURL := s.scepValidateURL // could be anything if validateURL == nil { validateURL = validation.ValidateSafeURL } if err := validateURL(rawURL); err != nil { ... } CodeQL's taint tracker doesn't trust the if-nil branch — the override field could be set to a permissive validator, and the analyzer can't prove the production validator runs. Fix: invert the dispatch. Always call validation.ValidateSafeURL literally first; only consult the test-override hook to grant an EXEMPTION when the production validator rejects: if err := validation.ValidateSafeURL(rawURL); err != nil { if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { return ... validate url error } } Same applies to ProbeSCEP's entry-point validator. Both call sites now have the literal validation.ValidateSafeURL call in-scope of the sink (client.Do), which CodeQL recognizes as a sanitizer. Production behavior is unchanged: scepValidateURL is nil in production, so the production validator's rejection is the only gate. Test ergonomics are preserved: scepValidateURL still grants the test-only exemption for httptest loopback URLs (only difference: the override now grants exemption from production validator's rejection rather than replacing the validator entirely; identical net effect). Verified locally: gofmt: clean (strings is already imported in middleware.go). go vet ./internal/api/middleware/... + ./internal/service/...: exit 0. go test -short ./internal/api/middleware/...: ok 0.244s. go test -short ./internal/service/...: ok 4.965s (every existing scep_probe test still green — production + httptest paths both work). References: https://github.com/certctl-io/certctl/security/code-scanning/17 https://github.com/certctl-io/certctl/security/code-scanning/23 Closes CodeQL #17. Re-closes CodeQL #23 with a fix CodeQL's taint tracker can verify. --- internal/api/middleware/middleware.go | 30 +++++++++++++++- internal/service/scep_probe.go | 51 ++++++++++++++++----------- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/internal/api/middleware/middleware.go b/internal/api/middleware/middleware.go index a1bdb34..4e795ee 100644 --- a/internal/api/middleware/middleware.go +++ b/internal/api/middleware/middleware.go @@ -44,6 +44,13 @@ func RequestID(next http.Handler) http.Handler { // Logging middleware logs request details including method, path, status, and duration. // Deprecated: Use NewLogging for structured logging with slog. +// +// CWE-117 log-injection defense: r.Method and r.URL.Path are +// attacker-controllable (request-line bytes — Go's net/http leaves +// percent-decoded path segments in r.URL.Path, which can include CR/LF +// in the decoded form even though the raw HTTP request line cannot). +// strings.ReplaceAll on CR/LF/NUL strips the forgery vector before the +// log line is emitted. Closes CodeQL #17 (go/log-injection). func Logging(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -55,10 +62,31 @@ func Logging(next http.Handler) http.Handler { duration := time.Since(start) requestID := getRequestID(r.Context()) - log.Printf("[%s] %s %s %d %v", requestID, r.Method, r.URL.Path, wrapped.statusCode, duration) + log.Printf("[%s] %s %s %d %v", + requestID, + scrubLogValue(r.Method), + scrubLogValue(r.URL.Path), + wrapped.statusCode, + duration, + ) }) } +// scrubLogValue strips control characters that an attacker could use to +// forge log entries (CWE-117). The replacement is structural, not +// destructive: we collapse CR/LF/NUL to a single space rather than +// dropping them, so an operator scanning the log can still see that the +// field was present (just neutralized). Defense in depth: the +// production middleware should prefer NewLogging (slog with structured +// fields) where the logger handles escape on its own. +func scrubLogValue(v string) string { + if !strings.ContainsAny(v, "\r\n\x00") { + return v + } + r := strings.NewReplacer("\r", " ", "\n", " ", "\x00", " ") + return r.Replace(v) +} + // NewLogging creates a structured logging middleware using slog. // Logs request_id, method, path, status, duration_ms, and remote_addr. func NewLogging(logger *slog.Logger) func(http.Handler) http.Handler { diff --git a/internal/service/scep_probe.go b/internal/service/scep_probe.go index d03e64f..c78995b 100644 --- a/internal/service/scep_probe.go +++ b/internal/service/scep_probe.go @@ -75,20 +75,21 @@ func (s *NetworkScanService) ProbeSCEP(ctx context.Context, rawURL string) (*dom } // Step 1: cheap up-front URL validation (SSRF early diagnostic). - // Defaults to validation.ValidateSafeURL; tests inject a permissive - // validator via service-level field so they can hit httptest - // loopback servers (which the production validator correctly - // rejects). Mirrors the webhook notifier's `newForTest` pattern. - validateURL := s.scepValidateURL - if validateURL == nil { - validateURL = validation.ValidateSafeURL - } - if err := validateURL(rawURL); err != nil { - result.Reachable = false - result.Error = "url validation: " + err.Error() - result.ProbeDurationMs = time.Since(started).Milliseconds() - s.persistProbeResult(ctx, result) - return result, fmt.Errorf("scep probe: validate url: %w", err) + // Direct literal call to validation.ValidateSafeURL so CodeQL + // go/request-forgery sees the sanitizer in-scope of every + // downstream HTTP call. Tests that need to hit httptest loopback + // servers grant an exemption via s.scepValidateURL (mirrors the + // webhook notifier's `newForTest` pattern). Production callers + // leave scepValidateURL nil so any production-validator + // rejection wins. + if err := validation.ValidateSafeURL(rawURL); err != nil { + if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { + result.Reachable = false + result.Error = "url validation: " + err.Error() + result.ProbeDurationMs = time.Since(started).Milliseconds() + s.persistProbeResult(ctx, result) + return result, fmt.Errorf("scep probe: validate url: %w", err) + } } // Normalize the base URL — strip any trailing query string so we @@ -255,12 +256,22 @@ func (s *NetworkScanService) scepGetCACert(ctx context.Context, client *http.Cli // callers leave scepValidateURL nil so validation.ValidateSafeURL // is the active gate. func (s *NetworkScanService) scepHTTPGet(ctx context.Context, client *http.Client, rawURL string) ([]byte, error) { - validateURL := s.scepValidateURL - if validateURL == nil { - validateURL = validation.ValidateSafeURL - } - if err := validateURL(rawURL); err != nil { - return nil, fmt.Errorf("validate url: %w", err) + // Production-grade SSRF validator — direct literal call so CodeQL + // go/request-forgery recognizes it as a sanitizer in-scope of the + // client.Do sink below. Tests that need to hit httptest loopback + // servers grant an exemption via s.scepValidateURL (returning nil + // for the test URL); when no exemption applies, the production + // validator's rejection wins. Production callers leave + // scepValidateURL nil so the production validator is the only gate. + if err := validation.ValidateSafeURL(rawURL); err != nil { + // Test-only exemption hook. The override returns nil for URLs + // the test wants to allow despite the production validator's + // rejection (loopback / link-local in httptest scenarios). + // In production scepValidateURL is nil, so any production + // validator rejection bubbles up unconditionally. + if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { + return nil, fmt.Errorf("validate url: %w", err) + } } req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil)