Skip to content

Commit 1422dcd

Browse files
masteradhocmasteradhoc
authored
Merge pull request #833 from PANawkar/831-local
#831 Fix nonce verification in 2FA flow
2 parents 07be7c3 + 6a1ac8e commit 1422dcd

3 files changed

Lines changed: 68 additions & 24 deletions

File tree

‎class-two-factor-core.php‎

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,12 @@ public static function get_user_two_factor_revalidate_url( $interim = false ) {
529529
* @return boolean
530530
*/
531531
public static function is_valid_user_action( $user_id, $action ) {
532-
$request_nonce = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : '';
532+
$request_nonce_raw = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Value sanitized and then only passed to wp_verify_nonce().
533+
if ( ! is_scalar( $request_nonce_raw ) ) {
534+
$request_nonce = '';
535+
} else {
536+
$request_nonce = sanitize_text_field( (string) $request_nonce_raw );
537+
}
533538

534539
if ( ! $user_id || ! $action || ! $request_nonce ) {
535540
return false;
@@ -550,8 +555,8 @@ public static function is_valid_user_action( $user_id, $action ) {
550555
*/
551556
public static function current_user_being_edited() {
552557
// Try to resolve the user ID from the request first.
553-
if ( ! empty( $_REQUEST['user_id'] ) ) {
554-
$user_id = intval( $_REQUEST['user_id'] );
558+
if ( ! empty( $_REQUEST['user_id'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in trigger_user_settings_action() via is_valid_user_action() before any state change.
559+
$user_id = intval( $_REQUEST['user_id'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in trigger_user_settings_action() via is_valid_user_action() before any state change.
555560

556561
if ( current_user_can( 'edit_user', $user_id ) ) {
557562
return $user_id;
@@ -570,8 +575,9 @@ public static function current_user_being_edited() {
570575
* @return void
571576
*/
572577
public static function trigger_user_settings_action() {
573-
$action = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) : '';
574-
$user_id = self::current_user_being_edited();
578+
$action_raw = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Value sanitized below; nonce verified in is_valid_user_action() before do_action.
579+
$action = ( is_scalar( $action_raw ) && (string) $action_raw !== '' ) ? sanitize_key( (string) $action_raw ) : '';
580+
$user_id = self::current_user_being_edited();
575581

576582
if ( self::is_valid_user_action( $user_id, $action ) ) {
577583
/**
@@ -984,7 +990,7 @@ public static function show_two_factor_login( $user ) {
984990
wp_die( esc_html__( 'Failed to create a login nonce.', 'two-factor' ) );
985991
}
986992

987-
$redirect_to = isset( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : admin_url();
993+
$redirect_to = isset( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : admin_url(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Value only used for redirect; auth protected by 2FA login nonce later.
988994

989995
self::login_html( $user, $login_nonce['key'], $redirect_to );
990996
}
@@ -1038,6 +1044,12 @@ public static function maybe_show_reset_password_notice( $errors ) {
10381044
return $errors;
10391045
}
10401046

1047+
// Verify login form nonce when present (e.g. wp-login.php); skip only when nonce is not sent (custom login forms).
1048+
if ( isset( $_POST['_wpnonce'] ) && ! wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['_wpnonce'] ) ), 'log-in' ) ) {
1049+
return $errors;
1050+
}
1051+
1052+
// phpcs:ignore WordPress.Security.NonceVerification.Missing -- Nonce verified above when _wpnonce present; absent for custom login forms.
10411053
$user_name = sanitize_user( wp_unslash( $_POST['log'] ) );
10421054
$attempted_user = get_user_by( 'login', $user_name );
10431055
if ( ! $attempted_user && str_contains( $user_name, '@' ) ) {
@@ -1558,11 +1570,11 @@ public static function rest_api_can_edit_user_and_update_two_factor_options( $us
15581570
* @since 0.2.0
15591571
*/
15601572
public static function login_form_validate_2fa() {
1561-
$wp_auth_id = ! empty( $_REQUEST['wp-auth-id'] ) ? absint( $_REQUEST['wp-auth-id'] ) : 0;
1562-
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : '';
1563-
$provider = ! empty( $_REQUEST['provider'] ) ? wp_unslash( $_REQUEST['provider'] ) : '';
1564-
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? wp_unslash( $_REQUEST['redirect_to'] ) : '';
1565-
$is_post_request = ( 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ) );
1573+
$wp_auth_id = ! empty( $_REQUEST['wp-auth-id'] ) ? absint( $_REQUEST['wp-auth-id'] ) : 0; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() via verify_login_nonce() before any use.
1574+
$nonce = ( isset( $_REQUEST['wp-auth-nonce'] ) && is_scalar( $_REQUEST['wp-auth-nonce'] ) ) ? sanitize_text_field( wp_unslash( (string) $_REQUEST['wp-auth-nonce'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.
1575+
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.
1576+
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.
1577+
$is_post_request = isset( $_SERVER['REQUEST_METHOD'] ) && 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- REQUEST_METHOD is not user input.
15661578
$user = get_user_by( 'id', $wp_auth_id );
15671579

15681580
if ( ! $wp_auth_id || ! $nonce || ! $user ) {
@@ -1624,7 +1636,7 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
16241636
delete_user_meta( $user->ID, self::USER_FAILED_LOGIN_ATTEMPTS_KEY );
16251637

16261638
$rememberme = false;
1627-
if ( isset( $_REQUEST['rememberme'] ) && $_REQUEST['rememberme'] ) {
1639+
if ( ! empty( $_REQUEST['rememberme'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.
16281640
$rememberme = true;
16291641
}
16301642

@@ -1665,7 +1677,7 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
16651677
$interim_login = isset( $_REQUEST['interim-login'] ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited,WordPress.Security.NonceVerification.Recommended
16661678

16671679
if ( $interim_login ) {
1668-
$customize_login = isset( $_REQUEST['customize-login'] );
1680+
$customize_login = isset( $_REQUEST['customize-login'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.
16691681
if ( $customize_login ) {
16701682
wp_enqueue_script( 'customize-base' );
16711683
wp_add_inline_script(
@@ -1699,10 +1711,10 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
16991711
* @since 0.9.0
17001712
*/
17011713
public static function login_form_revalidate_2fa() {
1702-
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : '';
1703-
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : false;
1704-
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? wp_unslash( $_REQUEST['redirect_to'] ) : admin_url();
1705-
$is_post_request = ( 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ) );
1714+
$nonce = ( isset( $_REQUEST['wp-auth-nonce'] ) && is_scalar( $_REQUEST['wp-auth-nonce'] ) ) ? sanitize_text_field( wp_unslash( (string) $_REQUEST['wp-auth-nonce'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
1715+
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : false; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
1716+
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : admin_url(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
1717+
$is_post_request = isset( $_SERVER['REQUEST_METHOD'] ) && 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- REQUEST_METHOD is not user input.
17061718

17071719
self::_login_form_revalidate_2fa( $nonce, $provider, $redirect_to, $is_post_request );
17081720
exit;
@@ -2559,11 +2571,7 @@ public static function get_current_user_session() {
25592571
* @return boolean
25602572
*/
25612573
public static function rememberme() {
2562-
$rememberme = false;
2563-
2564-
if ( ! empty( $_REQUEST['rememberme'] ) ) {
2565-
$rememberme = true;
2566-
}
2574+
$rememberme = ! empty( $_REQUEST['rememberme'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Non-destructive display/flow flag; value normalized to bool below.
25672575

25682576
/**
25692577
* Filters whether the login session should persist between browser sessions.

‎providers/class-two-factor-provider.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,11 @@ public static function get_code( $length = 8, $chars = '1234567890' ) {
173173
* @return false|string Auth code on success, false if the field is not set or not expected length.
174174
*/
175175
public static function sanitize_code_from_request( $field, $length = 0 ) {
176-
if ( empty( $_REQUEST[ $field ] ) ) {
176+
if ( empty( $_REQUEST[ $field ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Caller (core) verifies nonce before provider processing.
177177
return false;
178178
}
179179

180-
$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, handled by the core method already.
180+
$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Caller (core) verifies nonce; value normalized below.
181181
$code = preg_replace( '/\s+/', '', $code );
182182

183183
// Maybe validate the length.

‎tests/class-two-factor-core.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,42 @@ public function test_reset_notice_when_password_was_reset() {
867867
$this->assertSame( 'two_factor_password_reset', $errors->get_error_code() );
868868
}
869869

870+
/**
871+
* Test no reset notice when _wpnonce is present but invalid.
872+
*
873+
* @covers Two_Factor_Core::maybe_show_reset_password_notice()
874+
*/
875+
public function test_no_reset_notice_when_wpnonce_invalid() {
876+
$user = self::factory()->user->create_and_get();
877+
$errors = new WP_Error( 'incorrect_password', 'Incorrect password' );
878+
$_POST['log'] = $user->user_login;
879+
$_POST['_wpnonce'] = 'invalid-nonce';
880+
881+
update_user_meta( $user->ID, Two_Factor_Core::USER_PASSWORD_WAS_RESET_KEY, true );
882+
Two_Factor_Core::maybe_show_reset_password_notice( $errors );
883+
$this->assertCount( 1, $errors->get_error_codes() );
884+
$this->assertSame( 'incorrect_password', $errors->get_error_code() );
885+
unset( $_POST['_wpnonce'] );
886+
}
887+
888+
/**
889+
* Test reset notice when _wpnonce is present and valid for log-in.
890+
*
891+
* @covers Two_Factor_Core::maybe_show_reset_password_notice()
892+
*/
893+
public function test_reset_notice_when_wpnonce_valid() {
894+
$user = self::factory()->user->create_and_get();
895+
$errors = new WP_Error( 'incorrect_password', 'Incorrect password' );
896+
$_POST['log'] = $user->user_login;
897+
$_POST['_wpnonce'] = wp_create_nonce( 'log-in' );
898+
899+
update_user_meta( $user->ID, Two_Factor_Core::USER_PASSWORD_WAS_RESET_KEY, true );
900+
Two_Factor_Core::maybe_show_reset_password_notice( $errors );
901+
$this->assertCount( 1, $errors->get_error_codes() );
902+
$this->assertSame( 'two_factor_password_reset', $errors->get_error_code() );
903+
unset( $_POST['_wpnonce'] );
904+
}
905+
870906
/**
871907
* Test clear password reset notice.
872908
*

0 commit comments

Comments
 (0)