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)
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Validate BlockID cookie before use | mediawiki/core | master | +175 -29 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • TBolliger | T120734 Epic ⚡️ Improve MediaWiki's blocking tools | |||
| Resolved | MusikAnimal | T5233 Send a cookie with each block | |||
| Resolved | Samwilson | T152951 CookieSetOnAutoblock doesn't verify cookie before using it allowing an attacker to enumerate revdeleted users |
@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
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?
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.
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.
@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