Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[invalid] Prototype pollution found in dom.js #436

Closed
secdevlpr26 opened this issue Oct 10, 2022 · 29 comments · Fixed by #441
Closed

[invalid] Prototype pollution found in dom.js #436

secdevlpr26 opened this issue Oct 10, 2022 · 29 comments · Fixed by #441
Assignees
Labels
invalid This doesn't seem right
Milestone

Comments

@secdevlpr26
Copy link

Prototype pollution vulnerability in function copy in dom.js in xmldom xmldom 0.6.0 via the p variable in dom.js.

The prototype pollution vulnerability can be mitigated with several best practices described here: https://learn.snyk.io/lessons/prototype-pollution/javascript/

@karfau
Copy link
Member

karfau commented Oct 11, 2022

If you find a vulnaribility, please follow the process documented in https://github.com/xmldom/xmldom/blob/master/SECURITY.md to disclose it properly.

I'm not able to find any information related to the provided CVE id other then it has been reserved.

v0.6.0 contains the following vulnerabily, which has been fixed in the more recent version 0.7.0 s of the library:

the latest release of the library that has been published is 0.8.2 which fixes several other issues.
Due to #271 the package can no longer be published under xmldom but is published under @xmldom/xmldom since 0.7.0.

Please check if the issue you are reporting is also present in a more recent version.

Closing this issue for now, feel free to provide more information in this issue or follow the steps described in https://github.com/xmldom/xmldom/blob/master/SECURITY.md .

@karfau karfau closed this as completed Oct 11, 2022
karfau added a commit that referenced this issue Oct 11, 2022
by no longer using the `in` operator.

#436
karfau added a commit that referenced this issue Oct 11, 2022
by adding `hasOwnProperty` checks.

#436
karfau added a commit that referenced this issue Oct 11, 2022
karfau added a commit that referenced this issue Oct 11, 2022
by adding `hasOwnProperty` checks.

#436
@karfau karfau self-assigned this Oct 11, 2022
@karfau karfau added bug Something isn't working Security labels Oct 11, 2022
@karfau karfau added this to the 0.9.0 milestone Oct 11, 2022
@karfau
Copy link
Member

karfau commented Oct 11, 2022

A fix has been published as 0.8.3 (latest), 0.7.6 and 0.9.0-beta.2 (next)

The security advisory has been published: GHSA-9pgh-qqpf-7wqj

@jftanner
Copy link

jftanner commented Oct 12, 2022

Hi @karfau. The xml-encryption and xml-crypto (issue) are using ^0.7.0 for this package. Given their last update times, a new version of those packages may not be immediately forthcoming. So a fix on 0.7.x would be greatly appreciated in the meantime!

I've created a PR that I believe properly backports your fix: #441

jftanner added a commit to jftanner/xmldom that referenced this issue Oct 12, 2022
@karfau
Copy link
Member

karfau commented Oct 12, 2022

@jftanner thanks a lot!
I'm away from my computer with which I would be able to do a release until including Sunday the 16th of October.
But you can be sure this will be the first thing i take care of when I'm back!

@karfau karfau linked a pull request Oct 12, 2022 that will close this issue
@jftanner
Copy link

Very happy to be of assistance!

@karfau
Copy link
Member

karfau commented Oct 16, 2022

version 0.7.6 is available

@jftanner
Copy link

Thanks @karfau!

@RyanCommits
Copy link

Looks like the problem still exists in 0.7.6
https://security.snyk.io/package/npm/@xmldom%2Fxmldom/0.7.6

@jftanner
Copy link

jftanner commented Oct 17, 2022

I think that's a false positive. Snyk doesn't know that 0.7.6 was a fix.

Looks like the CVE is being re-analyzed. Hopefully they'll pick up the fixed version. https://nvd.nist.gov/vuln/detail/CVE-2022-37616

@karfau
Copy link
Member

karfau commented Oct 17, 2022

Yes, there is a pnding PR to update the information on github side: github/advisory-database#747

@jupenur
Copy link

jupenur commented Nov 1, 2022

@karfau I can't for the life of me figure out how #437 fixes a security issue. I see that the lack of hasOwnProperty is functionally somewhat incorrect, but not how that could be exploited in any way. Can you share any details? Is there a chance that this could actually be non-exploitable and a false positive to begin with?

Feel free to reach out to me in private if you don't want to comment in public.

@jupenur
Copy link

jupenur commented Nov 1, 2022

@karfau looks like @Supraja9726 has been routinely opening issues like this in a bunch of repositories. Many of them have been closed as false positives by maintainers. Can you please confirm if you've actually been able to verify the security impact here?

@karfau
Copy link
Member

karfau commented Nov 1, 2022

Thx, @jupenur for having a look.

I did not invest any time into trying to verify it (which is also not one of the things I know how to thoroughly do). Since it was announced in public I figured I find all potential cases like that in the code base and the missing checks to prevent all similar kind of issues, and publish a new version.
There was also never any other response from @Supraja9726 after the initial report.

@jdgregson
Copy link

jdgregson commented Nov 8, 2022

After finding this same pattern in an app I am reviewing I set out to write exploit code and verify that the app was vulnerable. This pattern of dest[attr] = src[attr] is flagged for prototype pollution in many repositories. However, after several days I have been unable to reproduce the issue. I put together some test code to test for prototype pollution in the xmldom copy function, as well as the merge function from lodash which was supposedly vulnerable as well.

After running every payload and PoC I can find through these two functions forwards and backwards, I have not been able to pollute the prototype:

function xmldomCopy(src, dest) {
  for (var p in src) {
    dest[p] = src[p];
  }
}

function lodashMerge(target, source) {
  for (let attr in source) {
    if (typeof(target[attr]) === 'object' && typeof(source[attr]) === 'object') {
      lodashMerge(target[attr], source[attr]);
    } else {
      target[attr] = source[attr];
    }
  }
  return target;
}

const payloads = [
  {foo: 'bar', test: {'__proto__': {'polluted': true}}},
  {foo: 'bar', test: {'__proto__.polluted': true}},
  {foo: 'bar', test: {'__proto__[polluted]': true}},
  {foo: 'bar', __proto__: {'polluted': true}},
  {foo: 'bar', '__proto__': {'polluted': true}},
  {foo: 'bar', '__proto__': {'polluted': 'true'}},
  {foo: 'bar', '__proto__.__proto__': {'polluted': true}},
  {foo: 'bar', '__proto__.polluted': true},
  {foo: 'bar', '__proto__[polluted]': true},
  {foo: 'bar', '__proto__[polluted]=polluted': true},
  {foo: 'bar', '{}.__proto__.polluted': true},
  {foo: 'bar', 'test': {'prototype': {'polluted': true}}},
  {foo: 'bar', 'test': {'proto[polluted]': {'polluted': true}}},
  {foo: 'bar', 'proto[polluted]': true},
  JSON.parse('{"constructor": {"prototype": {"polluted": true}}}'),
]

payloads.forEach((payload) => {
  xmldomCopy(payload, {});
  xmldomCopy({}, payload);
  lodashMerge({}, payload);
  lodashMerge(payload, {});
});

console.log('Polluted: ' + (new Object).polluted); // Polluted: undefined

If anyone, including and especially the reporter could provide a working PoC affecting statements such as dest[attr] = src[attr] I would greatly appreciate it. The only cases I have found where dest[attr] = src[attr] leads to prototype pollution is when dest is pointing to __proto__, such as when dest = dest[attr] was executed while attr was '__proto__'. This is what happened in the flat module's unflatten prototype pollution case, but is not what is happening here.

@karfau
Copy link
Member

karfau commented Nov 8, 2022

Thank you @jdgregson fro investing the time and focus.
Do you suggest that I update the score of the advisory to indicate that there is no known exploit?
(Update: Not sure that question makes any sense, I didn't find any related score as part of the CVSS guide.)

@karfau karfau removed the Security label Nov 8, 2022
@karfau karfau changed the title CVE-2022-37616/ Prototype pollution found in dom.js [invalid] Prototype pollution found in dom.js Nov 8, 2022
@karfau karfau removed the bug Something isn't working label Nov 8, 2022
@jdgregson
Copy link

There is another answer to a report from this person showing that dest[attr] = src[attr] will not lead to prototype pollution.

I might go through and challenge each of this person's CVEs when I have some time. This behavior is irresponsible and harmful.

@secdevlpr26
Copy link
Author

Hello,
Sorry for all the inconvenience caused. All the reports are based on the research work of my colleague (you can find her paper's link below) and I am reporting them here as per her analysis and records.

https://dl.acm.org/doi/pdf/10.1145/3488932.3497769 - This is the published paper with the Github link to her static analysis tool.
Thanks

@jdgregson
Copy link

Hi @Supraja9726, thanks for following up. I would be happy to read the paper and try to understand if there is a vulnerability we're missing or if this static analysis tool is finding false positives. However, the article appears to be locked behind a paywall. Is this article published publicly somewhere?

@jupenur
Copy link

jupenur commented Nov 9, 2022

@jdgregson the PDF is available here, via Google Scholar: http://users.encs.concordia.ca/~mmannan/publications/JS-vulnerability-aisaccs2022.pdf

@secdevlpr26
Copy link
Author

Hello,

Please refer the below code:

function xmldomCopy(src, dest) {
  for (var p in src) {
    dest[p] = src[p];
  }
  return dest;
}

var payload = JSON.parse('{"__proto__": {"polluted": true}}')
var new_obj = {}
var new_obj = xmldomCopy(payload,new_obj);
console.log(new_obj.polluted)

Here, only the target object is polluted (and in several other repositories). A prototype injection/Prototype pollution is not just when global objects are polluted with recursive merge or deep cloning but also when a target object is polluted. This is still harmful in places where other objects might make use of the polluted ones. This leads to Remote code execution, DOS when affecting the existing functions like toString valueOf, etc.

This can be avoided by placing the necessary preventive measures while copying or cloning the objects.
I thereby conclude that this is not a false report and advise you to kindly look into the issue.

Thanks

@karfau
Copy link
Member

karfau commented Nov 18, 2022

@Supraja9726

  1. I don't think it is enough to copy some code from the repository to show that it can be misused. To prove the vulnerability this would need to be done by using the library as is, and show how to achieve what you say is possible. All the cases I found where there was a for in loop without checking the keys were in code that was not exported from the modules and was never iterating over any object created outside of this library.

  2. The discussion is not about whether to "fix" it (this already took place weeks ago), but whether this can actually be exploited when using the library.

@karfau
Copy link
Member

karfau commented Nov 20, 2022

FYI: The request to mark the CVE as invalid led to the following response:

Regarding your CVE service request, logged on 2022-11-08T14:09:33, we have the following question or update:

After review we have marked the CVE record as disputed and have updated the record. Thank you for participating in the CVE Program. This request will now be closed.

Please do not hesitate to contact the CVE Team by replying to this email if you have any questions, or to provide more details.

@jdgregson
Copy link

jdgregson commented Nov 25, 2022

Thanks for the reply, @Supraja9726. I've given it some thought and have to reverse course. I agree that there is a security issue here, and it is not addressed by the PR that attempted to fix it.

My main misunderstanding was that prototype pollution always means that the global Object prototype was polluted, and as @Supraja9726 pointed out here, there can be security impact if only one object is polluted via the __proto__ attribute.

To demonstrate the issue, let's suppose a developer wants to allow users to update their profiles. To do this, they merge data sent by the user with the data they currently have stored. The developer is careful not to let users set sensitive attributes on their user object, and throws an exception if this is attempted:

function xmldomCopyFixed(src, dest) {
  for (var p in src) {
    if (Object.prototype.hasOwnProperty.call(src, p)) {
      dest[p] = src[p];
    }
  }
}

function copyUserData(newUserData, storedUserData) {
  const forbiddenKeys = ['isAdmin', 'isMod', 'hashedPassword'];
  Object.keys(newUserData).forEach((key) => {
    if (forbiddenKeys.indexOf(key) > -1) {
      throw new Error(`Attempted to set forbidden key ${key}`);
    }
  });
  xmldomCopyFixed(newUserData, storedUserData);
  return storedUserData;
}

When a user attempts to explicitly set isAdmin to true, their request is rejected as designed:

const userDataFromDatabase = {firstName: "John", lastName: "Doe", email: "johndoe231@example.com"};
const userDataFromPost = JSON.parse(`{"firstName": "Jon", "isAdmin": true}`);
const updatedUserData = copyUserData(userDataFromPost, userDataFromDatabase);
// Uncaught Error: Attempted to set forbidden key isAdmin

However, if the user instead uses "__proto__" to set the isAdmin key, the request is allowed, and the resulting merged object has been polluted:

const userDataFromDatabase2 = {firstName: "John", lastName: "Doe", email: "johndoe231@example.com"};
const userDataFromPost2 = JSON.parse(`{"firstName": "Jon", "__proto__": {"isAdmin": true}}`);
const updatedUserData2 = copyUserData(userDataFromPost2, userDataFromDatabase2);
console.log(updatedUserData2.isAdmin);
// true

This can lead to significant security impact in applications that use the copy function of xmldom to copy and merge objects which are sensitive and trusted. That said, I don't necessarily agree with the severity of the issue. Seems more like a high severity issue rather than critical to me.

@jupenur
Copy link

jupenur commented Nov 25, 2022

This can lead to significant security impact in applications that use the copy function of xmldom to copy and merge objects which are sensitive and trusted.

How would an application use the copy function of xmldom for anything other than the very specific internal use-cases of the library itself? The copy function is not exported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants