Also, if you make the lemma shorter, you can still restore an old revision with the long lemma, both via regular MediaWiki “undo” and via the Wikibase-specific “restore” feature. This surprises me (I would have thought that restoring old revisions would go through the ChangeOp mechanism, which seems to be where the lemma length validation happens), but I’m not sure if we should fix it.

Also, Lexemes with overlong lemmas can be merged into other Lexemes, so in theory you can “edit” the overlong lemma:

• change lemma to something short and with a different language code
• create new Lexeme with edited language code (spelling variant)
• merge that Lexeme into the original one
• remove short lemma again

This is probably not worth fixing separately, once it’s impossible to create these Lexemes in the first place it should no longer be an issue.

The Wikidata Query Service doesn’t find any existing Lexemes with a lemma above 1000 UTF-16 code units (query – Blazegraph STRLEN() counts code units not code points); since code units ≥ code points, I think that means there are no Lexemes with a lemma that’s too long in Wikidata at the moment.

Good catch, Lucas! Am I right to assume that the team will work on things like this with a high priority even without me doing something about it specifically?

This turns out to affect Special:NewProperty as well, it doesn’t validate the label, description and aliases using the general term validators. (Special:NewItem does validate them.)

Wikibase patch for Special:NewProperty:

I’ll do Special:NewLexeme(Alpha) tomorrow on Friday, probably.

In T308659#7939191, @Lucas_Werkmeister_WMDE wrote:

Wikibase patch for Special:NewProperty:

0001-SECURITY-Validate-term-length-in-Special-NewProperty.patch20 KBDownload

I’ll do Special:NewLexeme(Alpha) tomorrow on Friday, probably.

While that would be a fine patch on Gerrit, it does touch quite a bit of code. Do we actually need the cleanup changes to Special:NewItem in this security patch, when the actual issue we want to fix is in Special:NewProperty? Maybe this is ok though if the actual security release is done very soon

We don’t really need them, but I would prefer to do them at the same time so that we don’t forget about it or have the two special pages inconsistent with one another for longer than is necessary. Looking at the Release policy and News, it looks like we can expect the next security release by the end of June, which feels soon enough to me.

Mh, ok. After trying it out locally, the patch seems to work fine. And given that Special:NewItem and Special:NewProperty shouldn't seeing that many changes, I think going forward with it in its current form is ok. Let's make sure we really get that end-of-june security release though.

Okay, and here’s a WikibaseLexeme patch (shorter, because the relevant WikibaseLexeme code is more likely to have other changes in the coming weeks, so in this case it’s probably better to err on the side of caution and keep the security patch minimal):

In T308659#7949381, @Lucas_Werkmeister_WMDE wrote:

Okay, and here’s a WikibaseLexeme patch (shorter, because the relevant WikibaseLexeme code is more likely to have other changes in the coming weeks, so in this case it’s probably better to err on the side of caution and keep the security patch minimal):

+1 to this as an efficient security patch, for now, though I didn't test. The logic seems simple and correct enough. I assume the TODOs imply this patch might not be directly backported in gerrit, but improved upon once deployed?

My thinking was that the patch could be pushed to Gerrit and merged as is (together with other patches for the next security release), and then the TODO could be addressed afterwards.

Though I’m interested what workflow you would prefer for improving security changes once deployed – should we leave instructions in the commit messages about how the changes are to be combined for Gerrit (git rebase --autosquash style, perhaps), or directly squash patches together, altering history in /src/mediwiki-staging and committing changes to existing patch files in /srv/patches?

Alright, Wikibase and WikibaseLexeme patches deployed (tested on Test Wikidata). https://sal.toolforge.org/log/kF-j-4AB8Fs0LHO5NIzv

In T308659#7955574, @Lucas_Werkmeister_WMDE wrote:

My thinking was that the patch could be pushed to Gerrit and merged as is (together with other patches for the next security release), and then the TODO could be addressed afterwards.
...
Though I’m interested what workflow you would prefer for improving security changes once deployed – should we leave instructions in the commit messages about how the changes are to be combined for Gerrit (git rebase --autosquash style, perhaps), or directly squash patches together, altering history in /src/mediwiki-staging and committing changes to existing patch files in /srv/patches?

We don't have any official guidance for this as it's generally been handled on a case-by-case basis, IME. I think it comes down to a judgment call on how "sloppy" the security patches are. If they're hastily developed and intended as a very temporary fix and/or involve multiple patches due to iterative bug-fixing, then we'd likely recommend refactoring/squashing before backporting in gerrit. If the patches themselves are mostly fine but could or should be cleaned up later, such follow-up work can happen at a later date, after the initial security patch backports are merged in gerrit, at the discretion of the code maintainers.

Alright, thanks. I’d see this as the latter category, i.e. I don’t plan to further work on it until the current patches have been published as part of the next security release.

Hey @Lucas_Werkmeister_WMDE - The security team is attempting to get the next supplemental security release (T305209) out within the next week or two, and we were hoping to include this bug. I know there was some discussion above about polishing the two existing security patches a bit more. Would you still prefer to do that or should we try to push what exists now up to gerrit, get them merged so we can remove the patches from production and have something available for the supplemental release? I'd perfer that approach but I'm also fine with waiting and keeping this bug locked down for a while longer, perhaps until next quarter's supplemental security release, but likely not after that.

Feel free to upload and merge them now, the intention was to polish them afterwards (and it should be fine if the polishing patches don’t make it into the security release – they’d only make the code more maintainable, without changing anything at runtime, so they’re mostly important for the master branch).

Change 808989 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] SECURITY: Validate term length in Special:NewProperty

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

Change 808990 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] SECURITY: Validate lemma length in Special:NewLexeme(Alpha)

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

Change 808948 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_38] SECURITY: Validate term length in Special:NewProperty

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

Change 808949 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_38] SECURITY: Validate lemma length in Special:NewLexeme(Alpha)

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

Change 808989 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Validate term length in Special:NewProperty

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

Change 808990 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] SECURITY: Validate lemma length in Special:NewLexeme(Alpha)

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

Change 808948 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_38] SECURITY: Validate term length in Special:NewProperty

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

Change 808949 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_38] SECURITY: Validate lemma length in Special:NewLexeme(Alpha)

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

Change 809203 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Clean up lemma length validation

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

This appeared in the CVE feed as https://www.cve.org/CVERecord?id=CVE-2022-34750

Change 809572 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_37] SECURITY: Validate term length in Special:NewProperty

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

Change 809601 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: Validate term length in Special:NewProperty

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

In T308659#8036319, @MoritzMuehlenhoff wrote:

This appeared in the CVE feed as https://www.cve.org/CVERecord?id=CVE-2022-34750

Yes, I requested that ID a couple of days ago and forgot to update the task title here. Thanks for doing that.

Change 809603 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_37] SECURITY: Validate lemma length in Special:NewLexeme

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

Change 809604 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_35] SECURITY: Validate lemma length in Special:NewLexeme

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

Change 809572 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_37] SECURITY: Validate term length in Special:NewProperty

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

Change 809603 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_37] SECURITY: Validate lemma length in Special:NewLexeme

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

Change 809601 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: Validate term length in Special:NewProperty

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

Change 809604 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_35] SECURITY: Validate lemma length in Special:NewLexeme

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

Backported the patches to REL1_35 and REL1_37 (1.36 is out of support). Leaving the task open for review of the cleanup patch (which doesn’t need backporting).

Change 809203 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Clean up lemma length validation

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

In T308659#8037779, @Lucas_Werkmeister_WMDE wrote:

I think we’re done here (but please reopen if the task should still be open for security release process purposes).

Yes, looks good. This will be (re-)announced via the upcoming supplemental security release, due out tomorrow or early next week. And thanks for shepherding all of those additional backports.

Mentioned in SAL (#wikimedia-operations) [2022-07-20T13:54:10Z] <Lucas_WMDE> lucaswerkmeister-wmde@deploy1002 /srv/mediawiki-staging (master $ u=) $ git -C php-1.39.0-wmf.19/extensions/WikibaseLexeme am --skip # T308659 backport already applied