Page MenuHomePhabricator

Enable wgBreakFrames across all projects
Open, MediumPublic

Description

This task is basically a public version of T112428 to solicit comments/tokens as to whether or not this is a good idea. I (and I think most members of the Security-Team) would like to see this enabled so as to marginally increase our defenses against clickjacking and limit the number of reports from "security researchers" we receive, which do nothing but waste our time. If there are compelling arguments AGAINST enabling this config variable, then I'm happy to decline the task and abandon the config patch.

Event Timeline

sbassett created this task.Jun 19 2020, 4:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2020, 4:19 PM
sbassett triaged this task as Medium priority.Jun 19 2020, 4:19 PM
sbassett moved this task from Backlog to In Progress on the user-sbassett board.

Change 606723 had a related patch set uploaded (by SBassett; owner: SBassett):
[operations/mediawiki-config@master] Enable wgBreakFrames across all projects

https://gerrit.wikimedia.org/r/606723

sbassett updated the task description. (Show Details)Jun 19 2020, 4:29 PM

For reference, I believe CVE-2014-5243/T67778 was the last clickjacking issue in MediaWiki.

On T255881 there were some cases of legitimate framing for e.g. side-by-side comparisons. In general, we want to allow for wide reuse of our content, and framing is a really simple way of doing that. I like the suggestion of still allowing action=render to be framed, which I think should address all the legitimate cases, while closing the attack surface nearly everywhere else...is that reasonable? Or does leaving action=render open still cause problems?

sbassett moved this task from Incoming to In Progress on the Security-Team board.Jun 22 2020, 3:07 PM
sbassett added a comment.EditedJun 22 2020, 9:08 PM

On T255881 there were some cases of legitimate framing for e.g. side-by-side comparisons. In general, we want to allow for wide reuse of our content, and framing is a really simple way of doing that. I like the suggestion of still allowing action=render to be framed, which I think should address all the legitimate cases, while closing the attack surface nearly everywhere else...is that reasonable? Or does leaving action=render open still cause problems?

This sounds reasonable to me, certainly for now. Unfortunately $wgBreakFrames is a bit of a hammer, as-is. If OP->allowClickjacking() could override it, I think the solution would be simple for action=render. Otherwise I'm not quite sure what the cleanest approach might be. A new global? Seems a bit much. Creating some action exception logic for $wgBreakFrames? Defeats the purpose a bit IMO and I'm not sure just how ugly something like $wgBreakFramesAllowedActions would be.

Eh, I guess we'd have to explore something else anyways as there are probably too many important uses of allowClickjacking() right now, which $wgBreakFrames = true; would seemingly break.

This sounds reasonable to me, certainly for now. Unfortunately $wgBreakFrames is a bit of a hammer, as-is. If OP->allowClickjacking() could override it, I think the solution would be simple for action=render. Otherwise I'm not quite sure what the cleanest approach might be. A new global? Seems a bit much. Creating some action exception logic for $wgBreakFrames? Defeats the purpose a bit IMO and I'm not sure just how ugly something like $wgBreakFramesAllowedActions would be.

We currently have:

  • $wgBreakFrames
  • $wgEditPageFrameOptions
  • $wgApiFrameOptions

We could combine them all into:

$wgBreakFrames = [ 
'view' => 'DENY', // or maybe 'all' ??
'editpage' => 'DENY', 
'api' => 'DENY',

Then adding $wgBreakFrames['render'] = false; would be straightforward. It would be straightforward to have a layer that continues to support the old bool value as well for back-compat.

Eh, I guess we'd have to explore something else anyways as there are probably too many important uses of allowClickjacking() right now, which $wgBreakFrames = true; would seemingly break.

Based on that codesearch, it looks like there are 2 meanings for allowClickjacking(), one is "this page is safe to frame" and the other is "this page needs framing to work". So what if we have allowClickjacking() default to the "this page is safe to frame" case, in which a restrictive $wgBreakFrames policy would overrule it, and introduce a new requireClickjacking() with some security warning that would overrule $wgBreakFrames. Then we'd be free to tighten $wgBreakFrames as much as we want without breaking legitimate uses.

Change 606723 abandoned by SBassett:
[operations/mediawiki-config@master] Enable wgBreakFrames across all projects

Reason:
In favor of implementing T255881#6255282

https://gerrit.wikimedia.org/r/606723