Fixed to restore full paths and url (was the fix for that in Phatality not yet deployed?).

Also marked this one with security as the other ticket was protected.
Ready to be evaluated at the start of the week.

Hmm, I dont appear to be able to assign story points to this (as it is a Phatality ticket I think).
Anyway, we estimated it at 8, I'll add it to the description for now.

In T237667#5679134, @Ladsgroup wrote:

2- How to fix the underlying problem: Should we pray no language use "@" in their messages for time precision?

As long as the $delimiter passed to preg_quote() matches the delimiter actually used when constructing the $pattern, it shouldn't matter if they do. That's what that $delimiter parameter is for.

In T237667#5679340, @Anomie wrote:

In T237667#5679134, @Ladsgroup wrote:

2- How to fix the underlying problem: Should we pray no language use "@" in their messages for time precision?

As long as the $delimiter passed to preg_quote() matches the delimiter actually used when constructing the $pattern, it shouldn't matter if they do. That's what that $delimiter parameter is for.

True, my bad. Ignore that part then.

In T237667#5679134, @Ladsgroup wrote:

I'm not sure how to: 1- Make a patch: Should I treat it as a security issue or not?

This documentation should work and yes, this is definitely a legitimate security issue IMO. Happy to help with the process in any way I can, though I'm not at all familiar with wikidata deploys, which I assume are different in at least a few ways from that of other production wikis.

2- How to fix the underlying problem: Should we pray no language use "@" in their messages for time precision? It's very unlikely TBH OTOH it's vulnerable to malicious changes. Should let people to define parsing based on their language and cause XSS (un)intentionally or completely ban it and stick to the English version (which also can be compromised if the attacker can change MediaWiki:wikibase-time-precision-Gannum page). I think I should ping security team: @Reedy @sbassett @chasemp

Sounds like this is just a delimiter consistency issue per @Anomie's comment above (T237667#5679340). Are there any additional technical concerns beyond that which would block the creation of a patch?

In T237667#5683011, @sbassett wrote:

2- How to fix the underlying problem: Should we pray no language use "@" in their messages for time precision? It's very unlikely TBH OTOH it's vulnerable to malicious changes. Should let people to define parsing based on their language and cause XSS (un)intentionally or completely ban it and stick to the English version (which also can be compromised if the attacker can change MediaWiki:wikibase-time-precision-Gannum page). I think I should ping security team: @Reedy @sbassett @chasemp

Sounds like this is just a delimiter consistency issue per @Anomie's comment above (T237667#5679340). Are there any additional technical concerns beyond that which would block the creation of a patch?

I didn't know which way to proceed (gerrit or security). I just make the patch here now.

In T237667#5683011, @sbassett wrote:

In T237667#5679134, @Ladsgroup wrote:

I'm not sure how to: 1- Make a patch: Should I treat it as a security issue or not?

This documentation should work and yes, this is definitely a legitimate security issue IMO. Happy to help with the process in any way I can, though I'm not at all familiar with wikidata deploys, which I assume are different in at least a few ways from that of other production wikis.

Wikidata deploys used to be quite different, but we got rid of this (“killed the build”) a while ago, and today it should work like any other production wiki as far as I’m aware.

In T237667#5683974, @Ladsgroup wrote:

0001-SECURITY-Change-regex-delimiter-for-preg_match.patch1 KBDownload

I think it would make sense to also add a delimiter '@' to the other preg_quote() calls in getRegexpFromMessageText(), but if I understand correctly, that’s not necessary because the quoted strings are constants that don’t contain any @ characters.

It's deployed now.

The fatals have dropped to zero:

In T237667#5685443, @Ladsgroup wrote:

The fatals have dropped to zero:

Great, looks like that did the trick. As @Reedy alluded to on IRC, the next steps should probably be

1. Make this task public
2. Create backports for supported release branches in gerrit
3. Obtain a CVE (I feel like this merits one, since it's Wikibase and was a legitimate regexp injection)

Happy to help with any of this.

@Michael noticed that the logspam is back, and it looks like the security fix was undeployed again for some reason:

lucaswerkmeister-wmde@deploy1001 /srv/mediawiki-staging/php-1.35.0-wmf.5/extensions/Wikibase $ git reflog
c467aa879 HEAD@{0}: checkout: moving from ef2912472c300720ae29409c852612b4b902e5b0 to c467aa87971cd89901c5027dfa8355e6cbd5daa1
ef2912472 HEAD@{1}: am: SECURITY: Change regex delimiter for preg_match
c467aa879 HEAD@{2}: checkout: moving from f85badb5937c5469345c83c41171288f38e58299 to c467aa87971cd89901c5027dfa8355e6cbd5daa1
f85badb59 HEAD@{3}: checkout: moving from 94ffd59c930c6db907245b8b27f8f12220129d5e to f85badb5937c5469345c83c41171288f38e58299
94ffd59c9 HEAD@{4}: checkout: moving from 5f0e99baa6716b31976e64213c6d792e572f4252 to 94ffd59c930c6db907245b8b27f8f12220129d5e
5f0e99baa HEAD@{5}: checkout: moving from 3b0e9d7d9c140453d67943d6e002e63f815a694d to 5f0e99baa6716b31976e64213c6d792e572f4252
3b0e9d7d9 HEAD@{6}: checkout: moving from wmf/1.35.0-wmf.5 to 3b0e9d7d9c140453d67943d6e002e63f815a694d
814e7a53a HEAD@{7}: checkout: moving from 814e7a53ab65e6a90f30cb9f066a04b822a76c71 to wmf/1.35.0-wmf.5
814e7a53a HEAD@{8}: checkout: moving from master to 814e7a53ab65e6a90f30cb9f066a04b822a76c71
814e7a53a HEAD@{9}: clone: from https://gerrit.wikimedia.org/r/mediawiki/extensions/Wikibase
lucaswerkmeister-wmde@deploy1001 /srv/mediawiki-staging/php-1.35.0-wmf.5/extensions/Wikibase $ git log --oneline --all --graph --decorate ef2912472 | head
* ef2912472 SECURITY: Change regex delimiter for preg_match
* c467aa879 (HEAD, origin/wmf/1.35.0-wmf.5) Stop outputting anything in case of 304 responses in Special:EntityData
* f85badb59 Cache SqlEntityInfoBuilder also based on term type
* 94ffd59c9 Do not inject row when it's empty
* 5f0e99baa Put a layer of APC cache on top of reading wb_terms in SqlEntityInfoBuilder
* 3b0e9d7d9 Do not catch DBError in ReplicaMasterAwareRecordIdsAcquirer.
| *   eb4d8131d (origin/master, origin/HEAD) Merge "Stop outputting anything in case of 304 responses in Special:EntityData"
| |\  
| | * 794ca35b8 Stop outputting anything in case of 304 responses in Special:EntityData
| * |   b159d3ad5 Merge "bridge: add Api definition in MwWindow"

If I understand the Git metadata correctly, this seems to have been done by @Urbanecm, most likely during EU SWAT:

lucaswerkmeister-wmde@deploy1001 /srv/mediawiki-staging/php-1.35.0-wmf.5/extensions/Wikibase $ ls -l "$(git rev-parse --git-dir)/HEAD"
-rw-rw-r--  1 urbanecm        wikidev     41 Nov 25 11:12 HEAD

Though that wouldn’t explain why the logspam only restarted on the 26th around 18:00 (Logstash), over a day after this change.

I redeploy it now.

In T237667#5696604, @Ladsgroup wrote:

I redeploy it now.

Done now.

In T237667#5696585, @Lucas_Werkmeister_WMDE wrote:

If I understand the Git metadata correctly, this seems to have been done by @Urbanecm, most likely during EU SWAT:

lucaswerkmeister-wmde@deploy1001 /srv/mediawiki-staging/php-1.35.0-wmf.5/extensions/Wikibase $ ls -l "$(git rev-parse --git-dir)/HEAD"
-rw-rw-r--  1 urbanecm        wikidev     41 Nov 25 11:12 HEAD

Though that wouldn’t explain why the logspam only restarted on the 26th around 18:00 (Logstash), over a day after this change.

The most likely explanation is that it got messed up during EU SWAT then got synced later which happened because of the branch cut.

In T237667#5696622, @Ladsgroup wrote:

The most likely explanation is that it got messed up during EU SWAT then got synced later which happened because of the branch cut.

Ah, that makes sense.

For what it's worth, from yesterday's SAL and my notes:

19:46 	<brennen@deploy1001> 	rebuilt and synchronized wikiversions files: group0 to 1.35.0-wmf.8
19:42   (personal notes)        $ scap sync-wikiversions "group0 to 1.35.0-wmf.8"
19:39 	<brennen@deploy1001> 	Finished scap: testwiki to php-1.35.0-wmf.8 and rebuild l10n cache (duration: 32m 52s)
19:05   (personal notes)        $ scap sync "testwiki to php-1.35.0-wmf.8 and rebuild l10n cache"
19:06 	<brennen@deploy1001> 	Started scap: testwiki to php-1.35.0-wmf.8 and rebuild l10n cache
19:04 	<brennen@deploy1001> 	Pruned MediaWiki: 1.35.0-wmf.2 (duration: 07m 08s)

Thanks, @Ladsgroup.

So, we still need to:

• Merge the patch on master
• Backport to:
  • REL1_34
  • REL1_33
  • REL1_32
  • REL1_31
• Trigger a rebuild of the docker images
• Announce the change?
• Make this ticket public
• Create CVE?

I'll get this done this week...

Change 556160 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@REL1_34] SECURITY: Change regex delimiter for preg_match

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

Change 556161 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@REL1_33] SECURITY: Change regex delimiter for preg_match

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

Change 556162 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@REL1_32] SECURITY: Change regex delimiter for preg_match

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

Change 556163 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@REL1_31] SECURITY: Change regex delimiter for preg_match

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

Change 556158 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] SECURITY: Change regex delimiter for preg_match

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

Change 556161 merged by Ladsgroup:
[mediawiki/extensions/Wikibase@REL1_33] SECURITY: Change regex delimiter for preg_match

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

Change 556162 merged by Ladsgroup:
[mediawiki/extensions/Wikibase@REL1_32] SECURITY: Change regex delimiter for preg_match

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

Change 556163 merged by Ladsgroup:
[mediawiki/extensions/Wikibase@REL1_31] SECURITY: Change regex delimiter for preg_match

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

Change 556160 merged by Ladsgroup:
[mediawiki/extensions/Wikibase@REL1_34] SECURITY: Change regex delimiter for preg_match

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

Build of the docker images triggered at https://travis-ci.org/wmde/wikibase-docker/builds/623139231

Docker images were rebuilt

latest-bundle: digest: sha256:e8d16b27dbe999f74384187a94121a48570a71f579fe5927e2387dbbceb44911 size: 9092
1.33-bundle: digest: sha256:e8d16b27dbe999f74384187a94121a48570a71f579fe5927e2387dbbceb44911 size: 9092
latest-base: digest: sha256:f964c1517bdb9e6c6f130481fdbf99c9ebc86722abd0d680330324d9deab1cfa size: 6373
1.33-base: digest: sha256:f964c1517bdb9e6c6f130481fdbf99c9ebc86722abd0d680330324d9deab1cfa size: 6373
latest: digest: sha256:f964c1517bdb9e6c6f130481fdbf99c9ebc86722abd0d680330324d9deab1cfa size: 6373
1.33: digest: sha256:f964c1517bdb9e6c6f130481fdbf99c9ebc86722abd0d680330324d9deab1cfa size: 6373


1.32-bundle: digest: sha256:29f510b5d266a0b4b77f7dc2ec49b4f167a9748bee9b002a70ce1910e8c838ee size: 8883
1.32-base: digest: sha256:f3bb55d76a9875ceadd8b4a964dc6a8b43e4b31aba6fefe2d83ef86bd674c44e size: 6373
1.32: digest: sha256:f3bb55d76a9875ceadd8b4a964dc6a8b43e4b31aba6fefe2d83ef86bd674c44e size: 6373

lts-bundle: digest: sha256:1cade62538a5604e63544504cada0a8e9870b62169a2d52058a09d017fffe8fa size: 8884
1.31-bundle: digest: sha256:1cade62538a5604e63544504cada0a8e9870b62169a2d52058a09d017fffe8fa size: 8884
lts-base: digest: sha256:ead686faadfb6ad71cbf049ab8a30534e426265059b4601f1c92c83a50e709ab size: 6373
1.31-base: digest: sha256:ead686faadfb6ad71cbf049ab8a30534e426265059b4601f1c92c83a50e709ab size: 6373
lts: digest: sha256:ead686faadfb6ad71cbf049ab8a30534e426265059b4601f1c92c83a50e709ab size: 6373
1.31: digest: sha256:ead686faadfb6ad71cbf049ab8a30534e426265059b4601f1c92c83a50e709ab size: 6373

Moving to verification, then will make this public / whoever review it can.

@Ladsgroup already made it public earlier today (T237667#5727466).

Sorry, When I made the patch to gerrit it made sense to open the ticket so the bots can add the patch to this ticket, when the patch is in gerrit, this can be opened. right?

In T237667#5728294, @Ladsgroup wrote:

Sorry, When I made the patch to gerrit it made sense to open the ticket so the bots can add the patch to this ticket, when the patch is in gerrit, this can be opened. right?

This is fine. Security-Team's general rule of thumb here is that once a security issue has been patched (and is stable) on all relevant production systems, it's fine to make any security-protected tasks public. The two caveats for this are:

1. It's a patch for MW core or a bundled extension and needs to be kept protected for the (typically) quarterly MW security releases.
2. There's PII or other sensitive data on the task itself and it needs to remain PermanentlyPrivate.

Neither appears to be the case here. CVE requests, backports, announcements, etc. can all happen after the task is public, though our recommendation is that those all happen sooner than later. If there are going to be long, anticipated delays for any of these or there are extenuating circumstances (example: T239922#5720376), then the task can become public later.

In T237667#5728294, @Ladsgroup wrote:

Sorry, When I made the patch to gerrit it made sense to open the ticket so the bots can add the patch to this ticket, when the patch is in gerrit, this can be opened. right?

yup, all good, i just missed that it happened already :)