Page MenuHomePhabricator

Users can change the content model of other users' user pages to CSS or JS
Closed, ResolvedPublic

Description

If $wgContentHandlerUseDB is enabled, it's possible for a user to change the content model of a page in another user's userspace to CSS or JS. There are two problems with this: One is that a user could be a nuisance by mass-changing the pages, since once they're changed like that, only the userspace's owner or an admin can fix them. Another is that if a user accidentally sets their common.js to wikitext, a malicious user could add bad code to it and change its content model back to JS, so it would get executed. To fix this, when checking if a page is a protected user settings page, we need to check both the old and the new content model, rather than just the old one as we do now.


Version: unspecified
Severity: blocker
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=73490
https://bugzilla.wikimedia.org/show_bug.cgi?id=71163

Details

Reference
bz70901

Related Objects

Event Timeline

bzimport raised the priority of this task from to Normal.
bzimport set Reference to bz70901.
bzimport changed Security from none to Software security bug.
Restricted Application changed the visibility from "Public (No Login Required)" to "Security (Project)". · View Herald TranscriptNov 22 2014, 3:47 AM
Restricted Application changed the edit policy from "All Users" to "Security (Project)". · View Herald Transcript

Thanks Jackmcbarn. S mentioned this too earlier this week, since they saw something similar while testing flow. But thanks for calling out these specific issues.

Let me duplicate these.. I don't feel like I have a good understanding of all the changes we done in this area the past few months.

Confirmed the second scenario is definitely exploitable.

  • Being able to set a user's page content type to javascript or css without edituserjs/editusercss rights seems like a bug.
  • Being able to update another user's common.js and skin .js subpages without edituserjs rights, even when the content type isn't javascript, doesn't seem right to me. But I understand why we're doing it, so as long as we fix this first issue, I think we can continue allowing it.

On the vandalism side, this definitely needs a lot more testing before we enable $wgContentHandlerUseDB on more wikis. Rollback seems to work ok, but undo seems broken when undoing revisions that change the content type.

From a product standpoint, do we really want to allow other users to change the content type of another user's subpages at all? For that matter, should there be an 'editcontenttype' right that is needed to change the content type of any existing page?

(In reply to Chris Steipp from comment #2)

  • Being able to set a user's page content type to javascript or css without edituserjs/editusercss rights seems like a bug.

Only for pages in the User namespace (and even of them, only subpages). It's fine elsewhere.

  • Being able to update another user's common.js and skin .js subpages without edituserjs rights, even when the content type isn't javascript, doesn't seem right to me. But I understand why we're doing it, so as long as we fix this first issue, I think we can continue allowing it.

Agreed.

On the vandalism side, this definitely needs a lot more testing before we
enable $wgContentHandlerUseDB on more wikis. Rollback seems to work ok, but
undo seems broken when undoing revisions that change the content type.

I haven't looked at this in detail.

From a product standpoint, do we really want to allow other users to change
the content type of another user's subpages at all? For that matter, should
there be an 'editcontenttype' right that is needed to change the content
type of any existing page?

I don't see any reason to restrict that whatsoever.

(In reply to Chris Steipp from comment #2)

Confirmed the second scenario is definitely exploitable.

  • Being able to set a user's page content type to javascript or css without edituserjs/editusercss rights seems like a bug.
  • Being able to update another user's common.js and skin .js subpages without edituserjs rights, even when the content type isn't javascript, doesn't seem right to me. But I understand why we're doing it, so as long as we fix this first issue, I think we can continue allowing it.

In theory that second point seems innocent indeed, however we shouldn't because of current reality. The reality is that the two ways of loading scripts and styles (ResourceLoader modules and importScript/action=raw) do not pay attention to the content model. So content is executable regardless of the content model. The content model is merely a meaningless front-end flag at the moment for having a pretty editor, source highlighting (and in theory syntax validation).

(In reply to Krinkle from comment #4)

In theory that second point seems innocent indeed, however we shouldn't
because of current reality. The reality is that the two ways of loading
scripts and styles (ResourceLoader modules and importScript/action=raw) do
not pay attention to the content model. So content is executable regardless
of the content model. The content model is merely a meaningless front-end
flag at the moment for having a pretty editor, source highlighting (and in
theory syntax validation).

Well, actually RL does pay attention to the content format: https://github.com/wikimedia/mediawiki-core/blob/master/includes/resourceloader/ResourceLoaderWikiModule.php#L84. If it's not a JS or CSS format page, it'll ignore it.

(In reply to Krinkle from comment #4)

(In reply to Chris Steipp from comment #2)
> Confirmed the second scenario is definitely exploitable.
>
> * Being able to set a user's page content type to javascript or css without
> edituserjs/editusercss rights seems like a bug.
> * Being able to update another user's common.js and skin .js subpages
> without edituserjs rights, even when the content type isn't javascript,
> doesn't seem right to me. But I understand why we're doing it, so as long as
> we fix this first issue, I think we can continue allowing it.
>

In theory that second point seems innocent indeed, however we shouldn't
because of current reality. The reality is that the two ways of loading
scripts and styles (ResourceLoader modules and importScript/action=raw) do
not pay attention to the content model. So content is executable regardless
of the content model. The content model is merely a meaningless front-end
flag at the moment for having a pretty editor, source highlighting (and in
theory syntax validation).

I'm not too concerned about that, since attack is impossible in that scenario unless a user deliberately weakens their own security. Also what Legoktm said.

Created attachment 17009
Check css/js permissions on content type change

I didn't see a good way to check title permissions for an alternate content type in the existing Title class, so this patch starts what I'm intending to be longer-term work, pulling permission checking out of the Title class, so we can call the checks in other parts of the code.

There's some duplicated logic, and any extensions that implement userCan/getUserPermissionsErrors, and make a decision based on content type won't get to give input. But I'd like to get this specific case fixed up so we can have a larger discussion about how to handle updating content types going foward.

Daniel Kinzler, I'd especially appreciate your thoughts on if there is a better approach, since this is closely tied to a lot of your work.

attachment bug70901.patch ignored as obsolete

(from attachment 17009)
+ public static function checkCSSandJSPermissions( $title, $action, $user, $errors,
+ $isCssSubpage = null, $isJsSubpage = null
+ ) {

(from attachment 17009)
+ ) {
+ # Protect css/js subpages of user pages
+ # XXX: this might be better using restrictions
+ # XXX: right 'editusercssjs' is deprecated, for backward compatibility only
+ if ( $isCssSubpage === null ) {

These three lines of comment were moved from another part of the code. I think they belong to the following code however:

(from attachment 17009)
+
+ if ( $action != 'patrol' && !$user->isAllowed( 'editusercssjs' ) ) {

(from attachment 17009)
+ public static function checkCSSandJSPermissions( $title, $action, $user, $errors,
+ $isCssSubpage = null, $isJsSubpage = null
+ ) {

Hm... boolean parameters.

(from attachment 17009)
+ ) {
+ # Protect css/js subpages of user pages
+ # XXX: this might be better using restrictions
+ # XXX: right 'editusercssjs' is deprecated, for backward compatibility only
+ if ( $isCssSubpage === null ) {

These three lines of comment were moved from another part of the code. I think they belong to the following code however:

(from attachment 17009)
+
+ if ( $action != 'patrol' && !$user->isAllowed( 'editusercssjs' ) ) {

I notice that the patch makes the (currently correct) assumption that the content model can only be changed through the API. I don't think that'll always be the case, though. Can we worry about that later when/if we change it, or should we worry about it now?

(In reply to Jackmcbarn from comment #10)

I notice that the patch makes the (currently correct) assumption that the
content model can only be changed through the API. I don't think that'll
always be the case, though. Can we worry about that later when/if we change
it, or should we worry about it now?

I'd like to fix the current issue with as minimal of a patch as we can, then have a broader discussion on wikitech-l to make sure the assumptions behind $wgContentHandlerUseDB are sound.

Okay, sounds good.

Created attachment 17072
Check css/js permissions on content type change

Updated for Krinkle's comments

attachment bug70901b.patch ignored as obsolete

@Chris: can you put that patch on gerrit, so we can comment inline?

To recap the two scenarios originally described by Jackmcbarn:

"it's possible for a user to change the content model of a page in another
user's userspace to CSS or JS."

  1. "One is that a user could be a nuisance by mass-changing the pages, since once they're changed like that, only the userspace's owner or an admin can fix them."
  1. "Another is that if a user accidentally sets their common.js to wikitext, a malicious user could add bad code to it and change its content model back to JS, so it would get executed."

The brackground is that with $wgContentHandlerUseDB set to true, the content model attributed to subpages in the User namespace is not dictated by the ".js" and ".css" extensions; the default model can then be overwritten when creating or editing a page (via the API).

From what I can see (I have not tested), attachment 17072 will prevent unauthorized users to create or edit user-subpages with content model JS or CSS. This should fix both scenarios.

While that would definitely be an improvement, I see some remaining issues:

  1. as far as I can tell, any user can change the content model of a user JS/CSS page to something else, like wikitext, effectively but silently "disabling" these pages (if RL indeed ignored subpages with the "wrong" model). Or worse, it would lead to "active" JS/CSS pages editable by anyone.
  1. with $wgContentHandlerUseDB off, wouldn't it be possible for an attacker to move an arbitrary page to User:Foo/Common.js, making it an active user JS page? I have not tried, but even if it's not possible in the most obvious way, we should safeguard against this "deeper" in the code.

Basically: with $wgContentHandlerUseDB enabled, editing can change the content model (and thus the permissions that apply). With $wgContentHandlerUseDB disabled, moving a page can change the content model (and thus permissions). Permisswion checks are currently based on the current model, not the "targeted" model.

As Jackmcbarn suggests, this should be addressed by always checking permissions for the old *and* the new state of the page (Chris' current patch only checks the new state, as far as I can see, allowing the model to be changed "away" from JS/CSS).

This is further complicated by the fact that Title::userCan for a non-existing page will check against the title's default model, which may not be the model actually being used to create the page.

So, we'll have to be smarter when checking permissions, and I agree this logic could go into a TitlePermissionChecks class (which ideally would operate on TitleValue objects, but let's think about that later). However, I feel that the checkCSSandJSPermissions() method proposed by Chris exposes too much of the internal logic of checking permissions. The calling code shouldn't need to know about special cases for JS/CSS pages. Instead, there should be methods that are only slightly more specific than the current Title::userCan:

  • TitlePermissionChecks::userCan( $title, $action ) // for completeness
  • TitlePermissionChecks::userCanEdit( $title, $newModel )
  • TitlePermissionChecks::userCanMove( $title, $newTitle, $newModel )
  • ...

Any special logic for JS/CSS would be hidden in these methods.

Note that $user is missing from the signatures, because, in my thinking, the TitlePermissionChecks object would know the user it checks permissions for (and would also know whether "expensive checks" should be done).

(In reply to Daniel Kinzler from comment #14)

  1. as far as I can tell, any user can change the content model of a user JS/CSS page to something else, like wikitext, effectively but silently "disabling" these pages (if RL indeed ignored subpages with the "wrong" model). Or worse, it would lead to "active" JS/CSS pages editable by anyone.

We're safe against this, since we test against the old and new model.

  1. with $wgContentHandlerUseDB off, wouldn't it be possible for an attacker to move an arbitrary page to User:Foo/Common.js, making it an active user JS page? I have not tried, but even if it's not possible in the most obvious way, we should safeguard against this "deeper" in the code.

We're safe against this, since with $wgContentHandlerUseDB off, nobody (not even admins) can move a wikitext page to JS or vice versa.

As Jackmcbarn suggests, this should be addressed by always checking
permissions for the old *and* the new state of the page (Chris' current
patch only checks the new state, as far as I can see, allowing the model to
be changed "away" from JS/CSS).

It was already checking the old state, and he added a check for the new state.

I don't think there's any remaining security issues with this patch.

(In reply to Daniel Kinzler from comment #14)

@Chris: can you put that patch on gerrit, so we can comment inline?

Sadly no. Gerrit can't do private patches :(. The glorious phabricator phuture has some hope...

To recap the two scenarios originally described by Jackmcbarn:

"it's possible for a user to change the content model of a page in another
user's userspace to CSS or JS."

  1. "One is that a user could be a nuisance by mass-changing the pages, since once they're changed like that, only the userspace's owner or an admin can fix them."
  2. "Another is that if a user accidentally sets their common.js to wikitext, a malicious user could add bad code to it and change its content model back to JS, so it would get executed."

    The brackground is that with $wgContentHandlerUseDB set to true, the content model attributed to subpages in the User namespace is not dictated by the ".js" and ".css" extensions; the default model can then be overwritten when creating or editing a page (via the API).

    From what I can see (I have not tested), attachment 17072 [details] will prevent unauthorized users to create or edit user-subpages with content model JS or CSS. This should fix both scenarios.

    While that would definitely be an improvement, I see some remaining issues:
  3. as far as I can tell, any user can change the content model of a user JS/CSS page to something else, like wikitext, effectively but silently "disabling" these pages (if RL indeed ignored subpages with the "wrong" model). Or worse, it would lead to "active" JS/CSS pages editable by anyone.

That shouldn't be possible (as Jackmcbarn said), for existing pages. E.g., on every edit ApiEditPage does:

$errors = $titleObj->getUserPermissionsErrors( 'edit', $user );

For editing a user's common.js, $titleObj has it's content type set to Javascript, and is a subpage, so Title::isJsSubpage() is true, and checkCSSandJSPermissions won't let other users edit the page, including changing the content type.

  1. with $wgContentHandlerUseDB off, wouldn't it be possible for an attacker to move an arbitrary page to User:Foo/Common.js, making it an active user JS page? I have not tried, but even if it's not possible in the most obvious way, we should safeguard against this "deeper" in the code.

    Basically: with $wgContentHandlerUseDB enabled, editing can change the content model (and thus the permissions that apply). With $wgContentHandlerUseDB disabled, moving a page can change the content model (and thus permissions). Permisswion checks are currently based on the current model, not the "targeted" model.

    As Jackmcbarn suggests, this should be addressed by always checking permissions for the old *and* the new state of the page (Chris' current patch only checks the new state, as far as I can see, allowing the model to be changed "away" from JS/CSS).

    This is further complicated by the fact that Title::userCan for a non-existing page will check against the title's default model, which may not be the model actually being used to create the page.

This definitely concerns me. In this specific case, ContentHandler::getDefaultModelFor will correctly identify common.js as a javascript file ( "preg_match( '!\.(css|js)$!u', $title->getText()" ), but it seems too easy to miss another case.

For other files that might be included as javascript, we should correctly mark them as javascript content type, and make sure that loading them fails (I think we already handle that correctly) if they are changed to anything else. Further restricting which namespaces can have javascript content types I think would help.. but that's part of the larger discussion I'd like to have once this particular issue is patched.

So, we'll have to be smarter when checking permissions, and I agree this
logic could go into a TitlePermissionChecks class (which ideally would
operate on TitleValue objects, but let's think about that later). However, I
feel that the checkCSSandJSPermissions() method proposed by Chris exposes
too much of the internal logic of checking permissions. The calling code
shouldn't need to know about special cases for JS/CSS pages. Instead, there
should be methods that are only slightly more specific than the current
Title::userCan:

  • TitlePermissionChecks::userCan( $title, $action ) // for completeness
  • TitlePermissionChecks::userCanEdit( $title, $newModel )
  • TitlePermissionChecks::userCanMove( $title, $newTitle, $newModel )
  • ...

I had started out trying to implement TitlePermissionChecks::userCan, but with just TitleValue, all of the logic for handling a Title's content model, and whether it's a subpage had to get pulled either into TitleValue, TitlePermissionChecks, or another new abstraction, which was a larger rewrite than I wanted to do in a private security patch.

Something like TitlePermissionChecks::userCanEdit( $title, $newModel ) should be possible, although keeping it specific to content model checking (userCanEditModel, or something like that) feels appropriate to me since changing the content model should be a relatively rare case?

Any special logic for JS/CSS would be hidden in these methods.

Note that $user is missing from the signatures, because, in my thinking, the
TitlePermissionChecks object would know the user it checks permissions for
(and would also know whether "expensive checks" should be done).

@Chris: Please do not bind permissions (only) to the content model. Tehre should not be any extra restrictions on editing pages with the Javascript content model. There *should* be extra restrictions on executable ("active") content. Identifying executable content involves looking at the content model, but it's not generally determined by the content model.

I agree that basing TitlePermissionChecks on TitleValue would need quite a bit of refactoring, which should be done only after a broader discussion.

PS: Gerrit can do private patches: use the "draft" feature. Be careful to *always* use the -D option with git review then, though.

(In reply to Daniel Kinzler from comment #17)

PS: Gerrit can do private patches: use the "draft" feature. Be careful to
*always* use the -D option with git review then, though.

Unfortunately this is a common myth. Gerrit's ssh server doesn't respect the draft marker, and will happily give you draft changes if you ask it the right way.

Created attachment 17123
Add user right to change the content model of a page

This adds a new user right, and requires it for changing the content model of a page.

For pages without extra rights (User css/js subpages), this will still allow a user to move a page with one content model to another name. But there shouldn't be a way to escalate privileges from that.

attachment bug70901-newright.patch ignored as obsolete

(In reply to Chris Steipp from comment #19)

Created attachment 17123 [details]
Add user right to change the content model of a page

This adds a new user right, and requires it for changing the content model
of a page.

For pages without extra rights (User css/js subpages), this will still allow
a user to move a page with one content model to another name. But there
shouldn't be a way to escalate privileges from that.

I don't see a point to doing this. It seems to impair usability for no good reason.

attachment bug70901-newright.patch ignored as obsolete

(In reply to Jackmcbarn from comment #20)

I don't see a point to doing this. It seems to impair usability for no good
reason.

This came out of an internal email discussion, let me try to summarize it:

Robla initially flagged this for product review because of the broader effects of doing this. The major concerns he brought up were security (like this bug), that reverting them is non-trivial ("undo" throws an exception, bug 71163), and that the diff handling doesn't really have a way to indicate that the content model is changing.

In conversation, Robla and I have also talked about the lack of spam tool integration, and effect on product if, e.g., users revert Flow talk pages back to wikitext.

From products perspective (James F, Danny Horn), the ability to change a page's content type is something we're planning to use for Flow and maybe the Graph extension, so the flag needs to be enabled on wmf sites by next month.

In the end, James F, Robla, and Daniel Kinzler, and I basically agreed that adding a right (which would be given to a relatively small number of accounts) mitigates most of the the current issues, allows the current plans for Flow to move forward, and let's us get into a position where we can safely publicly discuss the more complex issues.

Anyone else involved, please speak up if I'm misrepresenting the views you shared via email.

(In reply to Chris Steipp from comment #21)

(In reply to Jackmcbarn from comment #20)
> I don't see a point to doing this. It seems to impair usability for no good
> reason.

This came out of an internal email discussion, let me try to summarize it:

[Snip]

Anyone else involved, please speak up if I'm misrepresenting the views you
shared via email.

LGTM.

(In reply to Chris Steipp from comment #21)

Robla initially flagged this for product review because of the broader
effects of doing this. The major concerns he brought up were security (like
this bug), that reverting them is non-trivial ("undo" throws an exception,
bug 71163), and that the diff handling doesn't really have a way to indicate
that the content model is changing.

I think we should handle each of those issues the "right" way.

In conversation, Robla and I have also talked about the lack of spam tool
integration, and effect on product if, e.g., users revert Flow talk pages
back to wikitext.

How does this relate to spam tool integration? Also, doesn't Flow have hardcoded rules for changing its content model?

In the end, James F, Robla, and Daniel Kinzler, and I basically agreed that
adding a right (which would be given to a relatively small number of
accounts) mitigates most of the the current issues, allows the current plans
for Flow to move forward, and let's us get into a position where we can
safely publicly discuss the more complex issues.

I guess that would be fine as a stopgap, but it should be made clear that the right will go away once all of the issues are properly addressed.

I'm planning to deploy the permission this week, and the patch will go out with the 1.23.7 release (sometime next week, I should have the exact date soon). Then we can discuss broader issues publicly.

Brad, I didn't realize you weren't already on this bug. Does this plan make sense from your perspective?

(In reply to Jackmcbarn from comment #10)

I notice that the patch makes the (currently correct) assumption that the
content model can only be changed through the API.

That assumption is not correct. The web UI has hidden fields for model and format, and changing these to appropriate values will save the page with the specified model and format.

This snippet entered in the browser's JS console should reveal the fields (works in Firefox anyway; may blow up in IE):

$( 'input[name=model], input[name=format]' ).attr( 'type', 'text' );

(In reply to Chris Steipp from comment #24)

Brad, I didn't realize you weren't already on this bug. Does this plan make
sense from your perspective?

Requiring a permission to change content type (at least temporarily) seems sane, if it catches all the relevant code paths. Do we need to worry about someone moving their own common.js to other places as a way to spam content-JS pages into someone else's user space?

For the other patch (attachment #17072), I don't really like the duplication of the "isCssSubpage" and "isJsSubpage" logic inside ApiEditPage.

(In reply to Brad Jorsch from comment #25)

(In reply to Jackmcbarn from comment #10)
> I notice that the patch makes the (currently correct) assumption that the
> content model can only be changed through the API.

That assumption is not correct. The web UI has hidden fields for model and
format, and changing these to appropriate values will save the page with the
specified model and format.

This snippet entered in the browser's JS console should reveal the fields
(works in Firefox anyway; may blow up in IE):

$( 'input[name=model], input[name=format]' ).attr( 'type', 'text' );

Thanks for pointing that out Brad, I missed that. I updated the patch to do the check in EditPage, which seems like the last place any permissions are checked.

(In reply to Chris Steipp from comment #24)
> Brad, I didn't realize you weren't already on this bug. Does this plan make
> sense from your perspective?

Requiring a permission to change content type (at least temporarily) seems
sane, if it catches all the relevant code paths. Do we need to worry about
someone moving their own common.js to other places as a way to spam
content-JS pages into someone else's user space?

The target name of the move couldn't end in .js, but yeah, someone could put pages into your user space that had a javascript content model. The user would be able to edit them, while the person moving them in couldn't after the move (without edituserjs rights).

I can't think of a way to exploit that unless some existing javascript tries to load a (non .js-named) page from the user's namespace and executed it as javascript. Or did you have anything else in mind?

Comment on attachment 17072
Check css/js permissions on content type change

Let's do this as a larger refactor of Title, and do it publicly.

Created attachment 17165
Add user right and require it to change the content model of a page

Attached:

(In reply to Chris Steipp from comment #26)

I can't think of a way to exploit that unless some existing javascript tries
to load a (non .js-named) page from the user's namespace and executed it as
javascript. Or did you have anything else in mind?

Just the annoyance of being able to shove pages into someone's userspace that can't be reverted or edited by anyone other than the user involved or admins.

(In reply to Chris Steipp from comment #28)

Created attachment 17165 [details]
Add user right and require it to change the content model of a page

+1: Looks good, haven't tested.

Attached:

This is a little orthogonal, but related, and I just want to put it on the radar:

With the Flow content models, the page is not purely one blob of text.

It doesn't make sense to simply mark that a wikitext talk page is now Flow. The transition is more complicated than that.

So initially we are probably not going to allow changing the content model of an existing page to Flow using the standard ContentHandler API.

However, we will probably soon allow creating new Flow boards with the normal ContentHandler mechanism, as long as you have the proper permission.

to follow up matt, within Flow we are implementing additional restrictions through the ContentHandler::canBeUsedOn method which is checked on every call to WikiPage::doEditContent.

The Current implementation checks a whitelist defined in mediawiki-config, but we've been wanting to get rid of that for some time. To get rid of the whitelist requires $wgContentHandlerUseDB to be enabled.

With the whitelist removed the implementation will change, our current gerrit patch involves creating a bit of shared state between the content handler and a special page designed for the task of 'activating' flow pages. Specifically the new implementation only allows saving a flow-board content type if the content type is already flow-board, or if the special page has explicitly allowed it during the current script run. The special page will have some new right attached to it, our first user will be a bot which will go through standard channels to have rights enabled.

(In reply to Brad Jorsch from comment #29)

+1: Looks good, haven't tested.

Thanks Brad. Could you (or anyone else) review it for deployment, and I'll push it out tomorrow?

(In reply to Chris Steipp from comment #32)

Thanks Brad. Could you (or anyone else) review it for deployment, and I'll
push it out tomorrow?

Looks good to deploy, manual testing passed.

Restricted Application changed the visibility from "Security (Project)" to "Custom Policy". · View Herald TranscriptNov 23 2014, 8:22 PM
Restricted Application changed the edit policy from "Security (Project)" to "Custom Policy". · View Herald Transcript
Restricted Application added a project: Security. · View Herald Transcript

Deployed

on the cluster. This should be backported for the 1.23.7 release tomorrow.

Once this is public, we'll start a public conversation on wikitech-l about how we want to do content type updates in the future. I'll also start breaking permission checking out of Title in a public patch, so we'll have a little more flexibility in how we do those checks in the future.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptNov 25 2014, 6:32 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptNov 25 2014, 6:33 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
chasemp changed the visibility from "Custom Policy" to "Custom Policy".Nov 25 2014, 9:07 PM
chasemp changed Security from Software security bug to None.
csteipp changed the visibility from "Custom Policy" to "Custom Policy".Nov 25 2014, 10:13 PM
csteipp changed the visibility from "Custom Policy" to "Custom Policy".Nov 26 2014, 5:57 AM
csteipp added a subscriber: Grunny.

Ok, here are the backports for REL1_23 and REL1_22.

The patch applies to REL1_24 without the need for a backport. There is no content handler in REL1_19, so no backport needed.

Can anyone confirm the two backports?

csteipp changed the visibility from "Custom Policy" to "Custom Policy".Nov 26 2014, 6:13 PM
csteipp added a subscriber: ProgramCeltic.

Tested editing MediaWiki:Common.css after applying the patch in REL1_24, REL1_23 and REL1_22

Ok, here are the backports for REL1_23 and REL1_22.

The patch applies to REL1_24 without the need for a backport. There is no content handler in REL1_19, so no backport needed.

Can anyone confirm the two backports?

Confirmed the backports prevent changing the content model

Since the fix for this is now fully public, can this bug be marked public?

csteipp changed the visibility from "Custom Policy" to "Public (No Login Required)".
csteipp changed the edit policy from "Custom Policy" to "All Users".

@csteipp is this still Open? FYI the fix is gerrit 176160.

csteipp closed this task as Resolved.Dec 5 2014, 5:59 PM

Change 196982 had a related patch set uploaded (by Legoktm):
Don't check content model in Title::isCssSubpage() and Title::isJsSubpage()

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

Restricted Application added a subscriber: Luke081515. · View Herald TranscriptJan 25 2016, 8:10 AM

(In reply to Chris Steipp from comment #2)

On the vandalism side, this definitely needs a lot more testing before we
enable $wgContentHandlerUseDB on more wikis. Rollback seems to work ok, but
undo seems broken when undoing revisions that change the content type.

I haven't looked at this in detail.

Robla initially flagged this for product review because of the broader effects of doing this. The major concerns he brought up were security (like this bug), that reverting them is non-trivial ("undo" throws an exception, bug 71163), and that the diff handling doesn't really have a way to indicate that the content model is changing.

In addition to T73163, there was T145044/T106195.

In the end, James F, Robla, and Daniel Kinzler, and I basically agreed that adding a right (which would be given to a relatively small number of accounts) mitigates most of the the current issues, allows the current plans for Flow to move forward, and let's us get into a position where we can safely publicly discuss the more complex issues.

Expanding the "changecontentmodel" user right to more users is currently being discussed at T85847.

Expanding the "changecontentmodel" user right to more users is currently being discussed at T85847.

Err, the "editcontentmodel" user right. The Special page is named Special:ChangeContentModel. :-)

(In reply to Chris Steipp from comment #26)

I can't think of a way to exploit that unless some existing javascript tries
to load a (non .js-named) page from the user's namespace and executed it as
javascript. Or did you have anything else in mind?

Just the annoyance of being able to shove pages into someone's userspace that can't be reverted or edited by anyone other than the user involved or admins.

I've filed this separately at T145316: It is possible to create a different user's subpage with a JavaScript or CSS content model, so only that user or a sysop can edit or revert.