(In reply to comment #0)

4. your username is shown in the top.

Not if you do not allow your browser to send requests from randomfax.net to wikimedia.org by using an add-on, like "RequestPolicy" for Firefox. ;)
Even after disabling that (and allowing cookies for randomfax.net, and checking that he.wikipedia.org displays me as logged in) I cannot reproduce the problem, however I guess that this is similar to bug 55887.

Pulling this into security because it is a privacy leak that we need to address.

There are two parts of the handshake that can expose the user's identity, /checkLoggedIn returns the centralauth gu_id. Then the final javascript has the username in the html.

We can fix the first part by keeping it server-side and referencing it by a random token.

The second part is a little more tricky, and I'm not seeing a way to do that without adding one more call back to the server. If we do an ajax call to the wiki to get the logged in user's name, then SOP will prevent other sites from reading the return.

We could return a script from /validateSession that makes an XHR call to /setCookies, and have that return json to update the html, but that seems a little more error-prone.

I was trying to work out a better solution to this, but I'm afraid we're going to have to add an extra round-trip to do the #p-personal update. Otherwise the flow gets a lot more complex.

I'll work up a patch to expose $skin->getPersonalToolsList(), and then we'll have a function pull that in and update #p-personal ul. Since IE <8 and Opera <12 can't do CORS, we'll probably need to expose this in a hidden iframe.

(In reply to comment #3)

Since IE <8 and Opera
<12 can't do CORS, we'll probably need to expose this in a hidden iframe.

How does the hidden iframe avoid exposing the username? The function reaches in to read out the data after the load completes?

I wonder if we could do this a lot simpler by transferring the username in a non-httponly cookie instead.

Created attachment 13834
Remove identifiers from AutoLogin process, get username via ajax

Realized since we're getting this from the originating wiki, legitimate users just need a simple ajax call to pull in the personal information to update the header.

This patch:

• Adds api module to get the header html. This is ugly, is there a better way without refactoring skins?
• Remove global userid from protocol (2x). Most browsers shouldn't be able to access those, but we can remove them, so let's do it.
• Remove direct html update of the #p-personal ul, and insert an ajax call that gets the html from the api.

Brad, I'd appreciate your input, and testing from anyone else who is able.

attachment bug57081.patch ignored as obsolete

Created attachment 13840
Remove identifiers from AutoLogin process, get username via ajax

Brad pointed out that the token from /checkLoggedIn for gu_id=0 would break caching, so this fixes that.

Also simplified the patch by moving the tools html call into Special:CentralAutoLogin, instead of using a separate api call.

attachment bug57081b.patch ignored as obsolete

We should probably add a call to $this->getOutput()->enableClientCache( false ); at around line 159. Otherwise it seems to me that it might be possible that someone hits /checkLoggedIn and gets a redirect with a token, and then they hit it again 2 minutes later (maybe their browser crashed, or their network connection dropped before they got to /setCookies) and get the cached reply with the now-expired token.

For the $.getJSON call, you need to pass jQuery in for a $ argument to the anonymous function like you do with mediaWiki and mw.

As written it looks like we'd need to push this to all wikis at once. Otherwise wikis updated before loginwiki will break for not getting token to /createSession, and then once loginwiki does get updated everything not already updated will break for not getting gu_id to /createSession. If the cache time isn't already short enough, we might also want to purge the caches for /checkLoggedIn so people don't get a cached redirect to /createSession with a gu_id.

If we want to ride the deployment train, we'd need an intermediate stage where /checkLoggedIn returns both a token and a gu_id and /createSession accepts gu_id as a fallback if token isn't given. That also gives two days for the cached /checkLoggedIn to expire without login disruption (or a week if we don't care about disruption logging in to Phase 1 wikis).

Created attachment 13891
Remove identifiers from AutoLogin process, get username via ajax

attachment bug57081c.patch ignored as obsolete

(In reply to comment #7)

We should probably add a call to $this->getOutput()->enableClientCache( false
); at around line 159. Otherwise it seems to me that it might be possible
that
someone hits /checkLoggedIn and gets a redirect with a token, and then they
hit
it again 2 minutes later (maybe their browser crashed, or their network
connection dropped before they got to /setCookies) and get the cached reply
with the now-expired token.

Done

For the $.getJSON call, you need to pass jQuery in for a $ argument to the
anonymous function like you do with mediaWiki and mw.

Done (I think I understand what you were asking)

As written it looks like we'd need to push this to all wikis at once.
Otherwise
wikis updated before loginwiki will break for not getting token to
/createSession, and then once loginwiki does get updated everything not
already
updated will break for not getting gu_id to /createSession. If the cache time
isn't already short enough, we might also want to purge the caches for
/checkLoggedIn so people don't get a cached redirect to /createSession with a
gu_id.

If we want to ride the deployment train, we'd need an intermediate stage
where
/checkLoggedIn returns both a token and a gu_id and /createSession accepts
gu_id as a fallback if token isn't given. That also gives two days for the
cached /checkLoggedIn to expire without login disruption (or a week if we
don't
care about disruption logging in to Phase 1 wikis).

This would be deployed as a security patch, so it would pretty much go to all wikis at the same time. However, the potential for cached responses is a good point, and we want to make sure those users get the localstorage script. So I accept both gu_id or a token in /createSession now.

The one issue I found in testing this is if a user is logged in on loginwiki, then hits a wiki anonymously via http, the call to /toolslist fails because changing protocol breaks SoP. Once gerrit 96317 is merged, that won't be an issue.

Created attachment 13894
Fix the JS

Sorry I didn't see these before.

The comment on toolslist is kind of weird, there aren't any obvious cookies going on there. But we probably still want to avoid caching for echo's number.

The three bits of JS immediately after the setting of the p-personal html need to go inside your callback, because they depend on being run after the html update. We can simplify things a little by passing returnto and returntoquery to the toolslist callback instead of trying to fix up the links in JS. This attachment shows what I'm talking about.

attachment diff ignored as obsolete

Sorry for the delay. I think brads additions to the patch look good. I'll deploy soon.

Created attachment 14064
Rebased patch

Brad's patch, rebased to account for https://gerrit.wikimedia.org/r/#/c/100508/.

attachment bug57081d.patch ignored as obsolete

Rebase looks good to me.

(In reply to comment #12)

Created attachment 14064 [details]
Rebased patch

Still needed:

• Call doesn't work if the visited site is http, and wgSecureLogin is enabled (call to /toolslist is redirected because the forceHTTPS cookie is set, so SoP prevents the result from being read).
• For users who isUIReloadRecommended returns true, username is leaked in the notification message.

I started updating the patch to remove the username from 'centralauth-centralautologin-logged-in' and show the notification if the call to /toolslist fails, but the we would need a way to get all those translations updated for a security patch, which would be difficult.

attachment bug57081d.patch ignored as obsolete

(In reply to comment #14)

I started updating the patch to remove the username from
'centralauth-centralautologin-logged-in' and show the notification if the
call
to /toolslist fails, but the we would need a way to get all those
translations
updated for a security patch, which would be difficult.

You could just make a new message instead of 'centralauth-centralautologin-logged-in'.

And for good measure we could use the new one only on /toolslist failure, if toolslist returns the username and $gender so we can fire the existing message from inside the /toolslist callback.

BTW, if you want me to code that up just let me know.

Created attachment 14114
Update notices too

Thanks Brad, I think this is what you were saying.

attachment bug57081e.patch ignored as obsolete

Seems to work. I note you have a stray statement at line 480 that could be deleted though.

Created attachment 14115
Don't return username from AutoLogin

Thanks Brad, updated for that. Unless you have any other thoughts, I'll schedule a deployment with Greg for tomorrow or Wed.

Attached:

This was assigned CVE-2013-6455

Both tomorrow or Wed already passed, is this going to be deployed after holidays?

Hi Matanya,

From Dec 17th SAL:
21:37 logmsgbot: csteipp synchronized php-1.23wmf7/extensions/CentralAuth 'bug57081'
21:36 logmsgbot: csteipp synchronized php-1.23wmf6/extensions/CentralAuth 'bug57081'

I just verified that wmf8 has the patch too (Sam added it before pushing it out).

We'll close this bug and make the issue public with the next security release. We're targeting Jan 13th for that.

Thanks a lot for the update! Glad to see it was deployed.

Older versions will not have this backported? I.E the latest stable release. Or is it wikimedia specific issue since it is centralauth?

It seems that the issue is still not fixed in wmf8, or wasn't merged to deployed version - wmf8 was already deployed and external sites (as http://randomfax.net/wiki/) are able to display user name of users of Wikipedia.

It appears that someone (accidentally, probably) removed the patch from the deployed versions of wmf8 and wmf9.

It should now be re-deployed to wmf8 and wmf9.