The Security-Team is trying to get away from providing a "pass" or "thumbs up" for code during security reviews, as it assumes a level of accountability on our part which we cannot sustain. So we are adopting the more standard system of risk classification and risk ownership for our security reviews. This entails us performing a risk analysis during the review process and then assigning and communicating a level of risk to the requesters/owners of the code. The levels of risk we're using within our analyses are:

Backports complete in gerrit, resolving task for now.

@WMDE-leszek @RazShuty - Just talked with @JBennett. Looks like everything is official re: risk ownership, so I'm going to resolve this task for now. Thanks everyone for all of the patience on working through this review and risk ownership assessment.

In T227591#5425595, @Mholloway wrote:

We're talking with them about what we're doing, and will follow up with them when we're code-complete.

Hey @ssastry - thanks for cutting the security review branch. @Reedy and I will plan to review that soon and reach out to @Arlolra with any questions.

@MarcoAurelio - +1'd both of these. I should be able to security-deploy these sometime today. Is it time to consider a project closure request?

In T229718#5419932, @TTO wrote:

Re number 6, there are already many avenues to deface the wiki if you have edit access to the MediaWiki: namespace. Editing MediaWiki:Sitenotice would have a similar effect as creating a page notice. I don't think the fact that this extension adds another avenue is of any special concern.

Update: CVE-2019-15124.

Patch tested locally, worked fine. Deployed patch to wmf/1.34.0-wmf.17 and tested. I'll request another CVE for this one. Once I have the id, I'll make this task public and backport to master and supported release branches in gerrit.

Proposed patch, same mitigation as T229541:

@Urbanecm - looks like other spambot incidents have been made public in the past, so I think it's fine to do so here. Nothing terribly secret on this particular task.

Deployed. Making task public. Backports already started in gerrit:

1. https://gerrit.wikimedia.org/r/530440
2. https://gerrit.wikimedia.org/r/530441
3. https://gerrit.wikimedia.org/r/530443

I'm deploying this now through gerrit. Will make this task public once deployed.

@Urbanecm - this seem fine for now. I see $wgAccountCreationThrottle is back to normal and @Praxidicae's AF filters are still in place on those 4 projects, correct? If this specific attack (hard to tell them apart, really) heats up again, we can reopen this task, but I'll resolve for now. As a note: we're (Security-Team) still working on some StopForumSpam issues and getting that deployed to beta soon, along with other mitigations which we hope will be more effective in stopping problematic account creations.

PS2 looks good to me. I can try to deploy this later today or tomorrow, since there's no train this week.

@Daimona - Awesome, glad to hear that. Thanks!

I don't believe there's anything else to do here for now.

@Daimona - I'd be interested to hear @Bawolff's thoughts, but for now I've just submitted a patch for Sanitizer.php as you suggested. I think that's the easiest approach for now, unless it gets too noisy in CI, in which case we can revert. But I'd personally rather find more things than not :)

@Rxy - a couple of thoughts:

1. So this patch just prevents a Special:Redirect of the numerical user_id of a hidden user, correct? To note: hidden user user pages are still publicly-viewable if they exist (i.e. they don't display the User account "xxx" is not registered. error message. The intention here is to throw an error if anyone tries to Special:Redirect to them, correct?
2. I'm not sure the hideuser permissions error message makes sense within this context:

You do not have permission to block a username, hiding it from the public...

as it seems to be more directed at users with that permission as opposed for someone trying to visit a redirect. I agree that's the right permission to check but I might recommend something like this for the actual error message:

throw new ErrorPageError( 'badaccess-group0', 'badaccess-group0' );

Otherwise, I think this seems fine and I can security-deploy it whenever, though honestly I'd view this more as a hardening measure that could probably be done in gerrit.

@MarcoAurelio, @Urbanecm:

1. It's fine (imo) to file separate tasks like these for (potentially) new incidents. Though I'd personally prefer if we could consolidate or resolve older tasks so as not to have several open at once, as that can become a bit chaotic to manage.
2. Hopefully T230245 gets resolved soon, though I'm honestly not sure it will make that much of a difference since our current captchas aren't very effective anyways. Some interested WMF folks are still working towards the best path forward for CAPTCHA.
3. Thanks @Urbanecm for adjusting the new account creation throttles. I honestly wouldn't be opposed to making those even more restrictive or potentially even disabling new account creations for a period of time. Unfortunately, this and a handful of other measures are about all we can do from a configuration standpoint right now, outside of digging through logs and attempting to find IPs/UAs/GeoIPs we can block, which is a lot of work and difficult to guarantee there won't be a large number of false positives.
4. @Reedy and I are still working to get StopForumSpam deployed to beta, which should help substantially, however the Security-Team is extremely short-staffed right now for a number of reasons and we don't have anything remotely like a 24/7 SOC.

@Daimona - ah, ok. Yeah, I guess we can leave open for 2.1.0. Thanks.

Ok, that makes sense.

@ssastry - no problem, thanks for the update.

@Daimona - can we call this done? And resolve for now?

@MarcoAurelio - no, was going to start that process now. Wanted to wait until we had a confirmed CVE id.

Update: CVE-2019-14807

Update (I was on vacation earlier this week, just getting back to this):

1. After chatting w/ @MoritzMuehlenhoff, I've gone ahead and requested a CVE for this vulnerability. I'm not sure we've consistently done this in the past for various MediaWiki extensions (at least for deployed and/or bundled extensions), though I'd like to start doing this on a more consistent basis going forward, perhaps even training other foundation/community folks on the process.
2. Once I receive confirmation from Mitre of the CVE id, I'll plan to make this task public and backport in gerrit to the 1.31, 1.32 and 1.33 release branches and master, per the current version lifecycle.
3. I'm not certain any additional communication is warranted here. Posts to phame, wikitech-l, etc. potentially seem to be a little overkill for issues like this and not what's been done in the past. Perhaps the Security-Team should further discuss what might be appropriate messaging for these kinds of vulnerabilities.

Hey @TTO and @Victar -

Are gadgets expected to fall under the standard Wikimedia privacy policy? I'm not seeing any specific privacy policy just for them, nor am I seeing any exceptional language for them within the standard privacy policy. Though gadgets are fairly similar to user scripts, which often call external resources, and which will indeed break once CSP is set to enforce (whenever that may be.)

Pardon the confusion, but are we now wanting to do submodule updates for security deploys? There are currently two for php-1.34.0-wmf.16:

	modified:   extensions/CheckUser (new commits)
	modified:   extensions/MobileFrontend (new commits)

@Reedy - ok, thanks. I removed it from T225152. I'll plan to backport in gerrit to master and any necessary MF release branches.

Update:

1. Patch deployed and tested: the above URL no longer renders an XSS for me.
2. Added this bug to the next security release tracking bug as a sub-task: T225152. I need to double-check w/ @Reedy how we normally do this for deployed extensions. I know that the security release tracking bug is the normal path towards requesting/issuing CVEs, but I know we sometimes just backport as necessary in gerrit for extensions. I'll keep this bug private until I know what to do here.

Note: I plan to security-deploy @MaxSem's patch from T229541#5383474 this afternoon (Aug 2nd) and provide another update here.

@Mholloway - from a high-level perspective, I think the Security-Team is fine with this and would assign a low risk for now, especially given the precedent of things like CX-cxserver. Some considerations:

1. Has this been reviewed by WMF-Legal yet? I believe cx server went through a similar process; see Acceptance Criteria in T76185. This should probably happen prior to deployment.
2. Some previous security reviews of the cx service (and related components) might be helpful to peruse at your leisure, as it's a close-ish parallel: T85686, T144467, T143185. Particularly the conversation starting at T143185#2632101, T144467#4795794 and T85686#958661. Mainly just as an idea of what we would look for during a more formal code review. Of course, the attack surface for this service would seem a bit smaller since we're talking about media metadata in a standard format that should not be easily manipulated by potential attackers, unlike the wikitext used by cx server.
3. Though it wasn't formally noted, I assume every component of this will be using TLS.
4. Even though some headers can be less critical for these types of services, I would strongly advise setting appropriate security headers, including a robust CSP for each component of this service.
5. I'd also recommend getting this on the radar of the Performance-Team to see if they have any additional concerns or recommendations for best practices. It may also make sense to reach out to the Language-Team to see if they have run into any performance issues with their usage of the various 3rd party MTs for the cx server.
6. Finally, once more code has been developed for this service (closer to production-ready), we can definitely perform a more formal security review if you'd like.

Looking at this - the XSS only appears to render for me within the first URL. I assume the second URL was provided just to show the diff?

Resolving now per T149869#5357956.