Page MenuHomePhabricator
Create Task
Maniphest T212071

Missing CSRF token in scribunto api console
Closed, ResolvedPublic
Actions

Description

From pentest: ApiScribuntoConsole (action=scribunto-console) does not require a CSRF token.

Looking at it right now, it also accepts GET requests.

I'm not sure this is an actual issue:

  • Scribunto sessions are extremely ephemeral.
  • What would an attacker even gain by messing with someone's scribunto debug session?
  • Each session has a 31 bit (non-cryptographically secure. i.e. generated via mt_rand()) random identifier associated with it. The attacker would have to be able to guess this. Given its only 31 bits, and its not securely generated, this isn't outside the realm of possibility, but it also makes things harder to pull off.

That said, no harm in doing things properly. So I think we should do the normal, require CSRF token and make this module must POST.

Details

SubjectRepoBranchLines +/-
Require CSRF token for action=scribunto-consolemediawiki/extensions/Scribuntomaster+5 -1
Customize query in gerrit

Event Timeline

Bawolff created this task.Dec 16 2018, 7:36 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 16 2018, 7:36 AM
Bawolff added a project: Scribunto.Dec 16 2018, 7:36 AM
Anomie triaged this task as Low priority.EditedDec 17 2018, 2:35 PM
Anomie subscribed.

I'm not sure this is an actual issue:

I don't think it is.

  • Scribunto sessions are extremely ephemeral.

Indeed. It's not saved anywhere persistent client-side so it's gone as soon as the person leaves or reloads the page, plus it's thrown away if the user clicks the "Clear" button or changes the textarea in the edit form.

  • What would an attacker even gain by messing with someone's scribunto debug session?

The "session" here is just the history of commands previously typed into the console and a copy of the previous value of the content parameter, so they don't have to be repeated for every request. The best an attacker could do would be to screw up the Lua code being run, either to confuse someone or to hope they're going to copy-paste output.

  • Each session has a 31 bit (non-cryptographically secure. i.e. generated via mt_rand()) random identifier associated with it. The attacker would have to be able to guess this. Given its only 31 bits, and its not securely generated, this isn't outside the realm of possibility, but it also makes things harder to pull off.

Note the cache key also includes the user's ID. So the attacker would either have to also be able to spoof the user somehow or restrict the attack to people using the console while logged out.

We generally require POSTs and CSRF tokens for actions that actually write the wiki. This doesn't, other than that ephemeral state cache.

• chasemp added a project: Security.Feb 10 2020, 11:00 PM
• chasemp removed a project: acl*security.Feb 20 2020, 8:08 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:02 PM
Legoktm added subscribers: sbassett, Reedy, Legoktm.Sep 23 2022, 6:41 AM

@sbassett, @Reedy: two questions, 1) this seems very unexploitable, can we make it public? 2) Do you all have an opinion on whether this should require a CSRF token? It's an internal API module so we can make the change without having to bother with the breaking change process.

sbassett added projects: SecTeam-Processed, Vuln-CSRF.Sep 27 2022, 5:06 PM
In T212071#8254923, @Legoktm wrote:
  1. this seems very unexploitable, can we make it public?

Given @Bawolff and Anomie's reasoning above, I think that's fine.

  1. Do you all have an opinion on whether this should require a CSRF token? It's an internal API module so we can make the change without having to bother with the breaking change process.

IMO if we're going to expend energy to fix an issue like this, we should just go ahead and require the token unless there is fear that it will break several existing workflows, etc. Digging around in turnilo, it seems action=scribunto-console endpoints get a pretty small amount of traffic each month, like under 2k hits, most likely.

sbassett lowered the priority of this task from Low to Lowest.Sep 27 2022, 5:07 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
gerritbot added a comment.Sep 30 2022, 12:58 AM

Change 836962 had a related patch set uploaded (by Legoktm; author: Legoktm):

[mediawiki/extensions/Scribunto@master] Require CSRF token for action=scribunto-console

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

gerritbot added a project: Patch-For-Review.Sep 30 2022, 12:59 AM
Legoktm added a project: User-notice.Sep 30 2022, 1:04 AM

Scripts to fix: https://global-search.toolforge.org/?q=%22scribunto-console%22&namespaces=2%2C4%2C8&title=%28Gadgets-definition%7C.*%5C.js%29

Legoktm added a comment.Sep 30 2022, 1:29 AM

https://meta.wikimedia.org/w/index.php?title=Tech%2FNews%2F2022%2F40&type=revision&diff=23872498&oldid=23871998

I'm pretty concerned that there are gadgets that are calling this API :/ will file a separate task for obsoleting libLua.js.

Anyways, I double-checked, passing the token when it's not needed just results in an API warning:

"warnings": {
  "main": {
    "*": "Unrecognized parameter: token."
  }
},

so it's safe for people to update to using .postWithToken( 'csrf', params ) ASAP.

Legoktm claimed this task.Sep 30 2022, 1:29 AM
Quiddity moved this task from To Triage to In current Tech/News draft on the User-notice board.Sep 30 2022, 7:59 PM
gerritbot added a comment.Oct 5 2022, 2:38 PM

Change 836962 merged by jenkins-bot:

[mediawiki/extensions/Scribunto@master] Require CSRF token for action=scribunto-console

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.5; 2022-10-10).Oct 5 2022, 3:00 PM
Maintenance_bot removed a project: Patch-For-Review.Oct 5 2022, 3:30 PM
Quiddity moved this task from In current Tech/News draft to Already announced/Archive on the User-notice board.Oct 6 2022, 11:37 PM
Legoktm closed this task as Resolved.Oct 7 2022, 4:14 PM

Not planning to do any backports given the low risk balanced with changing an API.

Maintenance_bot edited projects, added User-notice-archive; removed User-notice.Oct 17 2022, 4:30 PM
MrStradivarius subscribed.Oct 30 2022, 1:19 AM
In T212071#8274383, @Legoktm wrote:

Scripts to fix: https://global-search.toolforge.org/?q=%22scribunto-console%22&namespaces=2%2C4%2C8&title=%28Gadgets-definition%7C.*%5C.js%29

I went through and fixed all of these.

sbassett added a comment.Nov 1 2022, 4:09 PM
In T212071#8355028, @MrStradivarius wrote:
In T212071#8274383, @Legoktm wrote:

Scripts to fix: https://global-search.toolforge.org/?q=%22scribunto-console%22&namespaces=2%2C4%2C8&title=%28Gadgets-definition%7C.*%5C.js%29

I went through and fixed all of these.

Thanks!

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits