github / github/codeql

Java: Should these sanitizer heuristics completely stop taint in five security queries?

Offen
#22,262 2 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
question
Vorherrschende Sprache
CodeQL
Sterne
10.1k
Forks
2.1k
Ø Merge
2 T. 15 Std.
Gemergte PRs (30 T.)
141

Beschreibung

**Description of the issue**

I am trying to understand whether the following Java sanitizer heuristics are
intended to act as complete barriers. Each one suppresses a concrete result
even though the security effect relevant to the query remains possible. The
examples below show the behavior and ask whether the models can be narrowed.

| Query | Model | Baseline alerts | Reference alerts |
|---|---|---:|---:|
| `java/log-injection` | `LineBreaksLogInjectionSanitizer` | 0 | 1 |
| `java/unvalidated-url-redirection` | `ContainsUrlSanitizer` | 0 | 1 |
| `java/unvalidated-url-redirection` | `RegexpCheckRequestForgerySanitizer` | 0 | 1 |
| `java/command-line-injection` | `isSafeCommandArgument` | 0 | 1 |
| `java/xss` | `HtmlEscapeXssSanitizer` | 0 | 1 |
| `java/ssrf` | `ContainsUrlSanitizer` | 0 | 1 |

All six results were reproduced with CodeQL CLI 2.25.6 and the CodeQL Java
queries at commit `f6f45d1536312f53eed079868e344a5906bf3d72`. The relevant
models were also checked and remain present on upstream `main` at commit
`a8a77a60a68540662688e3e63e93021a8b8ed74a` (2026-07-30); the analyzer counts
below are from the tested `f6f45d1` snapshot.

## 1. `java/log-injection`: removing only one line-break character

`logInjectionSanitizer` considers a `String.replace` call sanitizing when its
target is either CR or LF and its replacement does not contain CR/LF:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/LogInjection.qll#L66-L92

```java
import java.util.logging.Logger;
import javax.servlet.http.HttpServletRequest;

class Witness {
static void handler(HttpServletRequest request) {
String input = request.getParameter("q");
String sanitized = input.replace("\r", "");
Logger.getLogger("x").info("user: " + sanitized);
}
}
```

An input containing `attack\nforged` retains the LF and can still forge a log
line. `java/log-injection` reports no alert. Removing only the `replace` call
produces one alert at the same logger sink.

Question: is removal of only one line-break character intended to be a complete
sanitizer? Would it be reasonable to require a composition that removes or
escapes both CR and LF before treating the value as fully sanitized?

## 2. `ContainsUrlSanitizer`: any method named `contains`

`isContainsUrlSanitizer` matches the first argument of any method named
`contains`; the implementation comment acknowledges that it also matches
unrelated methods:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/RequestForgery.qll#L95-L118

The following user-defined method performs only a bypassable prefix check:

```java
import javax.servlet.http.*;

class RedirectWitness {
static class UrlChecker {
boolean contains(String url) {
return url != null && url.startsWith("https://trusted.com");
}
}

void handle(HttpServletRequest req, HttpServletResponse resp)
throws Exception {
String url = req.getParameter("next");
if (new UrlChecker().contains(url)) {
resp.sendRedirect(url);
}
}
}
```

`https://trusted.com.evil.com` passes this check but redirects to the external
host `trusted.com.evil.com`. The `java/unvalidated-url-redirection` query
reports zero alerts. Renaming the method to `isAllowed`, without changing its
implementation or call site semantics, produces one alert.

The same model also suppresses `java/ssrf` for a server-side request:

```java
import java.net.URL;
import java.net.URLConnection;
import javax.servlet.http.*;

class SsrfWitness {
static class HostChecker {
boolean contains(String url) {
return url != null && url.startsWith("https://trusted.com");
}
}

void handle(HttpServletRequest req) throws Exception {
String url = req.getParameter("target");
if (new HostChecker().contains(url)) {
URLConnection connection = new URL(url).openConnection();
connection.connect();
}
}
}
```

Again, the baseline reports zero alerts and an otherwise identical method named
`isAllowed` produces one `java/ssrf` alert.

Question: is matching unknown user-defined methods by simple name intended?
Would it be reasonable to restrict this sanitizer to known collection types
and require the receiver to represent a trusted, fixed allowlist, while
retaining taint for unrelated methods named `contains`?

## 3. URL regexp checks treated as complete sanitizers

`RegexpCheckRequestForgerySanitizer` treats a regexp check as a complete
`RequestForgerySanitizer`:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/RequestForgery.qll#L151-L155

```java
import javax.servlet.http.*;

class Witness {
void handle(HttpServletRequest req, HttpServletResponse resp)
throws Exception {
String url = req.getParameter("next");
if (url.matches("https://trusted\\.com.*")) {
resp.sendRedirect(url);
}
}
}
```

The regexp accepts `https://trusted.com.evil.com`, whose host is external.
`java/unvalidated-url-redirection` nevertheless reports no alert. Removing only
the guard produces one alert.

Question: is a regexp check intended to be a complete sanitizer when it permits
the external-host bypass above? Would it be reasonable to retain URL taint
unless the regexp establishes an appropriate destination property?

## 4. `java/command-line-injection`: delegating launchers

`isSafeCommandArgument` treats a non-first argument as safe whenever the first
array element is not recognized as a shell:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/CommandArguments.qll#L11-L30

The shell recognition is a fixed name regexp and does not include delegating
launchers such as POSIX `env`:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/CommandArguments.qll#L33-L39

```java
import java.io.*;
import java.net.Socket;

class Witness {
static void handler(Socket socket) throws Exception {
BufferedReader reader = new BufferedReader(
new InputStreamReader(socket.getInputStream()));
String input = reader.readLine();
Runtime.getRuntime().exec(new String[]{"env", input});
}
}
```

On POSIX systems, the `env` utility executes its first non-option operand as a
command. Therefore an input such as `uname` selects the effective executable;
it is not inert data. The `java/command-line-injection` query reports no alert
for the baseline. A direct-execution control using
`Runtime.getRuntime().exec(input)` produces one alert, as do controls using
`{"sh", input}` or `{input, "x"}`. The direct-execution control changes the
launcher topology and is corroborating analyzer evidence rather than a
semantics-preserving program transformation.

Question: should arguments interpreted as commands by delegating launchers be
excluded from `isSafeCommandArgument`? Would it be reasonable to recognize
common delegating executables such as POSIX `env`, or avoid treating every
argument to an unknown non-shell executable as unconditionally safe?

## 5. `java/xss`: sanitizer recognition based only on method name

`HtmlEscapeXssSanitizer` treats the return value of any method whose name
matches `(?i)html_?escape.*` as sanitized:

https://github.com/github/codeql/blob/f6f45d1536312f53eed079868e344a5906bf3d72/java/ql/lib/semmle/code/java/security/XSS.qll#L69-L76

```java
import javax.servlet.http.*;

class Witness {
static String htmlEscape(String value) {
return value; // no-op
}

void handle(HttpServletRequest req, HttpServletResponse resp)
throws Exception {
String input = req.getParameter("q");
resp.getWriter().write(htmlEscape(input));
}
}
```

For input `alert(1)`, the method returns the payload unchanged,
but `java/xss` reports no alert. Renaming the method to `sanitizeOutput` while
leaving the implementation identical produces one alert.

Question: is method-name-only recognition intended to cover arbitrary
application methods? Would it be reasonable to model known framework escaping
APIs by canonical symbol identity while retaining taint for unknown
user-defined methods?

## Reproduction procedure

For the command-line example, create the database with its Java source:

```text
codeql database create db --language=java \
--source-root=. --command='javac *.java'
codeql database analyze db \
--format=sarif-latest --output=baseline.sarif
```

The log-injection, URL, XSS, and SSRF examples use minimal
`javax.servlet.http` interfaces with the correct fully qualified names during
extraction. The required declarations are:

```java
package javax.servlet.http;

import java.io.PrintWriter;

public interface HttpServletRequest {
String getParameter(String name);
}

public interface HttpServletResponse {
void sendRedirect(String location) throws java.io.IOException;
PrintWriter getWriter() throws java.io.IOException;
}
```

Place each public interface in its corresponding file under
`javax/servlet/http/`, compile it with the witness, and run the query listed in
the summary table. For each reference arm, make only the change described in
the relevant section and recreate the database. Each baseline has zero results
and each reference has one result.

## Questions / possible model directions

- Could the log-injection sanitizer require complete CR/LF neutralization?
- Could `ContainsUrlSanitizer` be restricted to known collection/allowlist
forms rather than every method named `contains`?
- Could regexp checks retain URL taint unless they establish a relevant
destination property?
- Could delegating launchers be accounted for in command-argument
classification?
- Could XSS sanitizer recognition use canonical models of known escaping APIs
rather than method names alone?

If these models are narrowed, could the examples above be added as `BAD` cases
while retaining genuinely safe controls for numeric types, correct host
comparisons, and real HTML escaping?

## Environment

- CodeQL CLI: 2.25.6
- CodeQL repository commit:
`f6f45d1536312f53eed079868e344a5906bf3d72`
- Java/Javac: OpenJDK 21.0.11
- Platform: Linux/amd64

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Beginne damit, die Java witness cases mit der CodeQL CLI zu reproduzieren, und lies dann LogInjection.qll, RequestForgery.qll, CommandArguments.qll und XSS.qll an den verlinkten Stellen. Damit wäre die Aufgabe erledigt: eine festgelegte Modellierungsrichtung, korrigierte Sanitizer- oder Argumentbehandlung, wo angemessen, sowie BAD-Fälle und sichere Kontrollen, die die gemeldeten Beispiele abdecken.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
java
Bereich
security
Issue-Typ
Bug
Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Aktivitätsstatus
Ruhig
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
28/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.