From d6535110f9b5bad7d8536dc5a66e11633894aacc Mon Sep 17 00:00:00 2001 From: ralyodio Date: Mon, 10 Aug 2026 15:07:37 +0000 Subject: [PATCH] fix(scan): require XML evidence in the file before reporting XXE at parse() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `java-xxe-parse-call` matched on the receiver's *name* — anything suffixed Builder, Parser or Reader calling `.parse(`. That shape is right for the vulnerability, because the real XXE is `builder.parse(is)` and the declared type is rarely on that line, but the suffix says nothing about XML. Any parser that parses something else matched too, at high severity under CWE-611. On ionic-team/capacitor that meant `HostMask.Parser.parse(origins)` — a hostname mask — reported three times in Bridge.java and UriMatcher.java, plus three more in a unit test asserting on "*.example.org" strings. Adds `fileRequires`, a precondition tested against the whole file rather than the guard window, and points the rule at the XML packages and types. A file that parses XML imports javax.xml or org.xml.sax at the top; one that never mentions XML is not parsing it. The window could not answer this — an import sits hundreds of lines from the match, and widening guardBack far enough would drag unrelated evidence into every other rule. The whole-file text is memoised on the lines array so the check costs one join per file rather than one per line. Verified against ionic-team/capacitor: 19 findings to 13, removing all six java-xxe-parse-call reports and nothing else. Combined with the scoped typosquat fix, 21 to 13. Co-Authored-By: Claude Opus 5 (1M context) --- .../cli/src/scan/__tests__/code-rules.test.ts | 33 ++++++++++ apps/cli/src/scan/code-rules.ts | 60 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/apps/cli/src/scan/__tests__/code-rules.test.ts b/apps/cli/src/scan/__tests__/code-rules.test.ts index 8c72a63..7fd2777 100644 --- a/apps/cli/src/scan/__tests__/code-rules.test.ts +++ b/apps/cli/src/scan/__tests__/code-rules.test.ts @@ -139,6 +139,39 @@ describe('guard windows', () => { expect(ruleIds('a.java', bare)).toContain('java-xxe-parser-defaults'); }); + it('flags an unhardened parse in a file that parses XML', () => { + const source = [ + 'import javax.xml.parsers.DocumentBuilder;', + 'import javax.xml.parsers.DocumentBuilderFactory;', + '', + 'public Document read(InputStream is) throws Exception {', + ' return builder.parse(is);', + '}', + ].join('\n'); + expect(ruleIds('a.java', source)).toContain('java-xxe-parse-call'); + }); + + // The receiver suffix alone matched any `.parse()` on anything named Builder, + // Parser or Reader. A hostname-mask parser is not an XML parser. + it('stays silent on a parser that has nothing to do with XML', () => { + const source = [ + 'package com.getcapacitor;', + '', + 'public void setAllowedOrigins(String[] origins) {', + ' this.mask = HostMask.Parser.parse(origins);', + '}', + ].join('\n'); + expect(ruleIds('a.java', source)).toHaveLength(0); + }); + + it('stays silent on a date parser and a JSON reader', () => { + const source = [ + 'LocalDate when = dateParser.parse(raw);', + 'Config cfg = jsonReader.parse(body);', + ].join('\n'); + expect(ruleIds('a.java', source)).toHaveLength(0); + }); + it('looks forward for an ObjectInputFilter installed after the stream', () => { const filtered = [ 'ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(blob));', diff --git a/apps/cli/src/scan/code-rules.ts b/apps/cli/src/scan/code-rules.ts index a244c62..98d6d16 100644 --- a/apps/cli/src/scan/code-rules.ts +++ b/apps/cli/src/scan/code-rules.ts @@ -62,6 +62,18 @@ export interface CodeRule { * harmless until something executes it. */ requires?: RegExp; + /** + * Evidence that must appear somewhere in the file, not merely in the guard + * window. + * + * For rules whose *applicability* is settled far from the match. Whether a + * `.parse()` call is XML parsing is decided by an import at the top of the + * file, which in a 1,600-line source is nowhere near the line that matched; + * widening `guardBack` far enough to reach it would drag in unrelated + * evidence for every other rule. Distinct from `requires`, which asks + * whether the surrounding lines complete a dangerous combination. + */ + fileRequires?: RegExp; /** * Evidence that the construct is already handled. `false` opts the rule out * of the generic guard entirely — the CWE-532 rules are *about* reading @@ -163,6 +175,17 @@ export const GENERIC_GUARD = const XXE_GUARD = /FEATURE_SECURE_PROCESSING|setExpandEntityReferences|disallow-doctype-decl|external-general-entities|external-parameter-entities|setXIncludeAware\s*\(\s*false/; +/** + * Evidence that a Java source parses XML at all. + * + * The package names are the reliable half: a file that reaches for + * `javax.xml.parsers` or `org.xml.sax` has declared its intent at the top, + * whatever the local variable ends up being called. The bare type names cover + * sources that import by wildcard or sit in the same package. + */ +const XML_PARSING_FILE = + /\b(?:javax\.xml|org\.xml\.sax|org\.w3c\.dom|org\.jdom2?|org\.dom4j|XmlPullParser|DocumentBuilderFactory|DocumentBuilder|SAXParserFactory|SAXParser|XMLInputFactory|XMLReaderFactory|XMLReader|SAXBuilder|SAXReader)\b/; + /** A sink that executes whatever string reaches it. */ const CODE_SINK = /\bglobalThis\s*\[|\bconstructor\b|\beval\b|\bFunction\b|\brun\s*\(|\bvm\s*\.\s*run/; @@ -507,6 +530,16 @@ export const CODE_RULES: readonly CodeRule[] = [ }, // ── XML external entities ──────────────────────────────────────────────── + // + // Evidence that a file parses XML at all. The XXE rules match on receiver + // *names* — `builder.parse(is)` is the shape the vulnerability actually takes, + // and the declared type is rarely on that line — so without this the pattern + // reads any `.parse()` on anything suffixed Builder, Parser or Reader as XML. + // In practice that meant a hostname-mask parser (`HostMask.Parser.parse(...)`) + // was reported as CWE-611 at high severity. + // + // An import is the cheapest honest signal: a file that parses XML says so at + // the top, and one that never mentions XML is not parsing it. { id: 'java-xxe-parser-defaults', title: 'XML parser left on its insecure defaults', @@ -530,7 +563,10 @@ export const CODE_RULES: readonly CodeRule[] = [ severity: 'high', languages: ['java'], // Receiver-qualified so `LocalDate.parse(s)` and friends stay out of it. + // The suffix alone is not enough — plenty of parsers parse things that are + // not XML — so `fileRequires` decides whether the file is in scope at all. pattern: /\b\w*(?:[Bb]uilder|[Pp]arser|[Rr]eader)\s*\.\s*parse\s*\(/, + fileRequires: XML_PARSING_FILE, guard: XXE_GUARD, guardBack: 6, guardForward: 4, @@ -802,6 +838,26 @@ function windowText( return collected.join('\n'); } +/** + * Whole-file text, memoised on the `lines` array it came from. + * + * `fileRequires` asks a question no window can answer, but joining the file on + * every line of every rule would make scanning quadratic in file length. The + * caller already reuses one `lines` array for the whole file, so keying on its + * identity gives one join per file. A `WeakMap` keeps nothing alive after the + * file is done with. + */ +const FILE_TEXT = new WeakMap(); + +function fileTextOf(lines: readonly string[]): string { + let text = FILE_TEXT.get(lines); + if (text === undefined) { + text = lines.join('\n'); + FILE_TEXT.set(lines, text); + } + return text; +} + export interface RuleMatch { rule: CodeRule; confidence: Confidence; @@ -821,6 +877,10 @@ export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | nul if (isComment(line) || ctx.prose?.has(ctx.index)) return null; if (!rule.pattern.test(line)) return null; + // Before any window work: a rule whose file-level precondition fails does not + // apply to this file at all. + if (rule.fileRequires && !rule.fileRequires.test(fileTextOf(ctx.lines))) return null; + const back = rule.guardBack ?? 8; const forward = rule.guardForward ?? 0; const context = windowText(ctx.lines, ctx.index, back, forward, ctx.prose);