1566036 - Update vulnerable lodash dependency in browser/components/newtab

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Christiane Ruetten [INACTIVE]]

The lodash team released a security update for a critical prototype pollution vulnerability that can lead to remote code execution.

• https://github.com/lodash/lodash/issues/4348
• https://github.com/lodash/lodash/pull/4336
• https://github.com/lodash/lodash/pull/4355

Only the latest release of lodash is not vulnerable. Please update dependencies to the latest release version, or at least verify that our code does not expose the vulnerability to untrusted input. (If not, I'd advise to update anyway.)

The non-vulnerable release versions are:

• lodash 4.17.14
• lodash.merge 4.6.2
• lodash.template 4.5.0

Any older releases contain the vulnerable code.

I'm assuming that this lodash code is actually shipping. Please report back here if it is not.

• List of files referencing lodash

Comment 1

[Kate Hudson :k88hudson]

Attachment: GitHub Pull Request

Comment 2

[Scott [:thecount] Downe]

Fixing in here: https://github.com/mozilla/activity-stream/pull/5182

You are correct in that yes, the vulnerable lodash code is actually shipping.

With the fix in the above pr, the fixed lodash is shipped, but the values in https://searchfox.org/mozilla-central/source/browser/components/newtab/package-lock.json still wouldn't reflect that.

Comment 3

[Christiane Ruetten [INACTIVE]]

Thanks for the update! The lock files definitely need to reflect the new version (as we're basing update/vulnerability checkers on top of them), but I don't know how to update npm's lock files. There might be an answer to bug 1566031, comment 2 soon.

Comment 4

[Scott [:thecount] Downe]

The issue with the lock file is it doesn't represent what's actually running, it represents the version our dependencies were initially shipped with.

So in order to update the lock file we need to wait for those dependencies to update and release versions using 4.17.14. Which at the time of my patch were not updated yet. Updating those in the end is the long term solution.

All I did was force the new version to be used and override what they are shipping until they all ship updates.

That's my understanding of package-lock.json, which admittedly I'm pretty new with.

Comment 5

[BugBot [:suhaib / :marco]]

The priority flag is not set for this bug.
:thecount, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Kate Hudson :k88hudson]

Although the actually dependency graph looks fine in node_modules (the only copy of lodash installed is 4.17.14) and there's no actual vulnerable code being installed here, I don't know of a way to update references for sub dependencies in package-lock.json other than upgrading the dependencies themselves.

We could consider:

• Upgrading affected dependencies (after a quick look it doesn't look like this will completely solve the issue, although this would be good to do anyway)
• Forking dependencies and force-upgrading them (this doesn't seem particularly sustainable/maintainable)

It looks like this is also an issue for some other package-locks in the tree, I'm curious if :standard8 has found a solution?

Comment 7

[Mark Banner (:standard8)]

(In reply to Kate Hudson :k88hudson from comment #6)

Although the actually dependency graph looks fine in node_modules (the only copy of lodash installed is 4.17.14) and there's no actual vulnerable code being installed here, I don't know of a way to update references for sub dependencies in package-lock.json other than upgrading the dependencies themselves.

Basically, this isn't possible until authors update their sub-versions (and then we update to those new versions). Unless we fork the world, which wouldn't be practical.

However, the important bit here isn't the sub-dependencies, it is what gets installed at the top-level. I've just been through the package-lock.json referenced on the PR and all the sub-dependencies are along the lines of ^4.17.11.

That means all those sub-dependencies will match anything above 4.17.11 (or whatever the actual version is). npm will automatically install any later version that is available that is within the range specified - in this case, any version until lodash gets to version 5.0.0. You can try it out here.

The important bit for us is that 4.17.4 or later is installed, if one of the dependencies had just 4.17.0 (no ^) then we'd be in trouble.

This is the usefulness of semantic versioning. You can update the sub-dependencies without being locked into specific versions all the time. The reason we have package-lock.json is that some of the sub-dependencies updates can sometimes break things (or fix) that we don't expect, and package-lock.json allows us to ensure everyone installs the same version of all dependencies.

The PR didn't actually need to specify lodash at the top level - just regenerating package-lock.json would have been enough. These days, when I regenerate one, I'll normally do:

$ rm -r node_modules package-lock.json
$ npm install

This helps to guarantee a clean install with all the latest packages (including sub-dependences where semvar allows) and an up to date package-lock file.

In summary, we should just be checking what is listed at the top-level of the package-lock.json, as this is what is actually used.

Comment 8

[Kate Hudson :k88hudson]

Thanks :standard8, I agree with this assessment and that forking down the dependency chain is not viable. I think we should consider this issue fixed and make changes to how package-lock is being checked if necessary – does that sound reasonable :cr?

Comment 9

[Christiane Ruetten [INACTIVE]]

Thanks for checking "compatibilty" of dependency semvers further down the rabbit holes! Sounds good to me.

However, any vulnerable versions appearing in package-lock.json (anywhere in the tree) will continue to raise alarms.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

It seems to me that we want to decide on a policy about what to do here, as I'd prefer we don't have a bunch of alarms get raised and subsequently ignored. I'd be curious to know if/how this has been handled in services repos affected by the lodash issue...

Comment 11

[Christiane Ruetten [INACTIVE]]

I don't know how this was handled in services land, but other place in mozilla-central that had vulnerable lodash versions so far could easily be upgraded.

There have been attempts of trying to keep track[of why keeping a certain vulnerable version of a dependency around is alright or even desired (for example [1]), but they're all high-maintenance, non-scalable, obsoletion-prone, and very non-exhaustive, so upgrading is always the preferable option.

[1] https://github.com/mozilla-services/third-party-library-alert

Comment 12

[Greg G]

I don't think we settled on a process or workflow for handling vulnerable dependencies for moz-central or services in previous meetings. We usually make bugs for critical alerts we think could affect many repos like this one, but it's best effort.

In services land, we've let dev teams use the built-in github vulnerability alerts and resolution mechanism and scraped that data into:

• https://console.cloud.google.com/bigquery?project=dependency-metadata-236420&page=savedqueries

and exposed it on STMO at:

• https://sql.telemetry.mozilla.org/dashboard/foxsec-dependencies-alert-summaries
• https://sql.telemetry.mozilla.org/dashboard/foxsec-dependencies-packages

(ping me if you want access to these pages).

We don't have alerts or automatic bug creation set up using that data. It'd be good to nail down that process.

We could add gecko-dev to the repos to scrape and create bugs for the issues. Also as :cr notes it would be limited to stuff GitHub detects, so no C++ or Rust support at the moment.

Comment 13

[Kate Hudson :k88hudson]

I’m going to resolve this since there’s no more work to be done on this particular issue, I’ll file a follow up for what we want to do in terms of process/tracking for upgrading dependencies in components/newtab.

Comment 14

[Daniel Veditz [:dveditz]]

Ryan: did your status-firefox70 mean that you don't know if Firefox 70 is affected, or did you mean to set it to "unaffected"?

Kate: is lodash code actually incorporated into the New Tab page delivered to Firefox users (comment 2 says this is "shipping" which led to me calling this sec-critical), or is it part of the build or development process (comment 6 says "there's no actual vulnerable code being installed here")?

This bug didn't go through the sec-approval process before a fix landed as would normally be expected for a sec-critical bug, but we didn't get any pushback about that rating being bogus. Trying to figure out whether this needs a security advisory for Firefox 70 or not and whether ESR-68 needs a fix since it has this code, too. Looks like the New Tab page is not chrome privileged and is loaded in a child process (possibly the "privileged about" one, but still sandboxed) so even if this library is being used in newtab content it probably should have capped the rating at sec-high.

Comment 15

[Kate Hudson :k88hudson]

To clarify, lodash does not ship as part of the code that runs in Firefox – is it only used as a sub-dependency of several dev dependencies for us (which run at build/development time).