Page MenuHomePhabricator

Using the 'default' parameter of 'hidden' fields in html form descriptor to provide tokens may introduce vulnerabilities
Closed, DeclinedPublicSecurity

Description

I noticed there are several places that use the default parameter of hidden field in html form descriptor to provide tokens, there can be some vulnerabilities:
If the request data didn't contain the token, HTMLForm would load it from the default, which means the submit callback would receive the token from server directly.
The riskiest places I found are in the OAuth extension:
/src/Frontend/SpecialPages/SpecialMWOAuth.php#641
/src/Frontend/SpecialPages/SpecialMWOAuthConsumerRegistration.php#427
/src/Frontend/SpecialPages/SpecialMWOAuthManageConsumers.php#298
I didn't try to investigate these codes are actually vulnerable or not, but create this task as a security issue to make sure.
Other possible parts via codesearch: https://codesearch.wmcloud.org/search/?q=default.%2B%5BTt%5Doken&files=php%24
Most of them might be fine since they use $request->getVal() to handle tokens manually, but in case of further change, it would be better to avoid using hidden fields for tokens in the form descriptor.

One possible fix is to override loadDataFromRequest in the HTMLHiddenField class:

	public function loadDataFromRequest( $request ) {
		return $request->getText( $this->mName );
	}

Details

Risk Rating
Low
Author Affiliation
Wikimedia Communities

Event Timeline

Func renamed this task from Don't use the 'default' parameter of 'hidden' field in html field descriptor to provide tokens to Using the 'default' parameter of 'hidden' fields in html field descriptor to provide tokens may introduce vulnerabilities.Feb 10 2022, 3:48 PM
Func updated the task description. (Show Details)
Func renamed this task from Using the 'default' parameter of 'hidden' fields in html field descriptor to provide tokens may introduce vulnerabilities to Using the 'default' parameter of 'hidden' fields in html form descriptor to provide tokens may introduce vulnerabilities.Feb 10 2022, 3:56 PM
Func updated the task description. (Show Details)

(adding some of the authors or reviewers of affected codes)

Here's a less noisy search: https://codesearch.wmcloud.org/search/?q=%5B%27%22%5Ddefault%5B%27%22%5D.%2B%5BTt%5Doken&i=nope&files=php%24&excludeFiles=&repos=

For the purposes of CSRF protection, HTMLForm::tryAuthorizedSubmit() and things that call to it (like HTMLForm::show()) have built-in CSRF token handling that is not affected by this.

OAuth I think is a false positive - SpecialMWOAuth/authorize takes the value from the request, and the change token in the other two cases is a hash of the current app settings that's used for edit conflict detection and isn't particularly secret (an attacker could easily compute it).

ProtectionForm is a false positive, it takes the token value directly from the request.

AbuseFilter is the same, takens the value from the request.

WSOAuth is doing it on a special page which is submitting to the normal login form, not itself, so it will never be invoked when POSTing the form.

So no vulnerability as far as I can see.

Other takeaways:

  • HTMLForm doesn't seem to have any tests whatsoever ensuring that the CSRF check actually works. That's... not great.
  • HTMLForm lacks built-in support for using a custom CSRF token (ie. a token type other than csrf) forcing users to roll their own manual CSRF check, which is bad. (Maybe I'm missing something here? I would expect more than just the protection form to do it.)
  • HTMLForm lacks built-in support for using a custom CSRF token (ie. a token type other than csrf) forcing users to roll their own manual CSRF check, which is bad. (Maybe I'm missing something here? I would expect more than just the protection form to do it.)

I think setTokenSalt() can be used to do this, or am I misunderstanding what it does?

(The ProtectionForm example was just changed to use it in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761092 by @Func)

I think setTokenSalt() can be used to do this, or am I misunderstanding what it does?

Yeah, my bad.

Func removed projects: Security, Security-Team.

OAuth I think is a false positive - SpecialMWOAuth/authorize takes the value from the request, and the change token in the other two cases is a hash of the current app settings that's used for edit conflict detection and isn't particularly secret (an attacker could easily compute it).

Thanks! Then there is no use case using tokens for security purposes other than the edit token.

(I had checked other use cases, sorry for not mentioning that clearly in the description.)

@Func, @Tgr - I don't see anything that should prevent this from being made public? Any objections before I do so?

(Also, thanks for the audit)

sbassett triaged this task as Lowest priority.Feb 15 2022, 3:58 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Low.

Change 762136 merged by jenkins-bot:

[mediawiki/core@master] HTMLForm: Add test for CSRF token check

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