Login API doesn't prevent getting csrf tokens via jsonp
Closed, ResolvedPublic

Description

curl --data "action=login&lgname=User1&lgpassword=xxx&format=json&callback=foo" 'http://localhost/wiki/api.php'
foo({"login":{"result":"NeedToken","token":"317d0a5134f80898af6c619115293e1f","cookieprefix":"wiki_test1","sessionid":"p7sgh8o7h2ltsi5635mh19h6c2pg7me2pekaee84kdq3k68gr1l0"}})


Version: unspecified
Severity: normal

bzimport added a project: MediaWiki-API.Via ConduitNov 22 2014, 1:44 AM
bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz49090.
csteipp created this task.Via LegacyJun 3 2013, 8:10 PM
csteipp added a comment.Via ConduitJun 4 2013, 2:01 AM

I audited the api modules in core, and a few others will give back login or anonymous edit tokens too. These need to be fixed before we can effectively fix bug 38417.

Anomie added a comment.Via ConduitJun 4 2013, 2:27 AM

I'm planning to look at this tomorrow morning. Current thoughts are that the block and unblock 'gettoken' parameters have been deprecated since 1.20, so they are ripe for removal. The others will get a check similar to that currently used for prop=info.

I also grepped through the WMF-deployed extensions. They all seem to use the generic edit token from action=tokens or prop=info, or use the hook to add to action=tokens, so that part of it looks ok.

I also note that ApiQueryDeletedrevs could give a token, but since anons don't typically have 'deletedhistory' it's not likely to actually be gettable via jsonp.

csteipp added a comment.Via ConduitJun 4 2013, 3:03 AM

(In reply to comment #2)

I'm planning to look at this tomorrow morning. Current thoughts are that the
block and unblock 'gettoken' parameters have been deprecated since 1.20, so
they are ripe for removal. The others will get a check similar to that
currently used for prop=info.

If we want to fix 38417 for REL1_19/REL1_20, then we'll probably need a patch that either checks for the callback param, or moves the gettoken handling after the permission checks. Either way is a pretty simple patch. But yes, we should remove the code altogether for 1.22 I think.

Anomie added a comment.Via ConduitJun 4 2013, 3:37 PM

Created attachment 12450
Patch against master

attachment patch-1.22.diff ignored as obsolete

Anomie added a comment.Via ConduitJun 4 2013, 3:37 PM

Created attachment 12451
Patch against REL1_21

attachment patch-1.21.diff ignored as obsolete

Anomie added a comment.Via ConduitJun 4 2013, 3:37 PM

Created attachment 12452
Patch against REL1_20

attachment patch-1.20.diff ignored as obsolete

Anomie added a comment.Via ConduitJun 4 2013, 3:38 PM

Created attachment 12453
Patch against REL1_19

attachment patch-1.19.diff ignored as obsolete

Anomie added a comment.Via ConduitJun 4 2013, 5:01 PM

Created attachment 12456
Patch against master

Attached: patch-1.22.diff

Anomie added a comment.Via ConduitJun 4 2013, 5:01 PM

Created attachment 12457
Patch against REL1_21

Attached: patch-1.21.diff

Anomie added a comment.Via ConduitJun 4 2013, 5:01 PM

Created attachment 12458
Patch against REL1_20

Attached: patch-1.20.diff

Anomie added a comment.Via ConduitJun 4 2013, 5:02 PM

Created attachment 12459
Patch against REL1_19

Attached: patch-1.19.diff

csteipp added a comment.Via ConduitJun 4 2013, 5:21 PM

Thanks Brad! I've tested the patch against master, and it seems to be working well and prevents the leak. As mentioned on irc, the action=tokens api returns the error "Unrecognized value for parameter 'type'" when the callback is added, instead of an explicit message about the callback parameter. But since this is the same way ApiQueryInfo currently works, it seems like api users should be able to figure it out.

Roan, can you take a look at the patches, and verify they look sane to you, or if there is anything we're missing by doing it this way?

csteipp added a comment.Via ConduitSep 3 2013, 8:39 PM

Roan approved on irc. Deployed into production on Aug 29.

gerritbot added a comment.Via ConduitSep 3 2013, 10:11 PM

Change 82527 merged by jenkins-bot:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 3 2013, 10:34 PM

Change 82537 had a related patch set uploaded by CSteipp:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 3 2013, 10:39 PM

Change 82541 had a related patch set uploaded by CSteipp:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 3 2013, 10:42 PM

Change 82545 had a related patch set uploaded by CSteipp:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 3 2013, 10:57 PM

Change 82537 merged by CSteipp:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 4 2013, 12:27 AM

Change 82545 merged by jenkins-bot:
SECURITY: Prevent tokens in jsonp mode

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

gerritbot added a comment.Via ConduitSep 4 2013, 3:49 AM

Change 82541 merged by jenkins-bot:
SECURITY: Prevent tokens in jsonp mode

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

Aklapper added a comment.Via ConduitSep 4 2013, 9:30 AM

[restoring RESOLVED FIXED state which was set before the Gerrit Notification Bot inserted links to the Gerrit patchsets]

csteipp added a comment.Via ConduitSep 5 2013, 5:00 PM

This issue was assigned CVE-2013-4302

csteipp added a project: Security.Via WebMar 26 2015, 8:39 PM

Add Comment