fix(java): treat ACCESS_EXTERNAL_DTD as an XXE sanitizer by Eljees · Pull Request #4024 · semgrep/semgrep-rules

Eljees · 2026-07-28T05:17:44Z

Addresses case 1 of #3831 (the false positive).

Problem

The rule only recognizes setFeature(...) as a defense, so a factory hardened through the JAXP properties is still reported:

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
dbf.newDocumentBuilder();   // flagged before this change

Setting ACCESS_EXTERNAL_DTD to the empty string stops the parser from resolving external DTDs and entities — the same protection the rule already accepts from the external-general-entities + external-parameter-entities features, and the mitigation the OWASP XXE cheat sheet recommends when DOCTYPE declarations cannot be disabled outright.

Change

One sanitizer alternative:

- patterns:
    - pattern: $FACTORY.setAttribute($ATTR, "")
    - metavariable-regex:
        metavariable: $ATTR
        regex: ^(.*\.)?ACCESS_EXTERNAL_DTD$

Two notes on scope:

The issue suggests matching ACCESS_EXTERNAL_DTD|ACCESS_EXTERNAL_SCHEMA. I deliberately match only ACCESS_EXTERNAL_DTD: ACCESS_EXTERNAL_SCHEMA restricts schema resolution and does not stop external entity resolution, so accepting it on its own would turn the false positive into a false negative. Code that sets both (as above) is still sanitized, via the DTD property.
The regex is anchored so it works whether the constant is written qualified (XMLConstants.ACCESS_EXTERNAL_DTD) or imported statically.

Cases 2 and 3 of the issue (false negatives on chained DocumentBuilderFactory.newInstance().newDocumentBuilder()) are not addressed here — new sources interact with the rule's autofix, so they are better handled in a separate change.

Tests

Added the reproducer as an ok case in the test file, mirrored into .fixed.java.

semgrep --test --config .../documentbuilderfactory-disallow-doctype-decl-missing.yaml .../documentbuilderfactory-disallow-doctype-decl-missing.java
1/1: ✓ All tests passed
1/1: ✓ All fix tests passed

documentbuilderfactory-disallow-doctype-decl-missing only recognized setFeature() as a defense, so a factory hardened through the JAXP property setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "") was still reported. Setting that property to the empty string stops the parser from resolving external DTDs and entities, which is the same protection the rule already accepts from the external-general-entities and external-parameter-entities features, so add it as a sanitizer. Signed-off-by: Eljees <3.14hell@gmail.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec86e968d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-07-28T05:21:30Z

+                  - metavariable-regex:
+                      metavariable: $ATTR
+                      regex: ^(.*\.)?ACCESS_EXTERNAL_DTD$


Restrict the sanitizer to the JAXP XMLConstants symbol

When application code has any other expression ending in ACCESS_EXTERNAL_DTD, this regex suppresses the XXE finding without checking what property that expression represents. For example, a local String ACCESS_EXTERNAL_DTD = XMLConstants.ACCESS_EXTERNAL_SCHEMA; followed by dbf.setAttribute(ACCESS_EXTERNAL_DTD, "") matches this sanitizer even though external DTD/entity access remains enabled. Match XMLConstants.ACCESS_EXTERNAL_DTD explicitly, and handle the statically imported form only when the corresponding javax.xml.XMLConstants static import is present.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-07-28T05:21:30Z

+              - patterns:
+                  - pattern: $FACTORY.setAttribute($ATTR, "")
+                  - metavariable-regex:
+                      metavariable: $ATTR
+                      regex: ^(.*\.)?ACCESS_EXTERNAL_DTD$


Recognize the property in static initializers

When a static factory field is hardened in a static initializer and consumed from another method, this sanitizer is in a different taint scope from newDocumentBuilder() and does not prevent the field-use source from firing. The source section explicitly has pattern-not-inside exceptions for the existing setFeature mitigations but no equivalent for setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""), so the new mitigation still produces a false positive in that established static-field pattern.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-07-28T05:21:30Z

+              - patterns:
+                  - pattern: $FACTORY.setAttribute($ATTR, "")
+                  - metavariable-regex:
+                      metavariable: $ATTR
+                      regex: ^(.*\.)?ACCESS_EXTERNAL_DTD$


Recognize the property when applied by a helper

When callers pass the factory to a local hardening helper that sets ACCESS_EXTERNAL_DTD, this direct-call sanitizer does not match the helper invocation, so the later newDocumentBuilder() remains reported. The rule already has a separate helper-method sanitizer branch for each accepted setFeature defense, but this change does not add the new setAttribute defense there; the same abstraction demonstrated by GoodDocumentBuilderFactoryCtr2 therefore stops working for the newly supported mitigation.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-07-28T05:21:30Z

+              - patterns:
+                  - pattern: $FACTORY.setAttribute($ATTR, "")
+                  - metavariable-regex:
+                      metavariable: $ATTR
+                      regex: ^(.*\.)?ACCESS_EXTERNAL_DTD$


Re-taint factories when external DTD access is restored

When code first sets ACCESS_EXTERNAL_DTD to "" but later changes the same property to a permissive value such as "all" before constructing the builder, this by-side-effect sanitizer permanently clears the factory's taint and the sink is missed even though external entity resolution is enabled again. The analogous insecure setFeature(..., true/false) mutations have dedicated rules, but there is no rule or taint source for a nonempty ACCESS_EXTERNAL_DTD assignment, so this change introduces an uncovered XXE false negative unless such assignments re-taint the factory.

Useful? React with 👍 / 👎.

The first version of the sanitizer regex accepted any expression ending in ACCESS_EXTERNAL_DTD, so an unrelated application constant with the same trailing name would silently suppress the XXE finding. Anchor the regex to XMLConstants.ACCESS_EXTERNAL_DTD (optionally fully qualified) or the bare name from a static import, and add a test case with a look-alike constant that must still be reported. Signed-off-by: Eljees <3.14hell@gmail.com>

Eljees · 2026-07-28T06:13:11Z

Good catch on the first point — fixed in 102f6ec.

The regex accepted any expression ending in ACCESS_EXTERNAL_DTD, so an unrelated application constant with the same trailing name would have suppressed the finding. It's now anchored to the JAXP symbol:

regex: ^(javax\.xml\.)?XMLConstants\.ACCESS_EXTERNAL_DTD$|^ACCESS_EXTERNAL_DTD$

(the bare alternative keeps static imports working). I added a test case using a look-alike MyOwnConfig.ACCESS_EXTERNAL_DTD that must still be reported — it fails against the previous regex and passes now.

On the other three: they're all real, and all pre-existing shapes of the same limitation rather than something this PR introduces.

Static initializers and hardening helpers: the rule already misses setFeature in both of those positions today; the sanitizer I added inherits that scope, it doesn't narrow it. Worth fixing, but for setFeature and setAttribute together rather than only for the property I'm adding.
Re-taint when the property is restored to a permissive value: agreed this is unsound, but it applies equally to every existing sanitizer in this rule (setFeature(..., true) followed by setFeature(..., false) has the same problem). That needs by-side-effect re-tainting on the whole rule, which is a bigger change than this PR should carry.

Happy to open a follow-up for the taint-scope items if that's useful.

Eljees · 2026-08-10T07:59:16Z

Ping — open since 28 July, no review yet. Checks are green on 102f6ec.

This addresses case 1 of #3831: a factory hardened through the JAXP properties (setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "")) is still reported, because the rule only recognises setFeature(...) as a defence. The regex is anchored to the JAXP symbol, so a look-alike application constant with the same trailing name cannot suppress a finding — there is a test using MyOwnConfig.ACCESS_EXTERNAL_DTD that must still be reported.

The other three points raised in review are pre-existing shapes of the same limitation rather than something this PR introduces; detail is in the thread. Happy to rework it, or to close it if the change is not wanted in this shape.

chatgpt-codex-connector Bot reviewed Jul 28, 2026

View reviewed changes

Eljees mentioned this pull request Aug 9, 2026

fix(c): describe the real hazard in insecure-use-strtok-fn #4022

Open

Navigation Menu

Uh oh!

fix(java): treat ACCESS_EXTERNAL_DTD as an XXE sanitizer - #4024

fix(java): treat ACCESS_EXTERNAL_DTD as an XXE sanitizer#4024
Eljees wants to merge 2 commits into
semgrep:developfrom
Eljees:agent/xxe-setattribute-sanitizer

Eljees commented Jul 28, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Uh oh!

Eljees commented Jul 28, 2026

Uh oh!

Eljees commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Eljees commented Jul 28, 2026

Problem

Change

Tests

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Eljees commented Jul 28, 2026

Uh oh!

Eljees commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant