On 4/22/23 13:14, Sebastian Ramacher wrote:
Control: tags -1 moreinfo
On 2023-04-21 11:16:32 +0400, Yadd wrote:
Package: release.debian.org
Severity: normal
User: release.debian....@packages.debian.org
Usertags: unblock
X-Debbugs-Cc: node-xml...@packages.debian.org
Control: affects -1 + src:node-xml2js
Please unblock package node-xml2js
This upload is causing autopkgtest regressions:
node-xml2js (0.4.23+~cs15.4.0+dfsg-4 to 0.4.23+~cs15.4.0+dfsg-5)
Maintainer: Debian Javascript Maintainers
Migration status for node-xml2js (0.4.23+~cs15.4.0+dfsg-4 to
0.4.23+~cs15.4.0+dfsg-5): BLOCKED: Rejected/violates migration
policy/introduces a regression
Issues preventing migration:
∙ ∙ autopkgtest for node-node-rest-client/3.1.1-2: amd64: Regression ♻
(reference ♻), arm64: Regression ♻ (reference ♻), armel: Regression ♻
(reference ♻), armhf: Regression ♻ (reference ♻), i386: Regression ♻
(reference ♻), ppc64el: Regression ♻ (reference ♻), s390x: Regression ♻
(reference ♻)
∙ ∙ autopkgtest for node-xml2js/0.4.23+~cs15.4.0+dfsg-5: amd64: Pass,
arm64: Pass, armel: Pass, armhf: Pass, i386: Pass, ppc64el: Pass, s390x: Pass
∙ ∙ blocked by freeze: is a key package (Follow the freeze policy when
applying for an unblock)
∙ ∙ Too young, only 1 of 20 days old
Additional info:
∙ ∙ Piuparts tested OK -
https://piuparts.debian.org/sid/source/n/node-xml2js.html
Please let us know once htey have been fixed.
Hi,
I just pushed node-xml2js 0.4.23+~cs15.4.0+dfsg-8.
In this new debdiff, instead of replacing `{}` by `Object.create(null)`,
I filter the forbidden __proto__ key.
A new autopkgtest proves that CVE is fixed and node-node-rest-client
test pass now
The explanation of this change is here:
https://github.com/Leonidas-from-XIV/node-xml2js/issues/672
Cheers,
Yadd
diff --git a/debian/changelog b/debian/changelog
index 98492d7..be97d0c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,33 @@
+node-xml2js (0.4.23+~cs15.4.0+dfsg-8) unstable; urgency=medium
+
+ * Team upload
+ * Fix regression in node-node-rest-client tests
+
+ -- Yadd <y...@debian.org> Tue, 25 Apr 2023 17:53:28 +0400
+
+node-xml2js (0.4.23+~cs15.4.0+dfsg-7) unstable; urgency=medium
+
+ * Team upload
+ * Better fix for CVE-2023-0842
+
+ -- Yadd <y...@debian.org> Tue, 25 Apr 2023 15:48:55 +0400
+
+node-xml2js (0.4.23+~cs15.4.0+dfsg-6) unstable; urgency=medium
+
+ * Team upload
+ * Fix regression in node-node-rest-client tests
+
+ -- Yadd <y...@debian.org> Tue, 25 Apr 2023 13:51:05 +0400
+
+node-xml2js (0.4.23+~cs15.4.0+dfsg-5) unstable; urgency=medium
+
+ * Team upload
+ * Update standards version to 4.6.2, no changes needed.
+ * Update nodejs dependency to nodejs:any
+ * Add patch to prevent prototype pollution (Closes: #1034148, CVE-2023-0842)
+
+ -- Yadd <y...@debian.org> Fri, 21 Apr 2023 11:11:13 +0400
+
node-xml2js (0.4.23+~cs15.4.0+dfsg-4) unstable; urgency=medium
* Team upload
diff --git a/debian/control b/debian/control
index dc4d6d0..406a88d 100644
--- a/debian/control
+++ b/debian/control
@@ -10,7 +10,7 @@ Build-Depends:
, node-sax <!nocheck>
, dh-sequence-nodejs
, node-diff
-Standards-Version: 4.6.1
+Standards-Version: 4.6.2
Vcs-Browser: https://salsa.debian.org/js-team/node-xml2js
Vcs-Git: https://salsa.debian.org/js-team/node-xml2js.git
Homepage: https://github.com/Leonidas-from-XIV/node-xml2js
@@ -21,8 +21,8 @@ Architecture: all
Depends:
${misc:Depends}
, node-sax
- , nodejs
, node-diff
+ , nodejs:any
Provides: ${nodejs:Provides}
Description: simple XML to JavaScript object converter - Node.js module
xml2js parses XML using node-sax and converts it to a plain JavaScript
diff --git a/debian/patches/CVE-2023-0842.patch
b/debian/patches/CVE-2023-0842.patch
new file mode 100644
index 0000000..6af0bd7
--- /dev/null
+++ b/debian/patches/CVE-2023-0842.patch
@@ -0,0 +1,114 @@
+Description: use Object.create(null) to create all parsed objects
+ (prevent prototype replacement)
+Author: James Crosby <ja...@coggle.it>
+Origin: upstream, commit:581b19a6
+Bug: https://github.com/advisories/GHSA-776f-qx25-q3cc
+Bug-Debian: https://bugs.debian.org/1034148
+Forwarded: not-needed
+Applied-Upstream: 0.5.0, commit:581b19a6
+Reviewed-By: Yadd <y...@debian.org>
+Last-Update: 2023-04-21
+
+--- a/src/parser.coffee
++++ b/src/parser.coffee
+@@ -107,14 +107,15 @@
+ obj[charkey] = ""
+ unless @options.ignoreAttrs
+ for own key of node.attributes
+- if attrkey not of obj and not @options.mergeAttrs
+- obj[attrkey] = {}
++ if attrkey not of obj and attrkey != '__proto__' and not
@options.mergeAttrs
++ obj[attrkey] = Object.create(null)
+ newValue = if @options.attrValueProcessors then
processItem(@options.attrValueProcessors, node.attributes[key], key) else
node.attributes[key]
+ processedKey = if @options.attrNameProcessors then
processItem(@options.attrNameProcessors, key) else key
+- if @options.mergeAttrs
+- @assignOrPush obj, processedKey, newValue
+- else
+- obj[attrkey][processedKey] = newValue
++ if processedKey != '__proto__'
++ if @options.mergeAttrs
++ @assignOrPush obj, processedKey, newValue
++ else
++ obj[attrkey][processedKey] = newValue
+
+ # need a place to store the node name
+ obj["#name"] = if @options.tagNameProcessors then
processItem(@options.tagNameProcessors, node.name) else node.name
+@@ -161,7 +162,7 @@
+ # put children into <childkey> property and unfold chars if necessary
+ if @options.explicitChildren and not @options.mergeAttrs and typeof obj
is 'object'
+ if not @options.preserveChildrenOrder
+- node = {}
++ node = Object.create(null)
+ # separate attributes
+ if @options.attrkey of obj
+ node[@options.attrkey] = obj[@options.attrkey]
+@@ -181,7 +182,8 @@
+ # push a clone so that the node in the children array can receive
the #name property while the original obj can do without it
+ objClone = {}
+ for own key of obj
+- objClone[key] = obj[key]
++ if key != '__proto__'
++ objClone[key] = obj[key]
+ s[@options.childkey].push objClone
+ delete obj["#name"]
+ # re-check whether we can collapse the node now to just the charkey
value
+@@ -197,7 +199,10 @@
+ # avoid circular references
+ old = obj
+ obj = {}
+- obj[nodeName] = old
++ if nodeName == '__proto__'
++ console.error 'Node name __proto__ forbibben'
++ else
++ obj[nodeName] = old
+
+ @resultObject = obj
+ # parsing has ended, mark that so we won't throw exceptions from
+--- a/test/parser.test.coffee
++++ b/test/parser.test.coffee
+@@ -531,13 +531,13 @@
+
+ 'test single attrNameProcessors': skeleton(attrNameProcessors:
[nameToUpperCase], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+- equ r.sample.attrNameProcessTest[0].$.hasOwnProperty('CAMELCASEATTR'),
true
+- equ r.sample.attrNameProcessTest[0].$.hasOwnProperty('LOWERCASEATTR'),
true)
++ equ {}.hasOwnProperty.call(r.sample.attrNameProcessTest[0].$,
'CAMELCASEATTR'), true
++ equ {}.hasOwnProperty.call(r.sample.attrNameProcessTest[0].$,
'LOWERCASEATTR'), true)
+
+ 'test multiple attrNameProcessors': skeleton(attrNameProcessors:
[nameToUpperCase, nameCutoff], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+- equ r.sample.attrNameProcessTest[0].$.hasOwnProperty('CAME'), true
+- equ r.sample.attrNameProcessTest[0].$.hasOwnProperty('LOWE'), true)
++ equ {}.hasOwnProperty.call(r.sample.attrNameProcessTest[0].$, 'CAME'),
true
++ equ {}.hasOwnProperty.call(r.sample.attrNameProcessTest[0].$, 'LOWE'),
true)
+
+ 'test single attrValueProcessors': skeleton(attrValueProcessors:
[nameToUpperCase], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+@@ -559,21 +559,21 @@
+
+ 'test single tagNameProcessors': skeleton(tagNameProcessors:
[nameToUpperCase], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+- equ r.hasOwnProperty('SAMPLE'), true
+- equ r.SAMPLE.hasOwnProperty('TAGNAMEPROCESSTEST'), true)
++ equ {}.hasOwnProperty.call(r, 'SAMPLE'), true
++ equ {}.hasOwnProperty.call(r.SAMPLE, 'TAGNAMEPROCESSTEST'), true)
+
+ 'test single tagNameProcessors in simple callback': (test) ->
+ fs.readFile fileName, (err, data) ->
+ xml2js.parseString data, tagNameProcessors: [nameToUpperCase], (err,
r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+- equ r.hasOwnProperty('SAMPLE'), true
+- equ r.SAMPLE.hasOwnProperty('TAGNAMEPROCESSTEST'), true
++ equ {}.hasOwnProperty.call(r, 'SAMPLE'), true
++ equ {}.hasOwnProperty.call(r.SAMPLE, 'TAGNAMEPROCESSTEST'), true
+ test.finish()
+
+ 'test multiple tagNameProcessors': skeleton(tagNameProcessors:
[nameToUpperCase, nameCutoff], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
+- equ r.hasOwnProperty('SAMP'), true
+- equ r.SAMP.hasOwnProperty('TAGN'), true)
++ equ {}.hasOwnProperty.call(r, 'SAMP'), true
++ equ {}.hasOwnProperty.call(r.SAMP, 'TAGN'), true)
+
+ 'test attrValueProcessors key param': skeleton(attrValueProcessors:
[replaceValueByName], (r)->
+ console.log 'Result object: ' + util.inspect r, false, 10
diff --git a/debian/patches/series b/debian/patches/series
index 2840ff2..c9bf5bb 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
fix-for-coffeescript-2.patch
drop-test-not-compatible-with-coffe-2.patch
+CVE-2023-0842.patch
diff --git a/debian/tests/CVE-2023-0842 b/debian/tests/CVE-2023-0842
new file mode 100755
index 0000000..fee8706
--- /dev/null
+++ b/debian/tests/CVE-2023-0842
@@ -0,0 +1,17 @@
+#!/usr/bin/node
+var parseString = require('xml2js').parseString;
+
+let normal_user_request = "<role>admin</role>";
+let malicious_user_request = "<__proto__><role>admin</role></__proto__>";
+
+const update_user = (userProp) => {
+ // A user cannot alter his role. This way we prevent privilege escalations.
+ parseString(userProp, function (err, user) {
+ if (userProp === malicious_user_request && user?.role && user.role[0])
{
+ throw new Error('Vulnerable to CVE-2023-0842');
+ }
+ });
+}
+
+update_user(normal_user_request);
+update_user(malicious_user_request);
diff --git a/debian/tests/control b/debian/tests/control
new file mode 100644
index 0000000..d45b8e8
--- /dev/null
+++ b/debian/tests/control
@@ -0,0 +1,3 @@
+Tests: CVE-2023-0842
+Depends: @
+Restrictions: allow-stderr
