Page MenuHomePhabricator

In AuthManager, avoid encrypted storage of the password in the session
Open, MediumPublic

Description

In an IRC discussion relating to session storage T206010, concern was expressed that AuthManager's storage of the encrypted password in the session provides attackers with access to the plain text password under plausible threat models, such as remote code execution as the www-data user.

The reason AuthManager stores the password is because it is sent by the user on the initial POST request, and yet the password validity needs to be checked in the context of later requests.

My idea is to call a form processing method on all registered AuthenticationProvider instances when each form is submitted. This method will return a data array giving only the data the AuthenticationProvider will later need to authenticate the request. Then beginPrimaryAuthentication() will only be passed the pre-filtered data arrays previously returned by the form processing method, instead of all form data.

For example, LocalPasswordPrimaryAuthenticationProvider could just store a boolean value representing whether or not the password matched. This would minimise the amount of sensitive data stored in the session, although at the cost of allowing the login if the password was changed halfway through the authentication process. If it's a requirement to abort the authentication process when the password is changed, then a hash of the password could be stored, although this brings with it the risk of disclosing weak passwords to an attacker by hash inversion. A last-change timestamp or serial would be a more secure way to detect a change of password.

LocalPasswordPrimaryAuthenticationProvider also needs to know the password validity so it can set the reset-pass flag. The form processor could serialize the Status object it gets from User::checkPasswordValidity(), to avoid the need to call checkPasswordValidity() in a later request.

Event Timeline

My idea is to call a form processing method on all registered AuthenticationProvider instances when each form is submitted. This method will return a data array giving only the data the AuthenticationProvider will later need to authenticate the request. Then beginPrimaryAuthentication() will only be passed the pre-filtered data arrays previously returned by the form processing method, instead of all form data.

For example, LocalPasswordPrimaryAuthenticationProvider could just store a boolean value representing whether or not the password matched.

It would probably make more sense for the new method to work on the array of AuthenticationRequest objects, rather than the form data directly. Each provider's method would return new AuthenticationRequest objects, and AuthManager would merge and deduplicate the results.

Also the method would need to know which action it's being called for. Your example applies to login, while for account creation it needs the password hash to store in user.user_password instead.

I note this would mean that every password-using PrimaryAuthenticationProvider would have to check the password, even if an earlier provider is actually going to handle the login. That could be a good thing or a bad thing from the perspective of timing attacks and the like, I'm not sure.

If it's a requirement to abort the authentication process when the password is changed

I don't know of anything that tries to do that currently.

I imagine this would take the form of a new PrimaryAuthenticationProvider method (as it would not be useful for pre-auth providers which are only called on the initial request, nor for secondaries which can only take useful action when the identity of the user has been established by a primary) that's called somewhere at the end of beginAuthentication() - after the preauth checks have passed but before the beginPrimaryAuthentication() calls.

Each provider's method would return new AuthenticationRequest objects, and AuthManager would merge and deduplicate the results.

The nice way to handle this would IMO be inside AuthenticationRequest, by either special-casing the password field or having an AuthenticationRequest::getFieldInfo() flag similar to sensitive (or maybe just use that), and preventing the affected fields from being stored during serialization. That way there is no need to merge and to resolve conflicts. I'm not sure there is a sane migration path to get there without breaking extensions in ugly ways, though. The merging approach would make a piecewise migration easy, so I guess that's more practical.

If it's a requirement to abort the authentication process when the password is changed, then a hash of the password could be stored, although this brings with it the risk of disclosing weak passwords to an attacker by hash inversion.

When the user is forced to change password on login, MediaWiki does not currently check if they are resubmitting the old password, but it does sound like a reasonable feature. (Not relevant for Wikimedia sites, since we do not have expiring passwords.) Not sure if that's what you meant. It does not seem problematic to handle though, we just need to replace the password with a salted hash. An attacker with remote code execution can already recover hashes from the database so that does not disclose new information. The biggest problem (but still a small one) would be performance, as password hashing needs to be slow, although if it's implemented intelligently, for the average site (which only has one password-based auth provider) there would probably be no extra hashing needed.

chasemp triaged this task as Medium priority.Dec 9 2019, 4:41 PM
chasemp added a project: Security-Team.