search

thlorenz / browserify-shim

📩 Makes CommonJS incompatible files browserifyable.
MIT License
933 stars 87 forks source link

CVE-2022-37617/ Prototype pollution found in resolve-shims.js #245

Closed secdevlpr26 closed 2 years ago

secdevlpr26 commented 2 years ago

Prototype pollution vulnerability in function resolveShims in resolve-shims.js in thlorenz browserify-shim 3.8.15 via the k variable in resolve-shims.js. The prototype pollution vulnerability can be mitigated with several best practices described here: https://learn.snyk.io/lessons/prototype-pollution/javascript/

bendrucker commented 2 years ago

https://nvd.nist.gov/vuln/detail/CVE-2022-37617

bendrucker commented 2 years ago

You can definitely overwrite the prototype for exposeGlobals:

https://github.com/thlorenz/browserify-shim/blob/bae7ece76cf76a5c0382468db324d61c1f533929/lib/resolve-shims.js#L130

But I'm not seeing a way that you could pollute the global scope, e.g., Object.prototype. Prototype pollution typically involves some form of recursion or at least two levels of property access so that you can write to the objects within a target object. Here there's only one level of property access, exposeGlobals[k].

Beyond that, I'm not even sure overwriting exposeGlobals is easily exploitable. Before it's returned, it gets passed to Object.keys, which will only return own properties:

https://github.com/thlorenz/browserify-shim/blob/bae7ece76cf76a5c0382468db324d61c1f533929/lib/resolve-shims.js#L99-L100

No methods are called so I'm not seeing a way to achieve code execution.

Would appreciate some clarification on whether there's a substantive vulnerability here or whether this was potentially vulnerable code identified through automated static analysis.

In any case, guarding against prototype assignment is easy enough.

secdevlpr26 commented 2 years ago

Hello,

The code has been flagged as a potentially vulnerable code and the CVE has the sink and the path details of the code.

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

thlorenz commented 2 years ago

This whole package depends on adding a property to the global since that is what certain packages expect. Thus it is only usd in scenarios where other packages already overwrite globals the exact same way. Additionally most of the code you keep flagging runs as part of the build step (not when the web app loads in the browser), but I'm not sure that your tools are aware of that.

In more modern scenarios where this is not desired/necessary browserify-shim is not used.

Please stop reporting these issues of the same family here. It is noise and adds overhead to the maintainers work.