Security Issue - CVE-2022-25645 - Node module dset

[ThanujV]

There is currently CVE open for the module dset for the following problem:

Affected versions of this package are vulnerable to Prototype Pollution via 'dset/merge' mode, as the dset function checks for prototype pollution by validating if the top-level path contains proto, constructor or protorype. By crafting a malicious object, it is possible to bypass this check and achieve prototype pollution.

This affect graphiql on the following line - which is affected as it uses dset/merge as the imported module:

dset(payload.data, path, data);

Would it be possible to migrate away from dset soon?

[andyedwardsibm]

Specifically, CVE-2022-25645 is described at https://security.snyk.io/vuln/SNYK-JS-DSET-2330881

[acao]

@ThanujV thanks, I should've addressed this sooner!

Note that in order to be vulnerable, you must be using @defer or @stream directives with http multipart IncrementalDelivery. These aren't even merged into graphql yet, and thus this feature is only available experimentally.

I will review with our security team to see if an advisory is needed.

@dotansimha noticing that url-loader is using dset as well, switching us to set-value which has the exact same signature, maybe you'll want to do the same?

[ThanujV]

I'll raise switching to set-value as an issue in our internal repos, thank you!

[acao]

Hmm, I don't know if it's a 1:1 parity of behavior though. The cypress suite caught some potential issues I didn't see with manual testing of the webpack build. Looks like deferred queries don't work with this approach. I hope to merge a fix tonight

https://github.com/graphql/graphiql/runs/5738853923?check_suite_focus=true#step:5:225

Update: I was wrong, i think set-value is just slower
Update again: it needs this one option: { merge: true }

[acao]

released a patch fix! 1.8.1

[yaacovCR]

I think you may need to provide a custom merge function as setting merge = true according to docs for set-value seems to only perform a shallow merge which is probably not enough.

I actually didn't check to see how setvalue works and how it's being used, but just thinking about overlapping deferred fragments, it seems that you would need deep merging.

Also, out of curiosity, why is set-value not vulnerable to the same concern? Is there no CVE for it yet or did they solve something differently?

Can dset be patched rather than discarded? Is there a link to separate discussion about that?

[n1ru4l]

@yaacovCR I created a PR for addressing this: lukeed/dset#34

[acao]

@yaacovCR we have a cypress test that proves this works, I think? You can always just modify the tests to prove it doesnt!

it actually failed until i added {merge: true}. Perhaps it's an issue with your streaming server, maybe check the paths? can you open a new issue with a repro if you still have the bug?

[lukeed]

This has been fixed in dset@3.1.2