Page MenuHomePhabricator

When special pages are included, OutputPage::$mPreventClickjacking is not respected
Closed, ResolvedPublic

Assigned To
None
Authored By
PleaseStand
May 26 2014, 6:45 PM
Referenced Files
F14042: bug65778b-REL1_19.patch
Nov 22 2014, 3:24 AM
F14041: bug65778b-REL1_22.patch
Nov 22 2014, 3:24 AM
F14040: bug65778b-REL1_23.patch
Nov 22 2014, 3:24 AM
F14039: bug65778b.patch
Nov 22 2014, 3:24 AM

Description


Version: unspecified
Severity: normal

Details

Reference
bz65778

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:24 AM
bzimport set Reference to bz65778.
bzimport added a subscriber: Unknown Object (MLST).

To reproduce, navigate to the following URL:

data:text/html,<iframe src="https://en.wikipedia.org/w/index.php?title=User:PleaseStand/Sandbox&oldid=610241510" width="800" height="600"></iframe>

Rollback links are visible inside the iframe.

Thanks Kevin, that's definitely an issue. I'm not sure what our options are for checking the properties of transcluded pages, but that seems like something we should do.

Adding Brad and Tim, in case they've thought through this already.

Created attachment 15487
Copy prevent-clickjacking between OutputPage and ParserOutput

I haven't thought this through yet. But it looks like we already have functions copying metadata back and forth between ParserOutput and OutputPage, so fixing it could easily go like this:

  1. Have a 'mPreventClickjacking' flag in ParserOutput.
  2. Have ParserOutput::addOutputPageMetadata() combine its flag with the one from the passed OutputPage.
  3. Have OutputPage::addParserOutputNoText() do the same thing from the passed ParserOutput.

attachment 0001-Copy-prevent-clickjacking-between-OutputPage-and-Par.patch ignored as obsolete

Created attachment 15894
Copy prevent-clickjacking between OutputPage and ParserOutput

I updated Brad's patch to take out the indentation change-- I'd like to make sure the security part is as small a change as possible. I'll submit an indentation change for all those once this is public.

Attached:

Deployed, and PleaseStand's PoC no longer works. Sorry for delay in getting this out!

19:45 csteipp: deployed patch for bug65778

Created attachment 16082
Backport for REL1_23

This backport was tested with

data:text/html,<iframe src="http://localhost/REL1_23/core/index.php?title=Benutzer:WikiSysop" width="800" height="600"></iframe>

Before the patch, the included special page was displayed. After the patch, the content of the iframe is blank (no errors). This is the same behavior as in master.

Attached:

Created attachment 16083
Backport to REL1_22

This backport was tested with

data:text/html,<iframe src="http://localhost/REL1_22/core/index.php?title=Benutzer:WikiSysop" width="800" height="600"></iframe>

Before the patch, the included special page was displayed. After the patch, the content of the iframe is blank (no errors). This is the same behavior as in master.

Can someone please confirm the backport works as designed?

Attached:

Created attachment 16084
Backport to REL1_19

This backport was tested with

data:text/html,<iframe src="http://localhost/REL1_19/core/index.php?title=Benutzer:WikiSysop" width="800" height="600"></iframe>

Before the patch, the included special page was displayed. After the patch, the content of the iframe is blank (no errors). This is the same behavior as in master.

Can someone please confirm the backport works as designed?

Attached:

Adding early access for Wikia and Debian

Backports all seem to work fine. +2.

So is there something left to do here, or is everything merged (RESOLVED FIXED)?

Looks like everything is merged to me. If I'm wrong, feel free to reopen.