Page MenuHomePhabricator

CookieSetOnAutoblock doesn't verify cookie before using it allowing an attacker to enumerate revdeleted users
Closed, ResolvedPublic1 Story Points

Description

Anyone can set a BlockID cookie. This allows a user to discover revdeleted users, by trying different blockids, until they hit upon one that works, but isn't public.

We should include a MAC with the cookie value. (e.g. the cookie is $blockId . '!' . hash_hmac( 'sha256', $blockId, $wgSecretKey ); Before using the value, the hmac is verified to prevent spoofing)

Details

Related Gerrit Patches:
mediawiki/core : masterValidate BlockID cookie before use

Event Timeline

Bawolff created this task.Dec 12 2016, 12:06 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 12 2016, 12:06 PM
Bawolff renamed this task from CookieSetOnAutoblock doesn't verify cookie before using it to CookieSetOnAutoblock doesn't verify cookie before using it allowing an attacker to enumerate revdeleted users.Dec 12 2016, 12:47 PM
Bawolff updated the task description. (Show Details)
Bawolff added a project: Community-Tech.
Bawolff added a subscriber: Samwilson.
Bawolff triaged this task as Normal priority.Dec 13 2016, 10:34 PM

@Bawolff: Could elaborate a bit. What is the purpose of discovering revdeleted users?

Users are not supposed to be able to determine the names of revdeleted users (i.e. blocked users where ipb_deleted field is set) unless they have the hideuser right (aka are in the oversight group).

The current cookie based blocking thing allows an attacker to set a cookie with any block id, including block ids for blocks where ipb_deleted db field is set. This allows the attacker to discover information about such blocks even if they dont have the hideuser right.

)

Do you know what information about a revdeleted user is potentially exposed here? If not, we can test and see.

The user's name would be the primary info. Other block details would be exposed although i dont think oversighters care about that as much

kaldari updated the task description. (Show Details)
kaldari set the point value for this task to 1.
kaldari changed the visibility from "Custom Policy" to "Custom Policy".

@Samwilson: Don't forget to assign yourself to tasks when you're working on them.

Sorry, I meant to do that!

This is ready for review. https://gerrit.wikimedia.org/r/#/c/330368/

The SecretKey config var defaults to false, but is supposed to always be set. If it's not, the above patch will fail (as it was in my first patch set). Is this okay?

The test structure does not set SecretKey, so I'm setting it manually in the tests. This strikes me as perhaps not the right way — anyone got an opinion?

Samwilson added a subscriber: TTO.Feb 3 2017, 4:30 AM

@TTO do you have an idea about the above?

TTO added a comment.Feb 3 2017, 4:51 AM

The test structure does not set SecretKey, so I'm setting it manually in the tests. This strikes me as perhaps not the right way — anyone got an opinion?

The comment in DefaultSettings for $wgSecretKey says in its entirety: "This should always be customised in LocalSettings.php". However, https://www.mediawiki.org/wiki/Manual:$wgSecretKey says that it is in fact not used on "modern PHP versions" (which versions?). So it's plausible that some installs will have left it unset.

So it actually seems that the tests should be confirming that this feature still works when $wgSecretKey is false. I'm sure @Bawolff could provide more insight on this and correct me if I'm wrong.

If $wgSecretKey is false, this feature should be turned off completely shouldn't it? And just the block ID stored in the cookie, on its own?

Alternatively, it could throw an error if $wgCookieSetOnAutoblock is true but $wgSecretKey is not set.

Anomie added a subscriber: Anomie.Feb 3 2017, 5:57 PM

The comment in DefaultSettings for $wgSecretKey says in its entirety: "This should always be customised in LocalSettings.php". However, https://www.mediawiki.org/wiki/Manual:$wgSecretKey says that it is in fact not used on "modern PHP versions" (which versions?). So it's plausible that some installs will have left it unset.

That comment on Manual:$wgSecretKey is referring to the use of the variable as a source of entropy when generating user_tokens. It's still used for other purposes, for example it's used for HKDF when $wgHKDFSecret isn't set and for session data encryption when $wgSessionSecret isn't set.

I've added another test to make sure it still works with a non-authenticated cookie. Also added a note to defaultsettings to say that it should really rather be used with $wgSecretKey.

kaldari closed this task as Resolved.Feb 13 2017, 11:45 PM
kaldari moved this task from Needs Review/Feedback to Q1 2018-19 on the Community-Tech-Sprint board.

@Bawolff In what way did the previous situation result in leaking of user names? It isn't mentioned here or in the patch, but I suspect this may be about a user interface feature. I suspect there may be something somewhere along the lines of "Hello, you are not logged-in but we see your cookie from when you were logged-in as <Name of UserID>, so we blocked you again." But.. I can't think of any reason why we display something like that. We don't, right?

So I guess this is not about exposure of rev_user_text based on user id (after rev_deleted), but rather about something specific to the user's overall status (after ipb_deleted). But it's unclear to me where that is used. Exactly where/how do we show users names in a way that is normally hidden but (intentionally?) ignores the hidden status if a matching BlockID cookie exists?

It sounds to me like those cases may not need to display such specific information in the first place.

By revdeleted users, i mean the hideuser right (aka ipb_deleted in db)

User could set cookie, and you are blocked message revealed the target of the block, which is supposed to be secret if ipb_deleted is set

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 11 2018, 4:02 AM