feat: XML Validator prevent parsing XXE

[jkowalleck]

prevent the XmlValidation from XXE parsing - prevent XML external entity (XXE) injection
This is considered a security measure.

fixes #1061 as described here: #1063 (comment)

[jkowalleck]

@artola please review

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 309dd5a1	✅ 100.00% (target: 90.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (309dd5a)	Report Missing	Report Missing	Report Missing
Head commit (96ae065)	22779	22368	98.20%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1063)	2	2	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[jkowalleck]

https://research.jfrog.com/vulnerabilities/libxmljs2-namespaces-type-confusion-rce-jfsa-2024-001034098/#poc reports DOS via payload

const libxmljs2 = require('libxmljs2');

var d = `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE note
[
<!ENTITY writer PUBLIC "` + "A".repeat(8) + "B".repeat(8) + "C".repeat(8) + "D".repeat(8) + "P".repeat(8) + `" "JFrog Security">
]>
<from>&writer;</from>
`;

t = libxmljs2.parseXml(d)
from = t.get('//from')
c = from.childNodes()[0]
c2 = c.childNodes()[0] //entity_decl
n = c2.namespaces(true) //onlyLocal = true

tested, acknowledged.

to prevent this, the following parser options were used:

Object.freeze({
  nonet: true,
  compact: true,
  noent: false // prevent https://github.com/CycloneDX/cyclonedx-javascript-library/issues/1061
/* !!! do not set to true.
setting true would enable XXE.
the oposite is the intended solution
*/

})

see https://github.com/CycloneDX/cyclonedx-javascript-library/pull/1063/files#r1592446370

fixed code:

const libxmljs2 = require('libxmljs2');

var d = `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE note
[
<!ENTITY writer PUBLIC "` + "A".repeat(8) + "B".repeat(8) + "C".repeat(8) + "D".repeat(8) + "P".repeat(8) + `" "JFrog Security">
]>
<from>&writer;</from>
`;

var po = Object.freeze({
  nonet: true,
  compact: true,
  noent: false // prevent https://github.com/CycloneDX/cyclonedx-javascript-library/issues/1061
})

t = libxmljs2.parseXml(d, po)
from = t.get('//from')
c = from.childNodes()[0]
c2 = c.childNodes()[0] //entity_decl
n = c2.namespaces(true) //onlyLocal = true 

This code does not cause a DOS anymore, nor does it open any sockets or such...
Instead, it crashes, since the XXE is not parsable nor executed.
This behavior is acceptable.

I consider this a fix.
The vulnerability of #1061 is not usable anymore.

Therefore: fixes #1061

[artola]

Awesome!

[sven-rsb]

Great work @jkowalleck! I found this when I was researching on how to prevent the parsing vulnerabilities following up on CVE-2024-34393.
This looks great and I agree that the nonet and noent options should solve the vulnerabilities.
I was wondering the reason behind adding the compact option. Was it only for the performance improvements for your specific use case or can it also prevent some kind of vulnerability?
According to the official documentation, all it does is:

Compact small text nodes; no modification of the tree allowed afterwards (will possibly crash if you try to modify the tree)

[jkowalleck]

yes, the compact was added for my specific purposes, it is not related to the actual workaround for the thing.

LIBXML_COMPACT:
Activate small nodes allocation optimization. This may speed up your application without needing to change the code.
Note: Only available in Libxml >= 2.6.21

[jkowalleck]

FYI: this fix will be resulting an security advisory: GHSA-38gf-rh2w-gmj7

[jkowalleck]

!!! REVERTED !!!
had to revert this merge/patch.
And had to yank v6.7.0
!!! REVERTED !!!

instead of preventing XXE, it actually enabled it.

[jkowalleck]

I was wrong. This library was not affected by XXE in the first place.
see a hardening and POC/tests #1064

[prabhu]

noent: true means substitute entities.

Omitting this option is the secure default.

[jkowalleck]

exactly.
therefore GHSA-38gf-rh2w-gmj7 exists,
and the #1064 introduced test for this