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

未关闭
#22,262 2 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

还没有人认领这个 Issue。

评估

难度
5/5
预计耗时
一周以上
新手友好度
28/100
Issue 类型
缺陷
描述清晰度
基本清楚
活跃度
冷清
技术栈
java
领域
security

调研方向

首先使用 CodeQL CLI 重现 Java witness cases,然后阅读链接位置的 LogInjection.qll、RequestForgery.qll、CommandArguments.qll 和 XSS.qll。完成这项工作需要确定建模方向,在适当情况下修正 sanitizer 或参数处理,并添加覆盖所报告示例的 BAD 案例和安全控制。

由索引模型根据 Issue 内容生成。

描述

question

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

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:

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:

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

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

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

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 <script>alert(1)</script>, 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:

codeql database create db --language=java \
  --source-root=. --command='javac *.java'
codeql database analyze db <query.ql> \
  --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:

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
主要语言
CodeQL
星标
10.1k
派生
2.1k
平均合并
2 天 11 小时
30 天内合并 PR
129

贡献指南

打开贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

github/codeql 的其他 Issue

查看 github/codeql 的全部 Issue

相似的 Issue

更多 Security Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。