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 created this task.Nov 28 2017, 6:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 28 2017, 6:50 PM
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)
Tgr added subscribers: Mholloway, MarkTraceur, Krinkle, Esanders.

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

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptNov 28 2017, 7:07 PM

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.

Reedy added a subscriber: Reedy.Nov 29 2017, 6:41 PM

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

Reedy added a comment.Nov 29 2017, 6:46 PM

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. :/

https://gerrit.wikimedia.org/r/394103 has the upgrade. Looks clean enough to me.

EBjune triaged this task as Low priority.Nov 29 2017, 6:56 PM
EBjune added a subscriber: EBjune.

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

EBjune moved this task from Backlog to To Follow Up on the Security-Team board.
Reedy added a comment.Nov 29 2017, 7:06 PM

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

Reedy added a comment.Nov 29 2017, 7:46 PM
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 closed this task as Resolved.Dec 8 2017, 9:42 PM
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

Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 31 2018, 8:36 PM
sbassett moved this task from To Follow Up to Done on the Security-Team board.Jun 11 2019, 6:09 PM