Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require a nonce be present for revalidate POST requests. #575

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented May 31, 2023

What?

Two-Factor includes a nonce during the validate_2fa callback, and while the fields are present in the POST request for the revalidate endpoint, it's not used.

Props @xknown for the report.

Why?

A nonce should be present on all POST requests that perform actions, to prevent potential CSRF attacks.

How?

Due to the revalidation occurring with an authenticated session, wp_create_nonce() is used to validate the request. The Two-Factor login_nonce functionality is not used, to ensure that the revalidate nonce can't be used to login a new session.

The nonce is ignored during GET requests for the revalidate endpoint.

  • The initial load of the revalidate screen can be sans-nonce, this is not a concern IMHO.
  • The change-provider link DOES include a nonce, but it is not validated. This is due to code-reuse between the validate_2fa and revalidate_2fa actions. The inclusion of the nonce here that is not validated is not a risk, due to it being a GET request and not performing an action.

The check could be changed to if ( ( $is_post_request || $nonce ) && ! wp_verify_nonce( $nonce .. ) ) { if required, but it seems more explicit to only require it for POST requests, as that's where it's actually protecting against an attack.

Testing Instructions

  1. Apply PR
  2. Login as normal.
  3. During revalidate, remove the wp-auth-nonce from the POST payload, ensure the request fails.
  4. Ensure revalidate succeeds with the wp-auth-nonce field.

Screenshots or screencast

Changelog Entry

N/A - Unreleased feature.

@dd32 dd32 added this to the 0.9.0 milestone May 31, 2023
@dd32 dd32 requested a review from xknown May 31, 2023 03:10
@dd32 dd32 self-assigned this May 31, 2023
@dd32 dd32 merged commit f21ed3c into WordPress:master Jun 1, 2023
@iandunn iandunn mentioned this pull request Jun 1, 2023
14 tasks
@kasparsd kasparsd mentioned this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant