security: close CodeQL #17 (log injection) + #23 (SSRF false-positive reopen)

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.
This commit is contained in:
shankar0123
2026-05-04 05:29:35 +00:00
parent c46a6aecbc
commit b0fc067317
2 changed files with 60 additions and 21 deletions
+29 -1
View File
@@ -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 {