Proposed patch:

In T397196#10924572, @Dreamy_Jazz wrote:

Proposed patch:

T397196.patch16 KBDownload

That's a pretty massive patch. It would be great if others who were familiar with AbuseFilter's internals could provide some code review here prior to any attempted production deployment.

In T397196#10928501, @sbassett wrote:

In T397196#10924572, @Dreamy_Jazz wrote:

Proposed patch:

T397196.patch16 KBDownload

That's a pretty massive patch. It would be great if others who were familiar with AbuseFilter's internals could provide some code review here prior to any attempted production deployment.

TSP will have a look and comment after we've reviewed.

Functionality broadly lgtm, have some questions/comments:

			if ( $filter->isProtected() ) {
				$varsArray = $this->afVariablesManager->dumpAllVars( $vars, true );

Can this be condensed into the general check below? getForbiddenVariables calls canViewProtectedVariables under the hood and it's called there under a subset of the variables called here.

// Check that the user can see all the protected filters in the abuse_filter_log log.

non-blocking, can happen w/i18n update if any change needs to be made - does this mean variables? Only 1 filter can be checked at a time afaict.

// This is to avoid a user being able to know the value of the variable if they repeatedly try values to

non-blocking, can happen w/i18n update if any change needs to be made - I agree with the logging but the comment isn't clear to me. This doesn't seem to stop the behavior, only surface it, I think? It logs against the filter being tested against so at least in the QA steps listed, I'd only know that someone was looking into the temporary account but I wouldn't be able to tell that they were trial/error-ing their way to an IP.

In T397196#10931904, @STran wrote:

Functionality broadly lgtm, have some questions/comments:

			if ( $filter->isProtected() ) {
				$varsArray = $this->afVariablesManager->dumpAllVars( $vars, true );

Can this be condensed into the general check below? getForbiddenVariables calls canViewProtectedVariables under the hood and it's called there under a subset of the variables called here.

No, because we are checking that the user can see all the protected variables in the specific abuse_filter_log row. That isn't applicable when providing an rcid or vars. The ::getForbiddenVariables call is specific to the provided test pattern and not the variables that were generated for the action.

This is okay because for the rcid type, the variable won't be checked against if it's not in the test pattern and vars would be specifying the variable value directly anyway (so you would know the value of it).

---

// Check that the user can see all the protected filters in the abuse_filter_log log.

non-blocking, can happen w/i18n update if any change needs to be made - does this mean variables? Only 1 filter can be checked at a time afaict.

Yes I meant to write "variables" here. Unless we need to modify the patch, I'll leave this as-is.

---

// This is to avoid a user being able to know the value of the variable if they repeatedly try values to

non-blocking, can happen w/i18n update if any change needs to be made - I agree with the logging but the comment isn't clear to me. This doesn't seem to stop the behavior, only surface it, I think? It logs against the filter being tested against so at least in the QA steps listed, I'd only know that someone was looking into the temporary account but I wouldn't be able to tell that they were trial/error-ing their way to an IP.

Yes it only surfaces it. The idea is that the logging discourages the user from using the trial-and-error method to access the value because it creates the log entry, so they instead would access the value directly. The comment isn't clear on this though. I'd prefer to update it later to avoid another version.

Ah I see, thanks for the clarification. Yes then this lgtm 👍 Comment updates can happen w/the follow-up i18n change needed when this task is public.

Tested this locally and confirmed that:

• I don't return a match if I don't have access to protected variables
• Logging works as expected (access recorded on the user associated w/the filter hit)

Fix only needs backporting to REL1_43 and REL1_44, because protected filters did not exist before bf28dbce0e5084340cda84365e4ffe65f020124c which was first present in release 1.43.

Deployed to wmf.6 (and wmf.5 in case of train rollback).

In T397196#10932740, @Dreamy_Jazz wrote:

Deployed to wmf.6 (and wmf.5 in case of train rollback).

Thanks, and thanks for adding this to the appropriate tracking tasks!

For a large number of recent changes and abuse logs, I used abusefiltercheckmatch API to test various patterns against them, including those which used protected variables.

I checked that, when the pattern used a protected variable, an unauthorised user always got a "permission denied" response.

Change #1166045 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/AbuseFilter@master] SECURITY: Check protected variable access in CheckMatch API

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

Change #1166046 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/AbuseFilter@REL1_44] SECURITY: Check protected variable access in CheckMatch API

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

Change #1166047 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/AbuseFilter@REL1_43] SECURITY: Check protected variable access in CheckMatch API

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

Change #1166046 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_44] SECURITY: Check protected variable access in CheckMatch API

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

Change #1166045 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] SECURITY: Check protected variable access in CheckMatch API

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

Change #1166047 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_43] SECURITY: Check protected variable access in CheckMatch API

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