Page MenuHomePhabricator

Only export pages the user can read
Closed, ResolvedPublic

Description

Author: fernandoacorreia

Description:
Special:Export doesn't check whether the user has the rights to read a page that
is being exported. This can be used to get access to pages that should not be
available to that user.

The proposed patch verifies if the user can read the page and does not export
pages that are not allowed to that user.


Version: 1.9.x
Severity: normal
OS: Windows XP
Platform: PC

Details

Reference
bz8824

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 9:35 PM
bzimport set Reference to bz8824.
bzimport added a subscriber: Unknown Object (MLST).

fernandoacorreia wrote:

does not export pages the user can't read

Index: Export.php

  • Export.php (revision 19680)

+++ Export.php (working copy)
@@ -110,6 +110,9 @@

  • @param Title $title
	 */

function pageByTitle( $title ) {
+ if (!$title->userCanRead()) {
+ return;
+ }

		return $this->dumpFrom(
			'page_namespace=' . $title->getNamespace() .
			' AND page_title=' . $this->db->addQuotes(

$title->getDbKey() ) );

Attached:

ayg wrote:

When would this possibly come up? When Special:Export is whitelisted?

fernandoacorreia wrote:

This would come up when the userCan hook was used to prevent read access to some
pages according to the groups the user belongs to.

while i think it would indeed be good to check access when exporting, the
simplest workaround would indded be to just deny ordinary users access to
Special:Export.

fernandoacorreia wrote:

I understand. But let's assume someone wants to hook userCan so some users can
see some pages, other users can see other pages, and it is still desired that
the users can export the pages they have read access to.

This patch would really help to close a breach that troubles security
extensions. If there is a problem with the patch I'm willing to work it out. Do
you think a hook would be better?

ayg wrote:

Well, I don't see any harm in this patch, I guess, but realize that MediaWiki is not configured
for granular read security and if you try to add it you'll have to plug a million holes like this.

A hook is probably a better idea.

fernandoacorreia wrote:

You're right, but each hole that is plugged is one less hole. Actually it
doesn't seem there are so many holes. The code base already calls "userCanRead"
and "userCanEdit" in a lot of places.

I suggest we try to extend its use to a few additional places such as the page
export.

Someone would have to decide between calling userCanRead and calling a hook that
will either do nothing (if not defined) or call userCanRead. I think the first
option is more clear and more consistent with the rest of the code base. Only if
there is a significant performance impact I would substitute it for a hook.

MediaWiki is open-access by design; Special:Export and other tools must not be
whitelisted on a closed-access wiki.

Fixed in r19935. Beware however that other loopholes to read-restrictions exist
(such as including as a template, which I addressed in r19934). Mediawiki will
probably not support air-tight read-restrictions any time soon. But we *can*
plug the large and easy holes, I guess.

I give my reluctant blessing to this experiment.

*waves hands mystically*

robchur wrote:

Where are we going, and what's with the handbasket?

fernandoacorreia wrote:

That is great news. Thank you all! I agree that MediaWiki core doesn't need to
implement advanced access control. Extensions may try and handle that. All I ask
is some hooks and basic checks like this one to make these extensions simpler
and more reliable.