Hmm, the PHP_ENGINE cookie seems to have vanished after some cache-clearing and a few re-authentications on my end. The metawikiSession cookie is strange - it seems like Chrome thinks enwiki should have access to it, while Firefox does not. And Chrome's Developer Tools console has it under a different domain (meta.wikimedia.org) than Chris Pederick's Web Developer toolbar (en.wikipedia.org), which erratically lists it. Perhaps this is just an issue with Chrome or with the various developer tools within Chrome.

In T240884#5810094, @Ladsgroup wrote:

One complicating factor here is that AbuseFilter and SpamBlacklist both don't have a clear maintainer.

Thanks for the feedback, @Krinkle !

Are we comfortable with every page title on private wikis potentially being made public, such that allowing any user to publicly find and/or enumerate them (as Tool-Pageviews allows) wouldn't result in any Vuln-Infoleak s? I'm thinking there might be at least a few pages on officewiki where this might not be the case.

Sounds like a good idea, and it looks like browser adoption is edging towards ubiquity. Just to clarify, assuming we're ignoring centralauth_Session for now, the session cookies I typically see being set on production wikis are:

1. GeoIP
2. {wikiID}wikiSession
3. metawikiSession - oddly appears as an enwiki cookie, but not on other projects.
4. PHP_ENGINE - I still have this checked as a beta pref on enwiki.
5. thanks-thanked

And I assume a few others. Do we need to worry about these individually or would the envisioned implementation simply be to prepend __Host- to to the name value for any cookie without a specified expiration within all relevant setCookie() functions?

In T242907#5806947, @chasemp wrote:

See the old param style, see the new phab actual form, go form go.

In T242907#5806968, @chasemp wrote:

I'm not sure on review security. Since this is a code review request and such. Is it best practice to restrict to Security-Team, Security, author, and subscribers?

In T239063#5697413, @Varnent wrote:

If they are static to a specific date, we should amend the text (as I mentioned before) to indicate that date - so something like "Number of edits across all databases in Wikimedia's servers as of 26 November 2019." and then remove the link that explains the live number count. Otherwise I fear someone may report that as a live number when it is now a historical number.

In T239680#5802879, @Tchanders wrote:

We proposed option #4 in case passing information in the URL and exposing it in the logs was deemed too large a risk; however, it sounds like this is not the case.

Hey @Krinkle - we were wondering if you had any thoughts about the viability, potential redundancy and performance of this proposed extension. Thanks!

Reassigning to @Reedy - if you'd like to pair up on this and explore some automated approaches for the review, let me know!

Patch merged. Should ride this week's train. Will file follow-up task suggesting removal of checkPasswordCannotBeSubstringInUsername.

In T237805#5698355, @jrbs wrote:

@eprodromou Hi! YGM. :)

In T232820#5801191, @fgiunchedi wrote:

Thanks for the feedback @sbassett, all good points! Since indeed the code turned out to be simpler than we initially expected no full security review seems needed. I'll ping you and/or @Reedy once the other points have been addressed

@Tchanders - are you proposing to pass the JWT around via POST or only the relevant data (likely the encrypted payload from the JWT option)? I suppose either would provide for a slightly smaller attack surface than sending any token/data as a GET param. Though using JWTs seems like it might be a bit cleaner, however I'm not sure if that also entails a reworking of the pager and filters as mentioned for option 4 in T239680#5772095.

@zhuyifei1999 - As a security-minded person, I tend to recommend requesting CVEs for pretty much all vulnerabilities, especially for FLOSS code. I'm not sure if Mitre would ever reject a request by deeming the application not noteworthy enough or something like that - I'm guessing Quarry probably isn't run outside of wmflabs? Regardless, the form should only take a few minutes to fill out, so if you have the cycles, I'd recommend doing that. And if a CVE ID is issued, we can update this tasks's title to include it.

In T242355#5800575, @zhuyifei1999 wrote:

Applied. https://quarry.wmflabs.org/run/424170/output/0/tsv https://quarry.wmflabs.org/run/424170/output/0/csv LGTM.
Shall we make the ticket public and put the patch on gerrit?

In T239680#5793918, @dbarratt wrote:

It would need to be passed in the URL because, as far as I know, it's impossible to to set the headers on a simple anchor element.

In T240472#5799828, @JTannerWMF wrote:

Hi @sbassett , I've merged the tasks.

In T232820#5796753, @fgiunchedi wrote:

AFAICT the code is basically ready to be merged, when does security review need to happen ?

In T242355#5799641, @Framawiki wrote:

Line 27: I don't see why using str.join() with only to items in the list, so I purpose content_type = mime_type + '; charset=utf-8'.
Otherwise +1

The following patch should fix this issue:

@ppelberg - So this task and T242134 are not ostensibly the same thing? If they are, we can merge them. Only reason I'm asking is because there doesn't seem to be anything actionable on this task, so at the very least I would probably untag Security Readiness Reviews.

Further researching implementations similar to option 2, could the token be passed around in a response header as opposed to a url param? That would vastly reduce its exposure in various logs, etc.

We had a few initial comments and questions:

1. General Comment 1: Security Concept Review s should never be considered a hard blocker of anything. Apologies if that hasn't been made clear within our documentation. If you/Releng feel you have some specific questions or concerns which are of critical importance and that only the Security-Team can address, feel free to let us know what those are.
2. Scoping Question 1: Do you have a final candidate list of new technologies that will be introduced within the new CI/CD and what those technologies will replace within the existing system? It's unclear from the various pieces of documentation where Releng is at in their selection process and we'd like to have this narrowed down to as small a list as possible prior to any review.
3. Scoping Question 2: Can you clarify the specifics of the testing and staging environments from the image promotion pipeline? Where will these environments exist and who will be the ostensible maintainers of said environments?
4. Scoping Question 3: This comment within the task description - some K8s cluster, possibly hosted by a commercial provider - seems to imply the potential for SaaS/PaaS options. Is this still being considered? Can we get a sense of what systems and services would be candidates for such an option?

In T239680#5792848, @dbarratt wrote:

We can do this, but there is a downside to 3rd parties:
We can bind it if you think it's necessary, but I wanted to highlight the downside of doing that, as it could prevent 3rd party wikis from pushing security fixes (from the library) quickly.

@Mooeypoo et al - a few initial security points/concerns for option 2:

1. firebase/php-jwt seems like a mature, stable JWT lib, especially since WMF already deploys 5.0.0. And jwt.io even likes it. A minor nit might be pinning to a specific version within composer.json on the patch, since that tends to be a bit more secure and would match what would need to be in mediawiki/vendor.
2. I noticed in the latest PS on the patch there was a TODO around setting an exp field in the payload. I'd strongly advise this be done and then the tokens properly invalidated after said expiry, otherwise I'd place a higher risk rating (medium+) for this option. I don't have enough experience with CU to know for certain, but it seems like there could be a theft/replay risk with folks who have both checkuser and checkuser-log rights and we'd want to minimize this attack surface as much as we can. I think we'd consider these to be very trusted users, but such assumptions aren't really part of a robust security model.
3. It might be a good idea to programmatically check for the existence of the encryption algorithm as we do in core in addition to the $secret check within the TokenManager constructor.
4. I'm not certain I love the way the IV seed is being created in the patch, though it's probably good enough for this application.

Update: after chatting w/ @Reedy a bit and having a look at the way ext:TorBlock (which ext:StopForumSpam borrows from in other places) does similar things within TorExitNodes.php, I'd like to model a patch for ext:StopForumSpan on what fetchExitNodesFromTorProject() and loadExitNodes() do by proxying out to an external URL (whichever SFS blacklist we want to use) and dumping it into WANObjectCache. I'd imagine this would also imply some work upon the better IPV6 support mentioned within T212528, particularly around serialization, which may or may not be needed as ext:TorBlock seems to just dump IPs into the cache. I'm guessing it might make sense to have this be a separate mode of operation from the existing code, as it's heavily tied to WMF production, which may not be appropriate for all users of ext:StopForumSpam, possibly controlled by a config variable.

First review initially scheduled for 2019-01-10.

Removing Security Readiness Reviews since there's no actual review request within this task.

I'm going to close this task as invalid for now, since the above discussions were related to a completely different set of security review policies which were superseded by a new SOP in February of 2019. The current SOP has continued to evolve and will likely evolve further as the Security-Team is in the process of revamping some of our intakes and workflows, which should hopefully wrap up Q3 of FY 2020. If there are still concerns over the current SOP, I would invite folks to first contribute their concerns on its talk page to keep pre-actionable discussion in a single, logical place.

@chasemp - Just checked a bunch of my old reviews and all of them look fine now. Hopefully we can figure out all of the weird edge-case ones that are still vanished into the aether.

In T242242#5786945, @elukey wrote:

You can use the one that all the members of analytics-privatedata-users can access, sudoing as the analytics-privatedata users. We don't create per-user keytabs for the moment, we'll see in the future if needed..

@elukey - thanks. Can I create a keytab for myself on stat1007 or does Analytics need to do that for me? Wasn't quite certain of the process from https://wikitech.wikimedia.org/wiki/Analytics/Systems/Kerberos/UserGuide#Run_a_recurrent_job_via_Cron_or_similar_without_kinit_every_day.

@jrbs - this ever get resolved?

Sure, most things here:

@Yaron_Koren - As the ostensible maintainer here, would you be ok with us making this task public as we traditionally do for any security issue that transitions into a "closed" state? While that technically involves exposing this not-fully-resolved security issue, it would probably be beneficial as a notice to anyone running the Cargo extension to be wary of this issue.

So it doesn't look like templates/search/PropertySuggestionsWidget.mustache+dom existed in REL1_34 or previous, so no backports necessary. I'm going make the task public now and request a CVE.

@Jdlrobson - it'd be nice to get backports to supported release branches completed - I can maybe get those started in gerrit now. I think we can make the task public now (since it's been patched and deployed within production) but I'd wait to resolve it if we can get the backports completed first.

So the mw.org Extension template was updated in T241243. And LDAPAuthentication2's info box was changed to reflect that it only supports LTS versions. I understand that isn't anywhere close to a foolproof solution, but it's a place to quickly point anyone who has questions regarding "missing" release branches.

In T240773#5779321, @matthiasmullie wrote:

Yes - not sure why gerritbot didn't pick it up, but here's the patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseMediaInfo/+/558203

@matthiasmullie - Is there a gerrit patch set or security patch we could reference here? Backports to supported release branches or a CVE? The Security-Team can help with the latter if need be. Thanks.