Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F57536770
T372998-REL1_41.patch
acl*security
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Authored By
Reedy
Sep 25 2024, 2:07 PM
2024-09-25 14:07:48 (UTC+0)
Size
4 KB
Referenced Files
None
Subscribers
None
T372998-REL1_41.patch
View Options
From 9d19efba7a3e0ad712eff9c53fa511bf1da93e94 Mon Sep 17 00:00:00 2001
From: Dreamy Jazz <wpgbrown@wikimedia.org>
Date: Wed, 11 Sep 2024 18:20:49 +0100
Subject: [PATCH] abusefiltercheckmatch: Check if user can see log details
Why:
* The 'abusefiltercheckmatch' API allows callers to match
arbitary filter conditions against existing AbuseFilter logs
* The API does not check if the performer has the ability to
see the log details for the given filter, so can allow a user
to bypass hidden and protected visibility settings.
What:
* Call AbuseFilterPermissionManager::canSeeLogDetailsForFilter
before attempting to match a filter against a given AbuseFilter
log.
* Add a test to verify that this security fix works.
Bug: T372998
Change-Id: I4a2467dc4e0d1f8401d5428a89c7f6d6ebcdfa70
---
includes/Api/CheckMatch.php | 10 +++++
.../integration/Api/CheckMatchTest.php | 37 +++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/includes/Api/CheckMatch.php b/includes/Api/CheckMatch.php
index 7b6aa0ef..f82c382f 100644
--- a/includes/Api/CheckMatch.php
+++ b/includes/Api/CheckMatch.php
@@ -10,6 +10,7 @@ use LogEventsList;
use LogicException;
use LogPage;
use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager;
+use MediaWiki\Extension\AbuseFilter\AbuseFilterServices;
use MediaWiki\Extension\AbuseFilter\Parser\RuleCheckerFactory;
use MediaWiki\Extension\AbuseFilter\Special\SpecialAbuseLog;
use MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGeneratorFactory;
@@ -113,6 +114,15 @@ class CheckMatch extends ApiBase {
$this->dieWithError( [ 'apierror-abusefilter-nosuchlogid', $params['logid'] ], 'nosuchlogid' );
}
+ // TODO: Replace with dependency injection once security patch is uploaded publicly.
+ $afFilterLookup = AbuseFilterServices::getFilterLookup();
+ $privacyLevel = $afFilterLookup->getFilter( $row->afl_filter_id, $row->afl_global )
+ ->getPrivacyLevel();
+ $canSeeDetails = $this->afPermManager->canSeeLogDetailsForFilter( $performer, $privacyLevel );
+ if ( !$canSeeDetails ) {
+ $this->dieWithError( 'apierror-permissiondenied-generic', 'cannotseedetails' );
+ }
+
$visibility = SpecialAbuseLog::getEntryVisibilityForUser( $row, $performer, $this->afPermManager );
if ( $visibility !== SpecialAbuseLog::VISIBILITY_VISIBLE ) {
// T223654 - Same check as in SpecialAbuseLog. Both the visibility of the AbuseLog entry
diff --git a/tests/phpunit/integration/Api/CheckMatchTest.php b/tests/phpunit/integration/Api/CheckMatchTest.php
index 20faccbe..9877708b 100644
--- a/tests/phpunit/integration/Api/CheckMatchTest.php
+++ b/tests/phpunit/integration/Api/CheckMatchTest.php
@@ -4,16 +4,23 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Integration\Api;
use ApiTestCase;
use FormatJson;
+use MediaWiki\Extension\AbuseFilter\AbuseFilterServices;
+use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter;
+use MediaWiki\Extension\AbuseFilter\Filter\Flags;
+use MediaWiki\Extension\AbuseFilter\FilterLookup;
use MediaWiki\Extension\AbuseFilter\Parser\Exception\InternalException;
use MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator;
use MediaWiki\Extension\AbuseFilter\Parser\ParserStatus;
use MediaWiki\Extension\AbuseFilter\Parser\RuleCheckerFactory;
use MediaWiki\Extension\AbuseFilter\Parser\RuleCheckerStatus;
+use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait;
+use MediaWiki\Title\Title;
/**
* @coversDefaultClass \MediaWiki\Extension\AbuseFilter\Api\CheckMatch
* @covers ::__construct
+ * @group Database
* @group medium
*/
class CheckMatchTest extends ApiTestCase {
@@ -97,4 +104,34 @@ class CheckMatchTest extends ApiTestCase {
] );
}
+ public function testExecuteWhenPerformerCannotSeeLogId() {
+ // Mock the FilterLookup service to return that the filter with the ID 1 is hidden.
+ $mockLookup = $this->createMock( FilterLookup::class );
+ $mockLookup->method( 'getFilter' )
+ ->with( 1, false )
+ ->willReturnCallback( function () {
+ $filterObj = $this->createMock( ExistingFilter::class );
+ $filterObj->method( 'getPrivacyLevel' )->willReturn( Flags::FILTER_HIDDEN );
+ return $filterObj;
+ } );
+ $this->setService( FilterLookup::SERVICE_NAME, $mockLookup );
+ // Create an AbuseFilter log entry for the hidden filter
+ AbuseFilterServices::getAbuseLoggerFactory()->newLogger(
+ Title::newFromText( 'Testing' ),
+ $this->getTestUser()->getUser(),
+ VariableHolder::newFromArray( [ 'action' => 'edit' ] )
+ )->addLogEntries( [ 1 => [ 'warn' ] ] );
+ // Execute the API using a user with the 'abusefilter-modify' right but without the
+ // 'abusefilter-log-detail' right, while specifying a filter abuse filter log ID of 1
+ $this->expectApiErrorCode( 'cannotseedetails' );
+ $this->doApiRequest(
+ [
+ 'action' => 'abusefiltercheckmatch',
+ 'logid' => 1,
+ 'filter' => 'invalidfilter=======',
+ ],
+ null, false, $this->mockRegisteredAuthorityWithPermissions( [ 'abusefilter-modify' ] )
+ );
+ }
+
}
--
2.34.1
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
18868188
Default Alt Text
T372998-REL1_41.patch (4 KB)
Attached To
Mode
T372998: CVE-2024-47913: abusefiltercheckmatch does not check the user for the abusefilter-log-detail right before matching against log details
Attached
Detach File
Event Timeline
Reedy
changed the visibility from "
Reedy (Sam Reed)
" to "
acl*security
(Project)".
Sep 25 2024, 2:08 PM
2024-09-25 14:08:24 (UTC+0)
Log In to Comment