Page MenuHomePhabricator

Reevaluate util.parseHTML semantics in MobileFrontend
Open, MediumPublic3 Estimate Story Points

Description

Follows-up from T214451#4961656, and:

Change 486145 (merged):

[mediawiki/extensions/MobileFrontend@master] Explicitly pass in parseHTML
https://gerrit.wikimedia.org/r/486145

I'm glad to hear util.parseHTML() is only used for literal inline HTML strings, with no variables or user-input passed in.

Background:

The only reason jQuery offers $.parseHTML(String html) in addition to $(String html) is for security reasons. $.parseHTML exists to allow a caller to safely parse a string without the current document's execution context being involved. E.g. inline scripts and legacy event attributes.

The above patch has made it insecure by passing in the one Document object the methods exists for to secure: The global one. Thus effectively turning it into a home-made version of $(String html). This may be fine for the currently audited calls to this functions but is risky for two reasons:

  • Given the purpose of util in MobileFrontend as a jQuery-proxy, it is reasonable for developers to expect it will behave as $.parseHTML. Specifically, its security semantics. When reviewing its 3-line source code in mobile.startup/util, the innocent-looking passing of the global document object likely does not suffice to signal most readers that unsafe JavaScript execution may be ahead. This is dangerous.
  • It is now effectively a duplicate of $(), which is also in use.

I propose to deprecate util.parseHTML in favour of $().

Or perhaps rename like util.evalHTML, and update remaining uses of $().

AC

Event Timeline

Krinkle created this task.Apr 9 2019, 2:10 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 9 2019, 2:10 AM
ovasileva triaged this task as Medium priority.May 28 2019, 4:30 PM
ovasileva moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

We're trying to minimize direct jQuery dependencies so util.evalHTML() is preferable to me.

pmiazga assigned this task to nray.Jun 6 2019, 5:22 PM
nray added a comment.Jun 8 2019, 12:05 AM

Thanks for bringing this up @Krinkle. tldr; I agree it could be better named and I ran some tests to see if I could make an attack vector.

I was curious to see if I could replicate the security vulnerability, and I was able to by running the following in the chrome console when visiting wikipedia:

$.parseHTML(`<img src="https://en.m.wikipedia.org/NONEXISTANT.jpg" onerror="console.log('evil stuff')" />;`, document);

Running the above will execute the JS in Chrome Version 74.0.3729.169 whereas relying on the default document param won't :

$.parseHTML(`<img src="https://en.m.wikipedia.org/NONEXISTANT.jpg" onerror="console.log('evil stuff')" />;`);

I could not replicate the inline script execution when parsed concern even when I set the keepScripts param to true :

$.parseHTML("<script>console.log('evil stuff');</script>", document, true);

^^ above doesn't print anything for me.

But regardless this warrants a rename.

nray updated the task description. (Show Details)Jun 8 2019, 12:11 AM
nray updated the task description. (Show Details)Jun 8 2019, 12:16 AM
nray updated the task description. (Show Details)
nray removed nray as the assignee of this task.Jun 8 2019, 12:34 AM
nray added a subscriber: nray.

[..] I was able to [replicate the vulnerability] by running the following in the chrome console when visiting wikipedia:

$.parseHTML(`<img src="https://en.m.wikipedia.org/NONEXISTANT.jpg" onerror="console.log('evil stuff')" />;`, document);

[..] I could not replicate the inline script execution [..] even when I set the keepScripts param:

$.parseHTML("<script>console.log('evil stuff');</script>", document, true);

Inline scripts execute when they are "connected" (e.g. attached to the tree root). So this example requires one more step, but is equally vulnerability:

nodes = $.parseHTML("<script>console.log('evil stuff');</script>", document, true);
$(document.body).append(nodes);
// logs: "evil stuff"

// Compare to
node = document.createElement('script');
node.textContent = 'console.log(123);';
document.body.append(node);
ovasileva set the point value for this task to 3.Jun 19 2019, 4:53 PM

Looks like in most places we use util.parseHTML() just to create a single element:

mobile.special.uploads.scripts/PhotoList.js:99:         this.parseHTML( '<p class="content empty">' ).text( mw.msg( 'mobile-frontend-donate-image-nouploads' ) )
mobile.notifications.overlay/NotificationsFilterOverlay.js:40:          this.parseHTML( '<div>' )
mobile.notifications.overlay/list.js:43:                $moreOptions = util.parseHTML( '<div>' )
mobile.startup/search/SearchOverlay.js:193:             this.parseHTML( '<input>' )
mobile.startup/search/SearchGateway.js:79:              label = util.parseHTML( '<span>' ).text( label ).html();
mobile.startup/search/SearchGateway.js:80:              term = util.parseHTML( '<span>' ).text( term ).html();
mobile.startup/time.js:106:             return util.parseHTML( '<div>' ).html( html ).text();
mobile.startup/references/ReferencesMobileViewGateway.js:63:                            var $section = util.parseHTML( '<div>' ).html( section.text );
mobile.startup/references/ReferencesMobileViewGateway.js:101:                   var $container = util.parseHTML( '<div>' );
mobile.startup/Icon.js:124:             return this.parseHTML( '<div>' ).append( this.$el ).html();
mobile.startup/util.js:84:       * Unlike jQuery.parseHTML this will return a jQuery object
mobile.startup/util.js:93:      parseHTML: function ( html, ctx ) {
mobile.startup/util.js:95:              return $( $.parseHTML( html, ctx ) );
mobile.startup/View.js:186:                     this.$el = this.parseHTML( '<' + this.tagName + '>' );
mobile.startup/View.js:354:      * See parseHTML method of util singleton
mobile.startup/View.js:361:     parseHTML: function ( html ) {
mobile.startup/View.js:365:             return util.parseHTML( html, document );
mobile.startup/Skin.js:87:              util.parseHTML( '<div class="transparent-shield cloaked-element">' )
mobile.startup/watchstar/WatchstarPageList.js:138:                              el = self.parseHTML( '<div>' ).appendTo( $item ),
mobile.talk.overlays/AddTopicForm.js:56:                        $subject = util.parseHTML( '<input>' ).attr( {
mobile.talk.overlays/AddTopicForm.js:63:                        $body = util.parseHTML( '<textarea>' ).attr( {
mobile.talk.overlays/AddTopicForm.js:71:                                makePanel( util.parseHTML( '<p>' ).addClass( 'license' ).html( options.licenseMsg ) ),
mobile.talk.overlays/talkBoard.js:19:           util.parseHTML( '<p class="content-header">' ).text( explanation ),
mobile.talk.overlays/talkBoard.js:21:           util.parseHTML( '<ul class="topic-title-list">' ).append(
mobile.talk.overlays/talkBoard.js:23:                           return util.parseHTML( '<li>' ).append(
mobile.talk.overlays/talkBoard.js:24:                                   util.parseHTML( '<a>' )
mobile.mediaViewer/ImageCarousel.js:243:                        $img = self.parseHTML( '<img>', document );
mobile.editor.overlay/SourceEditorOverlay.js:259:                       self.sectionLine = self.parseHTML( '<div>' ).html( parsedSectionLine ).text();

Is there a difference between for example:

$img = self::parseHTML('<img>')

and

$img = $('<img>')

and

$img = $( document.createElement('img') );

???

In most places looks like the parseHTML function is just a syntax sugar, and we can just use $object = $('<tag>');

nray added a comment.Jun 19 2019, 6:30 PM

Is there a difference between for example:
$img = self::parseHTML('<img>')
and
$img = $('<img>')
and
$img = $( document.createElement('img') );

I could be wrong, but I believe the reason we don't use the $(stuff) version in MF classes is because we wanted to to isolate our usage of jQuery to a single place - util.js so that we could better measure our JQuery usage footprint and maybe make it easier to deprecate/replace it in the future. I thought that's what I remembered Jon saying before anyways. ESlint will also complain if you try to use a global JQuery.

The reason we moved away from $ was we had views that were accessing elements outside their view. Given how we currently use parseHTML I think a $create util function that only allows creation of jQuery-ified DOM nodes would suffice.

e.g

util.parseHTML( '<p class="content empty">' )
could become
util.$create( 'p' ).addClass( 'content empty' )

I'm guessing further down the line we'll want to make use of document.createElement more, so this seems consistent with that motive.