Page MenuHomePhabricator

Tilde stripping in signatures is inadequate
Open, Needs TriagePublic

Description

Editors have been banned for using tides in their signatures (among other things):
https://en.wikipedia.org/wiki/Wikipedia:Requests_for_arbitration/-Ril-#Sig_change

The issue is that if you set your 'fancy' signature to ~~~~ it gets inserted literally in the output, and then the next editor to save the talk page gets *their* signature substituted, causing the evil editor's comments to be misattributed.

Code was added in 2006 to prevent this: 682e6e96e035e95dc44c8f17ce050bb7c16f60e2

But this is ineffective. For example consider the signature ~~{{subst:1x{{subst:1x|{{subst:!}}}}}}~~. This gets expanded on first save to ~~{{subst:1x|}}~~ which will then be treated as ~~~~ on a subsequent save (subst is handled before signature insertion).

This isn't really a security bug: we have a policy in place forbidding confusing signatures in general, and the original author is stored in the version history. But it is a case where our attempted sanitization is imperfect.


Dependencies

The patches to meet the "Requirements" described below should NOT be merged until the following tickets are resolved:

Requirements

  • When someone attempts to save a signature that meets the conditions below, prevent the signature from being saved and present a message that explains to people: 1) that their signature cannot be saved as it is currently written, 2) why their signature cannot be saved as it is currently written and 3) what changes they need to make to the signature they have written in order to save it.
    • Conditions:
      • The signature contains syntax that produces another {{subst:...}} or ~~~... after the substitution (which would be then substituted again when the next user edits the page), as described in T230652#5966006

Testing details

Scenario A

  1. Visit: https://en.wikipedia.org/wiki/Special:Preferences
  2. Navigate to the Signature section
  3. Check the Treat the above as wiki markup box
  4. Enter a signature like: ~~{{subst:1x{{subst:1x|{{subst:!}}}}}}~~
  5. Click Save
  6. Notice the follow message appears beneath the Signature: text field: Your signature contains nested substitution (e.g. subst: or ~~~~).

Done

  • Patches are written that meet the "Requirements" described above

Related Objects

Event Timeline

cscott created this task.Aug 17 2019, 12:23 PM
Restricted Application added subscribers: Liuxinyu970226, Aklapper. · View Herald TranscriptAug 17 2019, 12:23 PM

Change 549953 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] [WIP] preferences: Prevent "nested" substitution in signature

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

matmarex claimed this task.Nov 9 2019, 12:36 AM

Change 549953 abandoned by Bartosz Dziewoński:
[WIP] preferences: Prevent "nested" substitution in signature

Reason:
It'll be easier for me to work on this and related changes in one patch, see: I07c575c2d9d2afe7a89c4847d16ac044417297bf

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

Change 569062 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] preferences: Signature validation (lint errors, user links, nested subst)

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

sbassett edited subscribers, added: sbassett; removed: Security-Team.EditedFeb 3 2020, 5:19 PM

Per the description - This isn't really a security bug - I'm going to un-sub Security-Team for now.

JTannerWMF added a subscriber: JTannerWMF.

Once we get community input as mentioned in T244519 we can work on this task.

Hello, as I read, this change targets "nested" subst's. Does this code fall into that? It changes User talk:Jack who built the house link to User talk:Jack who built the house#top if I post on my talk page.

— [[User:Jack who built the house|Jack who built the house]] {{subst:#ifeq: {{subst:FULLPAGENAME}} | {{subst:ns:3}}:Jack who built the house | ([[User talk:Jack who built the house#top|talk]]) | ([[User talk:Jack who built the house|talk]]) }}

No, it only disallows syntax that produces another {{subst:...}} or ~~~... after the substitution (which would be then substituted again when the next user edits the page).

I could not think of a better way to describe it, "nested" subst is not a very precise description. I'd love to update the documentation page with a better one :)

Xiplus added a subscriber: Xiplus.Mar 24 2020, 1:00 PM
Pelagic added a subscriber: Pelagic.EditedMay 8 2020, 10:08 AM

I experimented with inserting my local time in my sig. At one stage I had nested {{subst:#time … {{subst:#expr … {{subst:#time …}}}}}} (subst one, subst 'em all), which would be banned under this proposal.

Fortunately I discovered that #time can take parameters like "+10 hours" and do it in one call. But there are potentially other legitimate reasons for multi subst'ing in sigs.

If the actual test is more subtle per @matmarex's post T230652#5966006, then there shouldn't be much problem. Perhaps the title could say "chained subst" or "evil subst", rather than "nested"? In the final communication, you could use a phrase like "certain pathological uses of subst:" or "abuses of subst:" to make it clear that the exclusion will be tightly focussed.

ppelberg updated the task description. (Show Details)Jun 12 2020, 9:41 PM
ppelberg added a subscriber: ppelberg.

Task description updates
Adding the following sections to the task description; please let me know if anything looks unexpected.

  • Dependencies
  • Requirements
  • Done

Blocked
This task will be unblocked once the following tasks are resolved:

ppelberg updated the task description. (Show Details)Jun 12 2020, 9:53 PM

So… this is a duplicate of my ancient T14495, right? :-)

@Mormegil It's a bit different. Your task is about disallowing signatures that produce {{blah}} after substitution. This task is about disallowing signatures that produce ~~~~ or {{subst:blah}} (etc.) after substitution, which would then be substituted again after another person edits the page.

Change 569062 merged by jenkins-bot:
[mediawiki/core@master] preferences: Signature validation (lint errors, user links, nested subst)

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

ppelberg moved this task from Inbox to High Priority on the Editing QA board.Thu, Jul 2, 9:56 PM
ppelberg updated the task description. (Show Details)Tue, Jul 7, 7:46 PM
ppelberg added a subscriber: Ryasmeen.

Like in T237700#6286867, @matmarex can you:

  1. REVIEW the "Testing details" section of the task description to ensure it is accurate and exhaustive?

cc @Ryasmeen

@matmarex:

I followed the testing details and for signature entered this exactly: ~~{{subst:1x{{subst:1x|{{subst:!}}}}}}~~

And it does show the appropriate message as expected.

Is that all I need to check for this?

I tried couple of other things, although not sure if those are part of validation checks for this. For example: I tried adding tildes as part of my username with the checkbox being both checked and unchecked. For both cases, it discards the tildes after saving the changes, however it does not show any error messages.

Just thought to add this observation here.

@matmarex:

I followed the testing details and for signature entered this exactly: ~~{{subst:1x{{subst:1x|{{subst:!}}}}}}~~

And it does show the appropriate message as expected.

Is that all I need to check for this?

I think that's good enough…

If you want to dig at it, here are some more test cases (as used for unit tests), which might provide some inspiration: https://github.com/wikimedia/mediawiki/blob/master/tests/phpunit/includes/preferences/SignatureValidatorTest.php#L69-L72

We could also double-check that just using a template in the signature doesn't cause this error.

I tried couple of other things, although not sure if those are part of validation checks for this. For example: I tried adding tildes as part of my username with the checkbox being both checked and unchecked. For both cases, it discards the tildes after saving the changes, however it does not show any error messages.

Just thought to add this observation here.

That's pre-existing code, which indeed tries to automatically fix this problem by removing the tildes, but it only handles the simple cases. I'm not sure if we should try to remove that now, I think it's fine.