[invalid CVE]: “Property Injection” in the function merge() CVE-2021-44907

[Marynk]

merge() (https://github.com/ljharb/qs/blob/main/dist/qs.js#L670) allows to assign properties on an array in the query. In case of any property being assigned a value the array is converted to an object containing these properties. Essentially, this means that the property whose expected type is Array always has to be checked with Array.isArray() by the user, which may not be obvious to the user and can cause unexpected behavior. While this seems intentional, this behavior is not stressed in documentation.

A couple of simple examples: https://jsfiddle.net/1s7pq93z/1/ https://jsfiddle.net/65jxksay/

The CVE Program has assigned the ID CVE-2021-44907 to this issue. This is a record on the CVE List, which standardizes names for security problems.

[ljharb]

I’m confused; why is there a CVE that wasn’t responsibly disclosed first? Filing security issues publicly is insanely irresponsible, and wildly insecure.

Someone who knows how to file a CVE should absolutely know how to follow a security policy.

[ljharb]

None of these seem like vulnerabilities, after a deeper look. Prototype pollution attacks are about polluting global prototypes, not about polluting a single instance.

You're totally right that someone who is writing bad code won’t be properly checking for an array, and that someone writing non-robust code won’t be doing Array.prototype.map.call, but at best this is a usability improvement, not a security vulnerability.

We could certainly warn when an own property being set on an array is shadowing an Array.prototype method - that seems reasonable to me - but either way, i don’t consider any of these to be CVE-worthy. Most bugs aren’t vulnerabilities, they’re just bugs.

[Cicko]

@ljharb Any suggestion how can we proceed with this issue?

[ljharb]

@Cicko the CVE needs to be invalidated, is the first thing, since it's not a vulnerability.

Since the only thing we could maybe do is console.warn, and, since doing that would potentially be a breaking change, I'm not sure it's worth doing anything.

[ljharb]

In other words, I might be willing to accept an option that, when enabled, throws when a builtin would be shadowed on an object or an array - but it couldn't be on by default.

Short of that, this can be closed, and so I'll do that.

[justinbhopper]

Whose responsibility is it to invalidate the CVE? Has any progress been made on invalidating it?

[ljharb]

@justinbhopper ideally the reporter (but mitre shouldn’t have approved it in the first place, ofc). I have no idea who that is.

[justinbhopper]

I've submitted a Rejection request to the following form: https://cveform.mitre.org/

[samandar-boymurodov]

Hello there,

@ljharb, has the rejection request been approved? I see that the CVE with id CVE-2021-44907 still exists.

[ljharb]

@samandar-boymurodov then it seems pretty obvious that it hasn’t been yet

[huineng]

Can this ticket be reopened until solved ? CVE-2021-44907 is being reported as "high"

My problem it's being used in "request" and that project is deprecated and there are many libraries still using request so the only solution is to have this rejected (imo)

[ljharb]

@huineng what would be the point? When a CVE is a false positive (as almost every CVE in the JS ecosystem is), you have to configure your system to ignore it.

That this project is used in a popular deprecated project would never be a reason not to accept a valid CVE. This one is just nonsense.

[samandar-boymurodov]

Can this ticket be reopened until solved ? CVE-2021-44907 is being reported as "high"

My problem it's being used in "request" and that project is deprecated and there are many libraries still using request so the only solution is to have this rejected (imo)

Can this ticket be reopened until solved ? CVE-2021-44907 is being reported as "high"

My problem it's being used in "request" and that project is deprecated and there are many libraries still using request so the only solution is to have this rejected (imo)

I have the same situation too. Security scanning tools are reporting about this CVE .

[ljharb]

Please inform these tools the CVE is invalid, and please configure your tools to ignore this one. There's nothing else I can do about it from here.

[huineng]

Thanks for your answer , i do belief you of course when you say

I’m confused; why is there a CVE that wasn’t responsibly disclosed first?

problem is of course the fact this this cve is logged https://nvd.nist.gov/vuln/detail/CVE-2021-44907 and thus picked op by many (not all) scanning tools, for example we are using "whitesource advice" and "dependency check" just to name 2

And for each i would need to give a reason why it would be a false positive. And thus i'm afraid the trap we/you are in, is that (sorry for this) it might not be enough for the owner of a github repo to say something is a false positive.

I have no clue how this was even identified and most of the evidences https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-44907 are things we're just discussing here. but just google this cve and you get already 288 returns (of course all of them repeating the same thing not bringing any clarity to the origin of the problem) thanks

[ljharb]

@huineng the reason you can give is my comment above: https://github.com/ljharb/qs/issues/436#issuecomment-1067538394

plus, i know many folks at these companies personally. Happy to discuss with anyone who thinks it's a real issue; either I'll convince them, or they'll convince me and I'll "fix" qs (which i clearly think is unlikely).

[ljharb]

I'm going to reopen this, for no reason except to decrease the likelihood of duplicate issues being filed.

[justinbhopper]

Just an update on this - I have received nothing more from MITRE other than the automated confirmation that they've received my rejection request and will review it. Honestly, I don't know how timely of a response to expect, or whether to expect one at all...

[liamcmitchell-sc]

...merge() (...) allows to assign properties on an array in the query. In case of any property being assigned a value the array is converted to an object containing these properties. Essentially, this means that the property whose expected type is Array always has to be checked with Array.isArray() by the user, which may not be obvious to the user and can cause unexpected behavior. While this seems intentional, this behavior is not stressed in documentation.

This is absolutely expected and documented behaviour.

From the README:

https://github.com/ljharb/qs/blob/542a5c7ff88d7229efa2e22c7c8a7d69375f5e72/README.md?plain=1#L270-L275

A couple of simple examples: https://jsfiddle.net/1s7pq93z/1/ https://jsfiddle.net/65jxksay/

You don't even need the above examples to demonstrate that parsed values may not be an array:

qs.parse('arr=a&arr=b').arr; // ['a', 'b'] - array, great! 👍🏻
qs.parse('arr=a').arr;       // 'a'        - not an array! 😱
qs.parse('arr[a]=a').arr;    // {a: 'a'}   - not an array! 😨
qs.parse('').arr;            // undefined  - not an array! 🤦🏼‍♂️

I don't think this library ever guaranteed returning arrays. Why would this suddenly become a security vulnerability?!

@Marynk (or anyone with the power), please close this CVE at the source and save 1000s of other developers the pain of even looking at this issue.

Side note: the second example above doesn't fail because the expected arr array is converted to an object, it fails because it is calling .map() on the returned query object.arr isn't even referenced.

const parsed = Qs.parse("arr=a&arr=b&arr=c&arr.map=true", {allowDots: true});
const edited = parsed.map(el => el+"test"); // Should be parsed.arr.map(...) 

[justinbhopper]

Another update - I have tried several times to get a response from MITRE and nothing has come back. I'm not sure what to do at this point.

[JaxonWright]

Seems weird to me that a CVE can just be reported like this without disclosing it to the developer first, and now there's no clear way to declare it as a false positive. Now millions of people have to land here to discover it isn't an issue, wasting a collective bucket of hours.

[justinbhopper]

Update - just got a reply from my CVE denial request and it looks like they approved the denial. The CVE status should be updated within a few hours. I'm not sure how long it takes GitHub's advisory database to update its status, but I imagine within 24 hours or so.

[justinbhopper]

It's been 24 hours and MITRE does seem to have updated their information. However, it seems Github's Advisory Database hasn't picked up the changes.

I'm not sure if we're supposed to wait for some automatic update or if we're supposed to submit a PR to the advisory database repo via this form: https://github.com/advisories/GHSA-rq7c-8f3g-q36h/improve

If anyone stumbles across here that is familiar with the process, please provide your insights!

[ljharb]

I've submitted https://github.com/github/advisory-database/pull/193.

[ljharb]

Looks like this should be resolved.

Please let me know if any tooling is complaining about this invalid CVE.

[notifications-for-me]

Please let me know if any tooling is complaining about this invalid CVE.

@ljharb WhiteSource is still complaining during SAST scan: https://www.whitesourcesoftware.com/vulnerability-database/CVE-2021-44907

[ljharb]

@notifications-for-me i've reached out to some whitesource folks; hopefully it resolves itself.

[CrossEye]

Someone just pointed this thread out to me as Ramda goes through the same frustrating process. The issue was opened by the same user who started this one. I can't tell from a glance through this whether or not qs is actually subject to the reported problem. Ramda isn't. It took a long battle to get Veracode to withdraw this, and now we have to fight Mitre too. I never even got an automated acknowledgement of my rejection request when I sent it more than two weeks ago.

I'm glad we're not alone. But there's something very, very wrong with the process here.