[SECURITY] advisories/GHSA-mjr4-7xg5-pfvh for optional dependency libxmljs2

[artola]

https://github.com/CycloneDX/cyclonedx-javascript-library/pull/668/files#r1171603405

[jkowalleck]

seams to be fixed for a while. https://github.com/CycloneDX/cyclonedx-javascript-library/blob/309dd5ac4dbb96303759a1dcc083dd56dff3708d/package.json#L72C1-L72C44

[artola]

seams to be fixed for a while. https://github.com/CycloneDX/cyclonedx-javascript-library/blob/309dd5ac4dbb96303759a1dcc083dd56dff3708d/package.json#L72C1-L72C44

I do not think so, see:
https://github.com/marudor/libxmljs2/issues/210

See advisory: GHSA-mjr4-7xg5-pfvh

[jkowalleck]

you approach with no info, just drop a link to some outdated code from Apr 19, 2023
for an optional dependency.

• What is the observed behavior?
• What is the expected behavior?
• what is the problem you try to solve?
• What are use (edge-/none-)cases and (out-of-)scopes and?

[artola]

@jkowalleck is totally right. Perhaps this issue should be moved to the other @CycloneDX package, but since this is the initiator.

• About optional dependency:

  An optional dependency is installed by npm even if it's not listed under dependencies because it's listed under optionalDependencies.
  When you run npm install, npm will try to install all dependencies listed in both dependencies and optionalDependencies.
  The difference is that if an optional dependency fails to install, npm will not stop the installation process, whereas if a regular dependency fails to install, npm will throw an error and stop the installation.
  So, even though libxmljs2 is not listed under dependencies, it will still be installed because it's listed under optionalDependencies.

• What is the observed behavior?
  There is a vulnerability in libxmljs2 that is a transcend dependency of @cyclonedx/webpack-plugin.

   └─ @cyclonedx/webpack-plugin@npm:3.10.0 [6950a] (via npm:^3.8.2 [6950a])
      └─ @cyclonedx/cyclonedx-library@npm:6.6.0 (via npm:^6.5.0)
         └─ libxmljs2@npm:0.33.0 (via npm:^0.31 || ^0.32 || ^0.33)

├─ libxmljs2
│  ├─ ID: 1097224
│  ├─ Issue: libxmljs2 type confusion vulnerability when parsing specially crafted XML
│  ├─ URL: https://github.com/advisories/GHSA-mjr4-7xg5-pfvh
│  ├─ Severity: high
│  ├─ Vulnerable Versions: <=0.33.0
│  │ 
│  ├─ Tree Versions
│  │  └─ 0.33.0
│  │ 
│  └─ Dependents
│     └─ @cyclonedx/cyclonedx-library@npm:6.6.0
│
└─ libxmljs2
   ├─ ID: 1097230
   ├─ Issue: libxmljs vulnerable to type confusion when parsing specially crafted XML
   ├─ URL: https://github.com/advisories/GHSA-78h3-pg4x-j8cv
   ├─ Severity: high
   ├─ Vulnerable Versions: <=1.0.11
   │ 
   ├─ Tree Versions
   │  └─ 0.33.0
   │ 
   └─ Dependents
      └─ @cyclonedx/cyclonedx-library@npm:6.6.0

• What is the expected behavior?
  The expected behavior is for the package to have no known vulnerabilities.

• what is the problem you try to solve?
  The problem being addressed is the existence of vulnerabilities, which are detrimental for various reasons.

• What are use (edge-/none-)cases and (out-of-)scopes and?
  The package introduces a vulnerability for which we cannot estimate the impact. Additionally, the author has stated that they do not maintain the package nor possess the necessary expertise to address the issue.

[jkowalleck]

• What is the expected behavior?
  The expected behavior is for the package to have no known vulnerabilities.

to make this happen, we need an non-vulnerable release of libxmljs2 or use a replacement for the xml-purpose.
Will keep this open to track this.

[artola]

• What is the expected behavior?
  The expected behavior is for the package to have no known vulnerabilities.

to make this happen, we need an non-vulnerable release of libxmljs2 or use a replacement for the xml-purpose. Will keep this open to track this.

It seems that the author is not intended to work on this package. See for example in his comments:

• https://github.com/marudor/libxmljs2/issues/207#issuecomment-1983316873
• https://github.com/marudor/libxmljs2/issues/210#issuecomment-2096066889

Knowing that, it makes sense to think in a replacement of the package. May be:
https://www.npmjs.com/package/sax

[jkowalleck]

@artola could you craft any POC to showcase that the vulnerability affects this cyclonedx-javascript-library at all?
This will help with regression testing and justifying the efforts ...

[jkowalleck]

npm audit                                                                                                                                                        13.1s  2024-05-07 11:30:56
# npm audit report

libxmljs2  *
Severity: high
libxmljs2 type confusion vulnerability when parsing specially crafted XML - https://github.com/advisories/GHSA-mjr4-7xg5-pfvh
libxmljs vulnerable to type confusion when parsing specially crafted XML - https://github.com/advisories/GHSA-78h3-pg4x-j8cv

[jkowalleck]

the library libxmljs2 is an optional dependency, reasoned here: https://github.com/CycloneDX/cyclonedx-javascript-library/blob/main/docs/dev/decisions/XmlValidator.md

it is used only here:

cyclonedx-javascript-library/src/validation/xmlValidator.node.ts

Line 35 in 309dd5a

libxml = await import('libxmljs2')

current view on the problem:

• some company reported issues in a portion of libxmljs2.
• no peer review happened for this report
• no POC was done so far
• it is questionable, that the vulnerability is reachable from the code in cyclonedx-javascript-library
• possible false alert from uninformed actors

---

to be researched: is any of the vulnerable code actually run?
can a payload be crafted to show the DOS that is described in GHSA-mjr4-7xg5-pfvh & GHSA-78h3-pg4x-j8cv

---

results:

a XXE injection was possible in the current implementation of the Xml validator.
this could be fixed by a NOENT parser option.

[artola]

@jkowalleck The problem with "optional dependency" is that you can opt-out for all of them, there is no way to select only some of them.

I did try to introduce the malformed code using this example:
https://github.com/CycloneDX/cyclonedx-javascript-library/blob/main/examples/node/javascript/example.mjs

I am not able to do it, if I do now know a deep nested child that is not sanitized (May be hashes?).

Anyway, our concern is to have an alert saying that a high severity vulnerability could be introduced by this package and very difficult to determine the impact without knowing the usage.

[jkowalleck]

expected to be fixed via #1063 (comment)

[artola]

@jkowalleck was it a typo in this line? I mean it seems that if was intended to avoid entities.

cyclonedx-javascript-library/src/validation/xmlValidator.node.ts

Line 50 in 20144a9

nonet: true,

fixed in:

cyclonedx-javascript-library/src/validation/xmlValidator.node.ts

Line 52 in 20144a9

noent: true // prevent https://github.com/CycloneDX/cyclonedx-javascript-library/issues/1061

But in the docs it mentions as noent: https://github.com/marudor/libxmljs2/wiki

The optional second argument is an object in which you can set recover, noent, noblanks, and nocdata properties to true to set the corresponding libxml parsing flag. noblanks removes non-significant whitespace and is particularly useful when round-tripping XML content with indentation, since the presence of non-significant whitespace in a document turns off automatic formatting in toString.

[jkowalleck]

was it a typo in this line?

nope. this was not a typo.

nonet disables network access,
and noent disables XXE

[jkowalleck]

background: the nonet was intended to disable loading remote code (by XXE or xinclude.)
this would also disable exfiltrating data that would be collected by XXE.
but this would not prevent DOS or code execution by XXE.

the noent will take care of it, now.

https://github.com/marudor/libxmljs2/blob/9b4260760c338c1a393ab61c4ad14e8710c598b2/index.d.ts#L11

[jkowalleck]

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

[jkowalleck]

for the record, see also GHSA-38gf-rh2w-gmj7

[jkowalleck]

some final words:

---

the mentioned fundamental security issue in one of our dependencies actually is a provided feature, that, IF it was used wrong, COULD potentially cause issues.
it is asserted, that it is NOT used wrong. see

cyclonedx-javascript-library/tests/functional/internals/OpPlug.node.xmlValidator.implementation.spec.js

Lines 70 to 95 in 5d58e0f

	test('is not affected by XXE injection', async () => {
	// see https://github.com/CycloneDX/cyclonedx-javascript-library/issues/1061
	const xxeFile = join(__dirname, '..', '..', '_data', 'xxe_flag.txt')
	const input = `<?xml version="1.0" encoding="UTF-8"?>
	<!DOCTYPE poc [
	<!ENTITY flag SYSTEM "${pathToFileURL(realpathSync(xxeFile))}">
	]>
	<bom xmlns="http://cyclonedx.org/schema/bom/1.6">
	<components>
	<component type="library">
	<name>bar</name>
	<version>1.337</version>
	<licenses>
	<license>
	<id>&flag;</id>
	</license>
	</licenses>
	</component>
	</components>
	</bom>`
	const validationError = (await makeValidator(schemaPath))(input)
	assert.doesNotMatch(
	JSON.stringify(validationError),
	/vaiquia2zoo3Im8ro9zahNg5mohwipouka2xieweed6ahChei3doox2fek3ise0lmohju3loh5oDu7eigh3jaeR2aiph2Voo/,
	'must not leak secrets')
	})

---

the dependency in question might be replaced via #1079 -- feel free to contribute 🚀

[jkowalleck]

documentation of exploitability is planned to be published. see #1183