I presume the backport patches need to be +2'd. If so, I'll look at doing that tomorrow UTC unless someone else gets there first.

In T311337#8190463, @Dreamy_Jazz wrote:

I presume the backport patches need to be +2'd. If so, I'll look at doing that tomorrow UTC unless someone else gets there first.

Security Team will handle that with the release. No need to worry.

@Zabe @Dreamy_Jazz et al - the patch from T311337#8178549 was deployed today:

2022-09-12 - 19:53 sbassett: Deployed security patch for T311337

It applied fine and didn't appear to cause any errors on any related CheckUser pages (at least not according to logstash). @Mstyles and I weren't really able to thoroughly test the patch in production though, as we don't have os anywhere. So if someone else could test a bit more and confirm that the issue does appear to be resolved within production, that would be great. If the patch does not appear to have solved the issue completely, we can always pull it off of production, test a bit more and deploy a rev 2. Also now tracking at T276237 (was already tracked at T311785).

Nice.

What have you not been able to test on production wikis? I have CU and OS on enwiki. Through a test have been able to verify that the performer still shows to OS'ers when suppressed (which is expected). By extension I was also able to verify that it does not throw any errors visible to the user. If looking for the check in log stash to check backend errors, I added the ticket number for this task in the reason for the check.

Can I have access to T276237 (if it's general purpose and not specific to CheckUser then it's fine)?

T276237 is the list of currently deployed security patches. Is there a reason you want access to that list?

I wasn't sure by the comment above whether that was specific to CheckUser (and so somewhere I might be able to help) or not. As it's just a general purpose ticket for deployed security patches I don't want access. Thanks.

In T311337#8230821, @Dreamy_Jazz wrote:

I wasn't sure by the comment above whether that was specific to CheckUser (and so somewhere I might be able to help) or not. As it's just a general purpose ticket for deployed security patches I don't want access. Thanks.

No problem. Access to T276237 is really only necessary if you're helping out with wikimedia-deployed security patches or are helping out with the quarterly MediaWiki security releases.

What have you not been able to test on production wikis? I have CU and OS on enwiki. Through a test have been able to verify that the performer still shows to OS'ers when suppressed (which is expected). By extension I was also able to verify that it does not throw any errors visible to the user. If looking for the check in log stash to check backend errors, I added the ticket number for this task in the reason for the check.

Well, nobody on the Security-Team currently has OS on any of the production projects, so not much. It sounds like all we'd still want to confirm is that an OS'd target is now hidden to non-OS CUs on a production project when viewing related data during a CU check. If someone can create a suppressed edit like the one you describe in the task on testwiki, then I can run a CU against the user/IP and confirm I cannot view the target (I have plain old CU on testwiki). Thanks.

I don't think anyone has OS on test.wikipedia.org currently but based on searching the user rights log Martin Urbanec had OS rights back in February this year. Not sure the process of getting OS on test wikis but they may be someone to contact.

@Urbanecm for the above, as you are already a subscriber here. Perhaps you could set up this edit?

In T311337#8233066, @Dreamy_Jazz wrote:

@Urbanecm for the above, as you are already a subscriber here. Perhaps you could set up this edit?

It doesn't have to be testwiki, that was just convenient for me :) Looks like most enwiki CUs have OS, but there are a few that do not, if we could persuade one of them to help test there.

I'll wait for Urbancem's response, but otherwise GeneralNotability is an enwiki CU but not OS that I think would be happy to help.

@sbassett I have coordinated with a enwiki CU who is not an OS and followed the steps:

1. Had them make a test edit (in this case one in their sandbox)
2. Suppressed their username in this edit
3. Got them to run a check on themselves
4. Asked them to verify what they see
5. Unsuppressed the edit
6. Asked them to re-run the check to see any difference

The test was a success with the first check (while the edit having the username suppressed) resulting in:

diff hist Page timestamp (Username or IP removed) summary

and the second check resulting in:

diff hist Page timestamp CU without OS talk contribs block Previously blocked Autopatrolled, Checkusers, Administrators summary

I've removed the identifying information about the results and the lack of brackets is due to T223871 et. al. making it not possible to copy out the content defined by CSS. I can (privately) detail the particular edit used if there are logs you want to consult, but would want to only do that on a task that won't become public.

The fix has not addressed the API (Investigate is in a subtask) but the fix for the API should be relatively similar I'm thinking.

I'm also not sure whether it would be a good idea to make this task public until the fix is also applied to logged actions as it not being fixed has been detailed here.

In T311337#8244241, @Dreamy_Jazz wrote:

@sbassett I have coordinated with a enwiki CU who is not an OS and followed the steps:
...
The test was a success with the first check (while the edit having the username suppressed) resulting in:

diff hist Page timestamp (Username or IP removed) summary

and the second check resulting in:

diff hist Page timestamp CU without OS talk contribs block Previously blocked Autopatrolled, Checkusers, Administrators summary

Excellent, thanks for coordinating all of this.

I've removed the identifying information about the results and the lack of brackets is due to T223871 et. al. making it not possible to copy out the content defined by CSS. I can (privately) detail the particular edit used if there are logs you want to consult, but would want to only do that on a task that won't become public.

I think we're ok, I trust your reporting of the above test with the enwiki CU.

The fix has not addressed the API (Investigate is in a subtask) but the fix for the API should be relatively similar I'm thinking.

Yes, we'll need to file another task for that, most likely, just to keep things organized.

I'm also not sure whether it would be a good idea to make this task public until the fix is also applied to logged actions as it not being fixed has been detailed here.

Yes, I'll make a note on the supplemental security release about this.

Is it okay to remove Patch-For-Review?

@mmartorana wasn't this supposed to stay private as the fix is not in place for logged actions and for Investigate for all actions?

I'm also not sure whether it would be a good idea to make this task public until the fix is also applied to logged actions as it not being fixed has been detailed here.

Yes, I'll make a note on the supplemental security release about this.

Let's keep this private until the API and Special:Investigate patches get CR'd and deployed.

In T311337#8286222, @Dreamy_Jazz wrote:

@mmartorana wasn't this supposed to stay private as the fix is not in place for logged actions and for Investigate for all actions?

We had hoped to include this and the related issues within the upcoming supplemental security release (T311785) - which we plan to release today. But unfortunately, given the timing of things and that the other two bugs either don't have patches or haven't been CR'd and deployed yet, I think we should keep this task private (I just did that) and plan to release it with the next supplemental security release when the remaining issues should be addressed.

In T311337#8296437, @Dreamy_Jazz wrote:

If desired I could create a new ticket for logged actions specifically, though it's already been mentioned several times throughout this ticket and as such making this task public would make creating a different task which is private not be different from a public task until a fix patch was proposed.

This should be a separate (sub-)task at this point. This task was also made private again until the various sub-tasks have at least been patched within wikimedia production.

Hey, is anyone opposing pushing T311337#8178549 through gerrit? I don't really think that this is very risky and I don't really like the idea of having this living as a sec patch for an eternity, since it is not really foreseeable when all those subtasks (including the schema change) are resolved. The patch for T207094 has also been pushed through gerrit while similar issues still exist.

I am fine with keeping this task private since it does contain direct information about those other issues.

I wouldn't oppose it. Perhaps a subtask could be created that is for hiding the performer for edits which also doesn't make it very obvious that logged actions are not fixed. The patch then could link to that instead?

In T311337#8307794, @Zabe wrote:

Hey, is anyone opposing pushing T311337#8178549 through gerrit? I don't really think that this is very risky and I don't really like the idea of having this living as a sec patch for an eternity, since it is not really foreseeable when all those subtasks (including the schema change) are resolved. The patch for T207094 has also been pushed through gerrit while similar issues still exist.

It likely wouldn't be an eternity. We had hoped for the sub-tasks T316414 and T318166 to have patches written and deployed last quarter, but that didn't happen. So it will likely be this quarter. T318166 already has a +2'd patch and can likely go out during next Monday's security deployment window (or sooner) and T316414 should have a patch up for review soon.

I am fine with keeping this task private since it does contain direct information about those other issues.

It does, since it's the same issue, just for two different layers, which would likely be one of the first things a hypothetical attacker would check.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/856930 is open to store the log ID which can be used to find the revision deletion status of the log entry. A quick review would be appreciated as the row will need to be on all WMF DBs to have this security issue resolved which requires this to be merged first.

Updated patch for 1.40.0-wmf.18:

In T311337#8514601, @sbassett wrote:

Updated patch for 1.40.0-wmf.18:

01-T311337-rev3.patch4 KBDownload

There is actually an issue with that rebase. Fixed in

(https://sal.toolforge.org/log/Hh4rpYUB6FQ6iqKiMZmR). Also fixed in gerrit in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/879167/2..3/src/CheckUser/Pagers/CheckUserGetEditsPager.php.

Change 879167 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879544 had a related patch set uploaded (by Dreamy Jazz; author: Zabe):

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879552 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879553 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/CheckUser@REL1_35] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879552 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879553 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_35] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879544 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879567 had a related patch set uploaded (by Dreamy Jazz; author: Zabe):

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: do not render suppressed usernames at Special:CheckUser

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

Change 879567 abandoned by Dreamy Jazz:

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: do not render suppressed usernames at Special:CheckUser

Reason:

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

In T311337#8518675, @Zabe wrote:

There is actually an issue with that rebase. Fixed in

01-T311337-rev4.patch4 KBDownload

(https://sal.toolforge.org/log/Hh4rpYUB6FQ6iqKiMZmR). Also fixed in gerrit in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/879167/2..3/src/CheckUser/Pagers/CheckUserGetEditsPager.php.

Ok, this likely needs a redeploy then.

In T311337#8521581, @Dreamy_Jazz wrote:

Unless I'm missing something this can be set to public now too?

It already should be, I believe.

In T311337#8521632, @sbassett wrote:

In T311337#8521581, @Dreamy_Jazz wrote:

Unless I'm missing something this can be set to public now too?

It already should be, I believe.

Hmm. Got confused by the task description editing window saying "IMPORTANT: This report is restricted to members of security, subscribers, and the author." Obviously that doesn't change when the task is set to public.

Thanks for the help with this all (and the security team for guiding me through). One more question for @sbassett / @Mstyles: Should the new tasks be associated with the CVE that these now resolved tasks are associated with (CVE-2022-39193)?

In T311337#8521666, @Dreamy_Jazz wrote:

Thanks for the help with this all (and the security team for guiding me through). One more question for @sbassett / @Mstyles: Should the new tasks be associated with the CVE that these now resolved tasks are associated with (CVE-2022-39193)?

Let's get a separate one for the log events piece, as I think it's a distinct-enough issue. And the related tasks for that can then roll out (hopefully) with the next supplemental security release.

In T311337#8521373, @sbassett wrote:

In T311337#8518675, @Zabe wrote:

There is actually an issue with that rebase. Fixed in

01-T311337-rev4.patch4 KBDownload

(https://sal.toolforge.org/log/Hh4rpYUB6FQ6iqKiMZmR). Also fixed in gerrit in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/879167/2..3/src/CheckUser/Pagers/CheckUserGetEditsPager.php.

Ok, this likely needs a redeploy then.

Er, actually, the patch on production for wmf.18 seems fine:

52         $user = new UserIdentityValue( $row->cuc_user ?? 0, $row->cuc_user_text );
53 -       if ( !IPUtils::isIPAddress( $user ) && !$user->isRegistered() ) {
54 -           $templateParams['userLinkClass'] = 'mw-checkuser-nonexistent-user';
55 +       if ( $row->cuc_type == RC_EDIT || $row->cuc_type == RC_NEW ) {
56 +           $hidden = !$this->usernameVisibility[$row->cuc_this_oldid];

No clue why git formatted my rev3 patch with line 52 getting removed. Sorry for the false alarm.

I just re-deployed it myself this morning, https://sal.toolforge.org/log/Hh4rpYUB6FQ6iqKiMZmR.

In T311337#8521686, @Zabe wrote:

I just re-deployed it myself this morning, https://sal.toolforge.org/log/Hh4rpYUB6FQ6iqKiMZmR.

Oh, ok, great.