Page MenuHomePhabricator

Verify that the MediaWiki API is not leaking Oversighted edit summaries within CheckUser results (CVE-2019-18611)
Closed, ResolvedPublic

Description

Per, @Umherirrender's comment at T207094#5553504: we need to verify whether or not the API is leaking the same information as the issue within the parent task and, if it is, create and deploy a security patch.

Details

Related Gerrit Patches:

Event Timeline

sbassett created this task.Oct 7 2019, 9:23 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 7 2019, 9:23 PM
sbassett triaged this task as Normal priority.Oct 7 2019, 9:25 PM
sbassett added subscribers: Daimona, Reedy, Anomie, Urbanecm.
Restricted Application added a project: Core Platform Team. · View Herald TranscriptOct 7 2019, 9:26 PM
sbassett updated the task description. (Show Details)Oct 7 2019, 9:26 PM
WDoranWMF added a subscriber: WDoranWMF.

@Daimona or @Urbanecm Would you have any thoughts on investigating this?

Urbanecm claimed this task.Oct 8 2019, 10:01 PM
Restricted Application added a project: User-Urbanecm. · View Herald TranscriptOct 8 2019, 10:01 PM

Basically duplicated my code from the previous patch to the API module, thanks for spotting this. Not sure how to incorporate @Umherirrender's edits made in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/541606, any suggestions?

Urbanecm moved this task from Backlog to Working on on the User-Urbanecm board.Oct 11 2019, 11:39 AM

@Urbanecm -

  1. + 1 to the patch above (T234862#5558854) as-is. It mostly mirrors the patch for T207094, which should be fine.
  2. To incorporate @Umherirrender's changes from r541606, I think we could do something like this in includes/api/ApiQueryCheckUser.php. Once r541606 is merged, we could maybe make loadRevisionRecords() static, add the appropriate use statements and do:
// around line 112 w/ T234862#5558854 applied
$revisionRecord = SpecialCheckUser::loadRevisionRecords( $result )
// around lines 126 to 149 w/ T234862#5558854 applied
$revisionRecord = $revisionRecords[$row->cuc_this_oldid] ?? false;
if ( $revisionRecord !== false &&
    $revisionRecord->audienceCan(
        RevisionRecord::DELETED_COMMENT, RevisionRecord::FOR_THIS_USER, $this->getUser()
    )
)

We might just want to get this security-deployed as-is for now (with the patch from T234862#5558854) and push the improvements through gerrit as is being done for the parent patch, as I believe this version of the code shouldn't have any performance issues.

@Urbanecm - did you want to deploy your patch from T234862#5558854? If not, I'm happy to deploy it on Monday (2019-10-21) during the security deployment window.

sbassett moved this task from Backlog to Waiting on the user-sbassett board.

i can do so :)

Patch was deployed today.

Patch was deployed today.

Thanks, I'll request a CVE soon. Also tracking in T234983.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Mon, Oct 28, 9:24 PM

Change 546741 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@master] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546742 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@REL1_34] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546743 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@REL1_33] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546745 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@REL1_32] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546746 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546746 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546745 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_32] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546742 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_34] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546743 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_33] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

Change 546741 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] SECURITY: Do not show oversighted edit summaries in CheckUser API

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

sbassett added a comment.EditedMon, Oct 28, 9:53 PM

Backports to supported release branches merged, CVE requested. I'll update this task and T234983 once I have the CVE ID.

sbassett renamed this task from Verify that the MediaWiki API is not leaking Oversighted edit summaries within CheckUser results to Verify that the MediaWiki API is not leaking Oversighted edit summaries within CheckUser results (CVE-2019-18611).Tue, Oct 29, 3:48 PM
sbassett closed this task as Resolved.Tue, Oct 29, 3:50 PM
sbassett moved this task from Waiting to Done on the user-sbassett board.Tue, Oct 29, 3:52 PM
sbassett removed a project: user-sbassett.