OPC-1507 | Genesys XXE Vulnerability

[jspencer92]

Card(s)

• [Apiiro] Managed Semgrep - High severity finding · High Risk + 1 more

Related Pull Requests

• NA

Changes Made

1. Disabled XML parsing features vulnerable to XXE attacks

Test Procedure

Scenario:
Given:
And:
When:
Then:

[Copilot]

Pull request overview

This PR hardens the shared XML parsing utility (XMLUtilities.getDocumentBuilder()) to mitigate an XXE (CWE-611) vulnerability path (notably affecting Genesys attached-data parsing) by configuring the underlying DocumentBuilderFactory to reject/disable external entity expansion and external DTD loading.

Changes:

• Configures DocumentBuilderFactory features to disallow DOCTYPE declarations and disable external general/parameter entities.
• Disables XInclude and entity-reference expansion on the created DOM builder factory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bward-mdla]

Claude says...

Verdict: Directionally correct, but this is a cheat-sheet paste that closes a Semgrep finding without remediating the vulnerability class. One of the six lines is wrong, the hardening reaches exactly one low-traffic call path, and the more exposed runtime parsers are left open. Request changes.

Scope: XMLUtilities.getDocumentBuilder(), +15/−0, one file. Confirmed blast radius (grep): the only caller of getDocumentBuilder() is GenesysVoicePlatform.java:177; the sibling loadDocument() has zero callers. So every effect of this PR — protection and behavior change — applies solely to Genesys attached-data parsing.

---

1. setExpandEntityReferences(false) is not a security control — remove it

This line reveals the block was copied without understanding. It blocks no XXE vector. It only changes the DOM representation of internal general entities: false leaves EntityReference nodes in the tree instead of expanding them inline.

• With disallow-doctype-decl=true already set, no DTD can be declared, so there are no custom entities to expand — the line is a no-op today.
• If disallow-doctype-decl is ever relaxed, it becomes a correctness hazard: the DOM would carry EntityReference nodes, and the caller's getElementsByTagName("key") / getAttribute("value") logic in GenesysVoicePlatform does not walk entity-reference children, so content would silently vanish.

It is on neither the primary nor the fallback path of the OWASP XXE cheat sheet. Delete it.

2. One line does the work; the rest is dead weight that will be copied forward

disallow-doctype-decl=true alone fully closes XXE: no DOCTYPE ⇒ no internal/external DTD, no general or parameter entities, no external DTD fetch. The other four settings sit behind a control that already throws. Belt-and-suspenders is acceptable, but it matters here because this exact block is the template that will be pasted into the other parsers (see §3) — propagating both the redundancy and the bad setExpandEntityReferences line. Trim to the control that works plus the two external-entity fallbacks; setXIncludeAware(false) is already the default.

3. The vulnerability class is not remediated — this is whack-a-mole

The Semgrep ticket closes on the one path it flagged while the same sink stays open on the runtime paths that matter more:

• interactions.core/.../InputRequestAction.java:115,198, plus SelectionRequestAction and DataRequestAction — parse the VXML lastResult XML (attacker-influenceable at call time) through a bare DocumentBuilderFactory.newInstance(). Unhardened.
• WebServiceCallAction.java:159-162 already hardens inline — but with a different, divergent subset of flags. After this PR the codebase has two inconsistent copies of XXE hardening plus three unprotected runtime parsers.

The right shape is what the PR half-did: one hardened factory in XMLUtilities, with every runtime parser routed through it, and WebServiceCallAction's inline copy deleted. As submitted, the fix hardens a path (§scope: only Genesys) that isn't even among the higher-exposure ones, and leaves XXE reachable elsewhere. A finding closed, not a bug fixed.

(Design-time parsers in desktop/* that read developer-supplied project files are lower priority and can be excluded explicitly — but say so, don't leave it implicit.)

4. Behavior change is unverified and fails silently

disallow-doctype-decl=true makes any input containing <!DOCTYPE> throw SAXParseException. In GenesysVoicePlatform.processMetaDataResponse that is swallowed by catch (Exception e) { e.printStackTrace(); } → returns an empty map → silent attached-data loss at call time.

• The PR asserts nothing about whether real Genesys GetAttachedData payloads ever carry a DOCTYPE. The test-procedure block is empty. "Probably don't" is not evidence for a live runtime path.
• There is no way to distinguish a blocked attack from a broken parse — both land in the same swallowed printStackTrace. If you're hardening this method, attach an ErrorHandler, or have the caller log the rejection distinctly, so a legitimate-payload regression is detectable in production instead of surfacing as mysteriously missing attached data.

Related pre-existing smell on the same path (not introduced here, worth fixing while you're in the file): GenesysVoicePlatform.java:173 does System.out.println(attachedDataContent), logging raw untrusted input.

5. No testing is specified

The PR's Test Procedure section is left as an empty template — no manual steps, no evidence, nothing. For a security change that also alters parser behavior on a live runtime path, that's a gap on both fronts: it neither demonstrates the XXE vector is actually blocked nor that legitimate traffic still parses.

At minimum the PR should state how it was validated. Better, it should include:

• A representative Genesys GetAttachedData payload (the real <key name= value=> shape), pasted in the PR or committed as a fixture, so reviewers can confirm the assumption that production payloads carry no <!DOCTYPE>.
• A test around that payload exercising getDocumentBuilder() / the Genesys parse path with two cases: (a) a normal payload still parses to the expected key/value map, and (b) a DOCTYPE/external-entity payload is rejected rather than resolved. This is the direct, executable proof that the fix works and doesn't regress.

Note the repo has no unit-test suite today (mvn verify only compiles and assembles the p2 site), so even a small standalone parse test would be a net improvement — and if adding one is out of scope, the PR should at least document the manual verification and attach the sample payload.

---

Minimal changes to land this

1. Delete setExpandEntityReferences(false).
2. Keep disallow-doctype-decl + the two external-entity flags; drop the redundant remainder (or keep knowingly).
3. File/link a follow-up to route the three interactions.core action parsers and WebServiceCallAction through XMLUtilities, and note it in the PR so the Semgrep closure isn't read as class-wide remediation.
4. Verify a real Genesys payload still parses, and stop DOCTYPE rejections from dying in a silent catch.