Page MenuHomePhabricator

Regex DoS vulnerability in moment.js
Closed, ResolvedPublic

Description

Parsing of localized month names in moment.js involves a regex that has no length limit and does some amount of backtracking. See moment/moment#4163. MediaWiki core is also affected.

From the upstream report:

The slowdown is moderately low: for 50.000 characters around 2 seconds matching time.

so not too much of a deal (at worst a maliciously crafted wiki page can lock up browsers for a few seconds) - it only came up because some of our node.js services are using moment and the node dependency security checks are breaking the build. Nevertheless, no reason not to fix it in the frontend as well, once upstream has a patch.

Side note: there are various vulnerability trackers for JS dependencies (even Github itself sends warnigns these days), but we don't formally track the stuff in resources/lib as dependencies so we don't get them. Should that be improved?

Event Timeline

Tgr added a subtask: Restricted Task.Nov 28 2017, 6:51 PM
Tgr updated the task description. (Show Details)
Tgr added a project: Security-Core.
Tgr updated the task description. (Show Details)

Moment is used in MediaViewer, the VisualEditor media upload dialog, in the OOUI calendar widget; not sure if there's anything else. (See also T146798: Should we replace Moment.js in core with something smaller? (If yes, what?).)

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

the VisualEditor media upload dialog,

That's part of MediaWiki core, not VisualEditor: https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki/mediawiki.Upload.BookletLayout.js

in the OOUI calendar widget;

That's part of MediaWiki core, not OOUI: https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.widgets/mw.widgets.DateInputWidget.js

not sure if there's anything else. (See also T146798: Should we replace Moment.js in core with something smaller? (If yes, what?).)

Used by Notifications (ext.echo.ui dependency), StructuredDiscussions (ext.flow.templating dependency), ContentTranslation (ext.cx.translationlist dependency), and it looks like a bunch of other places, though possibly not exposed: https://github.com/search?l=JSON&q=moment+%40wikimedia&type=Code&utf8=%E2%9C%93

Side note: there are various vulnerability trackers for JS dependencies (even Github itself sends warnigns these days), but we don't formally track the stuff in resources/lib as dependencies so we don't get them. Should that be improved?

Yeah, good idea, let's spin that off to a new task.

A fix for the issue was released in moment v2.19.3.

We're on 2.15.0 in core....

Is there only a 2.19 point release to fix it?

There's a lot of changes between these https://github.com/moment/moment/compare/2.15.2...2.19.3 - I'm guessing this isn't a trivial version bump?

https://github.com/moment/moment/blob/develop/CHANGELOG.md suggests no obvious breaking changes, but some new features, various bugfixes and alike

2.19 has another bug that looks useful to fix

#4213 [critical] Rename dynamic require to avoid React Native crash

@Reedy I've been following the issue and haven't heard any plans to backport the fix to earlier minor versions. :/

EBjune added a subscriber: EBjune.

Looks like @Jdforrester-WMF submitted a patch, we'll keep an eye on it.

We should probably backport to REL1_27, REL1_29 and the upcoming REL1_30 (even though we've just had an rc)

Shouldn't be too bad..

https://gerrit.wikimedia.org/r/#/c/310367/ is in 29 and 30... So just a cherry pick will do of the master commit, fix RELEASE-NOTES

For REL1_27.. We're on an even older 2.8.4 - https://github.com/wikimedia/mediawiki/blob/REL1_27/resources/lib/moment/moment.js

So we should just cherry pick https://gerrit.wikimedia.org/r/#/c/310367/ first, then do the newer bump ontop, and add release-notes

remote:   https://gerrit.wikimedia.org/r/394112 momentjs: Hack around bug in node/browser compat wrapper in locale files        
remote:   https://gerrit.wikimedia.org/r/394113 Override momentjs's digit transform logic with MW's        
remote:   https://gerrit.wikimedia.org/r/394114 Don't override all Moment locales to English        
remote:   https://gerrit.wikimedia.org/r/394115 resources: Bump moment.js from 2.8.4 to 2.15.0

Brings REL1_27 into line

Make this public now?

Do we care about deploying to WMF first?

Wait a week until the train's gone everywhere?

Make this public now?

Wait a week until the train's gone everywhere?

That week is over now

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".
Mholloway closed subtask Restricted Task as Resolved.Dec 8 2017, 11:42 PM

Change 407694 had a related patch set uploaded (by Ejegg; owner: Jforrester):
[mediawiki/core@fundraising/REL1_27] resources: Bump moment.js from 2.15.0 to 2.19.3

https://gerrit.wikimedia.org/r/407694

Change 407694 merged by jenkins-bot:
[mediawiki/core@fundraising/REL1_27] resources: Bump moment.js from 2.15.0 to 2.19.3

https://gerrit.wikimedia.org/r/407694