Maniphest T220467

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

Assigned To

None

Authored By

	Krinkle
	Apr 9 2019, 2:10 AM

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

Rename util.parseHTML to util.evalHTML or another name that signals it deviates from the default jQuery method
Adapt all old client calls to use the new name
Remove the following comment from https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/ed393746f6ae602666438517db37ed878108c22c/src/mobile.mediaViewer/ImageCarousel.js#L237-L242 as it is outdated. The comment was valid when util.parseHTML did not pass the global document. Additionally, the explicit global document arg can be removed from that call since it is the default now.

Related Objects

Mentioned Here: T214451: [Bug] A large amount of our errors are occurring in iOS Safari

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

Jdlrobson moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.Apr 9 2019, 4:32 PM

ovasileva moved this task from Needs Prioritization to Incoming on the Web-Team-Backlog board.May 1 2019, 4:13 PM

ovasileva triaged this task as Medium priority.May 28 2019, 4:30 PM

ovasileva moved this task from Incoming to Needs Prioritization on the Web-Team-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

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 moved this task from Needs Prioritization to Upcoming on the Web-Team-Backlog board.Jun 8 2019, 12:24 AM

• nray removed • nray as the assignee of this task.Jun 8 2019, 12:34 AM

• nray subscribed.

In T220467#5244196, @nray wrote:
[..] 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);

MBinder_WMF added a project: Technical-Debt.Jun 19 2019, 4:45 PM

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>');

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.

Jdlrobson moved this task from Upcoming to Triaged but Future on the Web-Team-Backlog board.Jul 23 2019, 6:36 PM

Jdlrobson moved this task from Triaged but Future to Tracking on the Web-Team-Backlog board.Jul 31 2019, 6:46 PM

Jdlrobson edited projects, added Web-Team-Backlog (Tracking); removed Web-Team-Backlog.

Jdlrobson edited projects, added MobileFrontend (MobileFrontend and MinervaNeue architecture); removed MobileFrontend.

Jdlrobson moved this task from Backlog to Misc on the MobileFrontend (MobileFrontend and MinervaNeue architecture) board.Jul 31 2019, 8:40 PM

Jdlrobson moved this task from Untriaged to Untag on the Web-Team-Backlog (Tracking) board.Sep 30 2021, 9:23 PM

Jdlrobson edited projects, added Web-Team-Backlog (Needs Prioritization (Tech)); removed Web-Team-Backlog (Tracking).Nov 5 2021, 12:04 AM

Jdlrobson moved this task from Backlog to WVUI migrations on the Web-Team-Backlog (Needs Prioritization (Tech)) board.Nov 5 2021, 2:39 AM

Jdlrobson edited projects, added MobileFrontend; removed MobileFrontend (MobileFrontend and MinervaNeue architecture).Nov 30 2021, 5:55 PM

Jdlrobson moved this task from WVUI migrations to MobileFrontend frontend cleanup on the Web-Team-Backlog (Needs Prioritization (Tech)) board.Dec 7 2021, 1:44 AM

Reevaluate util.parseHTML semantics in MobileFrontendOpen, MediumPublic3 Estimated Story PointsActions

Description

AC

Related Objects

Event Timeline

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