Page MenuHomePhabricator

Loader broken in IE because mediawiki thinks the periods in module names is a security leak (spoof extension)
Closed, ResolvedPublic

Description

How a diff displays in IE. Firefox displays this properly with all the highlighting etc.

Running a Windows machine, I find diffs are not displaying properly in Internet Explorer.


Version: unspecified
Severity: normal

Attached:

Related Objects

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz28840.
bzimport added a subscriber: Unknown Object (MLST).

Firefox displays it properly.

Attached:

Does hard-refreshing the page (Ctrl+F5) fix it?

Sometimes. Sometimes not - and, experiment shows, sometimes hard refreshing on the rare occasions it displays properly actually leads to the diff ceasing to display correctly.

brion added a comment.May 5 2011, 9:30 PM

Likely related to bug 27488, bug 27769; might be some oddity disturbing the post-loading of the styles.

Loading
http://bits.wikimedia.org/de.wikipedia.org/load.php?debug=false&lang=de&modules=mediawiki.legacy.diff&only=styles&skin=vector
directly in IE 8 gives me a 403 Forbidden, although the download of the CSS starts in background.

On secure.wikimedia.org not only diff.css but almost all CSS and JS seems to be missing.

We had an out-of-sync server in the bool, that's probably what was causing this. Is this fixed now?

Sadly, no, it doesn't seem to be. I'll try again around noon UTC.

It is working fine on local installation with r87481, update to r87482 breaks it.

Still not working

Going over into IE it is still much the same, despite again attempting hard refresh.

Attached:

CCing Tim, since this is possibly caused by the security fix by the look of the other comments.

See also bug 27814, bug 28851, and bug 28857 !
See also problem reports at http://meta.wikimedia.org/wiki/Wikimedia_Forum#Troubles

Updating bug summary based on duplicate bug's comments

Created attachment 8519
patch to use ! marks instead of .'s in module names during requests

So based on above comments this is triggered by the whole IE6 is stupid with extensions issue, combined with using dots to denote the hierarchy of RL module names.

So the most obvious fix (but rather hacky) would be to use ! marks instead of dots to denote the hierarchy (! mark chosen arbitrary. Perhaps something that isn't hex escaped would be better). This patch does that (only during requests for the user. Everything in the backend is still periods.) Not sure if this is an acceptable solution.

In my testing this patch does fix the issue on IE.

Attached:

TheDJ added a comment.May 8 2011, 3:18 PM
  • Bug 28874 has been marked as a duplicate of this bug. ***
TheDJ added a comment.May 8 2011, 3:20 PM
  • Bug 28847 has been marked as a duplicate of this bug. ***

It seems to me like the IE 6 security measure is overinclusive. Tim, didn't you say the bug only affected URLs that don't have an extension before the question mark? load.php URLs always do, so they should be fine.

bdanee88 wrote:

*** Bug 28877 has been marked as a duplicate of this bug. ***

  • Bug 28890 has been marked as a duplicate of this bug. ***

(In reply to comment #13)

Created attachment 8519 [details]
patch to use ! marks instead of .'s in module names during requests

So based on above comments this is triggered by the whole IE6 is stupid with
extensions issue, combined with using dots to denote the hierarchy of RL module
names.

So the most obvious fix (but rather hacky) would be to use ! marks instead of
dots to denote the hierarchy (! mark chosen arbitrary. Perhaps something that
isn't hex escaped would be better).

Hmm. The exclamation marks get urlencoded in the URLs that MW outputs in the
HTML (e.g. for <link href="...">) but not for AJAX requests, at least in
Firefox.

This patch does that (only during requests
for the user. Everything in the backend is still periods.) Not sure if this is
an acceptable solution.

Thanks for this patch. It's not a good permanent solution IMO, but it's the
best we have right now to fix the site for IE users.

I committed the patch in r87711 and applied it to 1.17wmf1 (by hand) in r87712,
which is live now. The breakage should disappear within 10 minutes for
logged-in IE users, and may persist a bit longer for anonymous IE users.

Not closing the bug as FIXED because the fix is temporary and ugly.

Attached:

(Re. comment #11)

See also bug 27814, bug 28851, and bug 28857 !

Typo: I meant bug 28714, not 27814.

  • Bug 28878 has been marked as a duplicate of this bug. ***
  • Bug 29010 has been marked as a duplicate of this bug. ***

Making the summary sane and shorter.

Fixed in r88883.

  • Bug 28962 has been marked as a duplicate of this bug. ***
  • Bug 29158 has been marked as a duplicate of this bug. ***

p.oranje wrote:

Will the patch be applied onto branches/REL1_17 (and when) so that the fix can be got with svn update?

brion added a comment.May 27 2011, 9:08 PM

I still see this on trunk in r88997, and it's affecting Firefox 4 not just IE.

Reproduce:

  1. Enable MwEmbedSupport extension in LocalSettings.php:

require( "$IP/extensions/MwEmbedSupport/MwEmbedSupport.php" );

  1. Load up a page on the wiki
  1. See a 403 Forbidden error for load.php of some initial modules, and JS errors due to jQuery and mw not being loaded by:

http://stormcloud.local/trunk/load.php?debug=true&lang=en&modules=jquery%7Cmediawiki%7Cjquery.triggerQueueCallback%7Cjquery.mwEmbedUtil%7Cmw.MwEmbedSupport&only=scripts&skin=vector&version=20110526T174802Z

<!DOCTYPE HTML PUBLIC "-IETFDTD HTML 2.0//EN"><html><head><title>Forbidden</title></head><body><h1>Forbidden</h1><p>Invalid file extension found in PATH_INFO or QUERY_STRING.</p></body></html>

brion added a comment.May 27 2011, 9:29 PM

I also see this for some things still using legacy action=raw to load site css pages, such as [[MediaWiki:Filepage.css]] being loaded by ImagePage:

http://stormcloud.local/trunk/index.php?title=MediaWiki:Filepage.css&action=raw&maxage=18000&usemsgcache=yes&ctype=text%2Fcss&smaxage=18000

<!DOCTYPE HTML PUBLIC "-IETFDTD HTML 2.0//EN"><html><head><title>Forbidden</title></head><body><h1>Forbidden</h1><p>Invalid file extension found in PATH_INFO or QUERY_STRING. Raw pages must be accessed through the primary script entry point.</p></body></html>

Manually adding a '&*' on the end of the URL resolves it (as does removing the '.' from the '.css' though obviously then it doesn't load!)

(Fixing deps/blockers; RL, API, and action=raw failures here all affect TimedMediaHandler: bug 27699).

So it this going anywhere ? Cause it is heavily breaking other platforms atm, i'm starting to nudge towards 'revert and IE6 is no longer supported + banner "upgrade your browser"'.

It should be fixed as of r88883, to be released soon.

Well brion and I myself experienced issues after r88883 it seems. At least I had my trunk up to date, and I doubt that brion wasn't using up to date trunk.

(In reply to comment #34)

Well brion and I myself experienced issues after r88883 it seems. At least I
had my trunk up to date, and I doubt that brion wasn't using up to date trunk.

He actually specifically says: "I still see this on trunk in r88997"

We can fix a few more special cases, but there's not going to be a solution for this that can allow any arbitrary api.php or action=raw request.

It should be somewhat better as of r89249. It should mostly only block requests that have dots in the last parameter of the query string now.

One possible way to make it break less things would be to have it redirect to a safe equivalent of the same URL, instead of showing an error page. It wouldn't help with POST requests that have some parameters in the URL, and it wouldn't help with API clients that don't understand redirects, but at least it would fix any JavaScript client issues. What do you think?

mdale wrote:

bug 29224 is related patch for the RL start-up module names not using Packed Module String

(In reply to comment #37)

One possible way to make it break less things would be to have it redirect to a
safe equivalent of the same URL, instead of showing an error page.

Done in r89397.

(In reply to comment #39)

(In reply to comment #37)
> One possible way to make it break less things would be to have it redirect to a
> safe equivalent of the same URL, instead of showing an error page.

Done in r89397.

Can this ticket be closed now or is there still something pending ?

p.oranje wrote:

Shouldn't r89397 be tagged with 1.17, 1.17wmf1?
OT: When do (ports of) these revisions land on the 1.17 branch?

(In reply to comment #41)

Shouldn't r89397 be tagged with 1.17, 1.17wmf1?

It will be once I review it.

OT: When do (ports of) these revisions land on the 1.17 branch?

Probably this week.

(In reply to comment #40)

Can this ticket be closed now or is there still something pending ?

Roan can close it once he has reviewed the patch. This is the last real blocker to 1.17. No pressure, though. ;)

(In reply to comment #41)

Shouldn't r89397 be tagged with 1.17, 1.17wmf1?
OT: When do (ports of) these revisions land on the 1.17 branch?

Tim's already merged them, apparently.

(In reply to comment #43)

(In reply to comment #40)
> Can this ticket be closed now or is there still something pending ?

Roan can close it once he has reviewed the patch. This is the last real
blocker to 1.17. No pressure, though. ;)

Tim's code is fine, closing this bug.

  • Bug 29531 has been marked as a duplicate of this bug. ***

Mark: Please explain why you reopened this bug.

It's probably easier to keep bug 29531 separate, since it doesn't seem to be very closely related to this bug.

For people coming here from the PHP comment

// Prevent the IE6 extension check from being triggered (bug 28840)

and wondering why RL appends &* to load.php URLs, the explanation is in Roan's original fix to OutputPage.php, rMW8dab43f7031e: (bug 28840) URLs with dots break because of IE6 security check.

For people coming here from the PHP comment

// Prevent the IE6 extension check from being triggered (bug 28840)

and wondering why RL appends &* to load.php URLs, the explanation is in Roan's original fix to OutputPage.php, rMW8dab43f7031e: (bug 28840) URLs with dots break because of IE6 security check.

I am indeed wondering why RL appends &*, because as Roan said:

It seems to me like the IE 6 security measure is overinclusive. Tim, didn't you say the bug only affected URLs that don't have an extension before the question mark? load.php URLs always do, so they should be fine.

The over-inclusiveness was fixed (as part of rMW8dab43f703 / r88883 was committed). I don't see another explanation in the commit message for appending &*.

All the URLs reported in this task work normally. I've been trying but couldn't find anyway to trigger the IE extension security warning from any url that starts with https://en.wikipedia.org/w/load.php?.

https://de.wikipedia.org/w/load.php?debug=false&lang=de&modules=mediawiki.legacy.diff&only=styles&skin=vector
https://test.wikipedia.org/w/load.php?debug=true&lang=en&modules=jquery%7Cmediawiki%7Cjquery.triggerQueueCallback%7Cjquery.mwEmbedUtil%7Cmw.MwEmbedSupport&only=scripts&skin=vector&version=20110526T174802Z
https://test.wikipedia.org/w/api.php?list=allpages&apprefix=Folgers_local_prototype.ogv&apnamespace=700&aplimit=200&prop=revisions&action=query&format=json
https://test.wikipedia.org/w/api.php?action=parse&page=File%3AFolgers_local_prototype.ogv&smaxage=3600&maxage=3600&format=json

Even if it contains ?.html we're not screwed as far as I know.

https://en.wikipedia.org/w/load.php?x.html&x.exe&x.zip?x.exe&x.zip?.html

The issue is the case where IEUrlExtension::haveUndecodedRequestUri() returns false. In that case, there's no way to tell whether or not the dot was URL-encoded, so the exploit detector proceeds on the assumption that it was URL-encoded. That means extracting the extension from QUERY_STRING instead of REQUEST_URI, ignoring the path part.

You can easily trigger the security redirect by patching IEUrlExtension::haveUndecodedRequestUri() to return false. You can trigger a 403 with such a patch by doing a POST request. I don't think it is possible for a GET request -- the infinite loop case would have to be hit, which seems unlikely. So for RL, it may be possible to remove the &* and rely on the security redirect.