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

Issue-49: Add nonce to wp-login #56

Merged
merged 21 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 194 additions & 0 deletions src/alley/wp/alleyvate/features/class-login-nonce.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
<?php
/**
* Class file for Login_Nonce
*
* (c) Alley <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-alleyvate
*/

namespace Alley\WP\Alleyvate\Features;

use Alley\WP\Alleyvate\Feature;

/**
* Adds a nonce field to the login form.
*/
final class Login_Nonce implements Feature {

/**
* The name to use for the nonce.
*
* @var string
*/
private string $nonce_name;
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved

/**
* The action to use for the nonce.
*
* @var string
*/
private string $nonce_action;

/**
* The nonce lifetime, in minutes.
*
* @var int
*/
private int $nonce_timeout;

/**
* Boot the feature.
*/
public function boot(): void {
add_action( 'login_init', [ $this, 'initialize_nonce_fields' ] );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
add_action( 'login_head', [ $this, 'add_meta_refresh' ] );
add_action( 'login_form', [ $this, 'add_nonce_to_form' ] );
add_filter( 'authenticate', [ $this, 'validate_login_nonce' ], 9999, 3 );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Add a meta refresh to the login page, so it will refresh after the nonce timeout.
*/
public function add_meta_refresh(): void {
if ( ! ( (bool) apply_filters( 'alleyvate_nonce_meta_refresh', true ) ) ) {
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
return;
}

echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonce timeout is stored in minutes but is used directly in the meta refresh tag, which expects seconds.

- echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) );
+ echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout * 60 ) );

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) );
echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout * 60 ) );

}

/**
* Add the nonce field to the form.
*/
public function add_nonce_to_form(): void {
wp_nonce_field( $this->nonce_action, $this->nonce_name );
}

/**
* Initializes the nonce fields. Is only run on `login_init` to restrict nonce data to login page.
*/
public function initialize_nonce_fields(): void {
/**
* Filters the nonce name.
*
* @param string $nonce_name The nonce name.
*/
$this->nonce_name = (string) apply_filters( 'alleyvate_nonce_name', $this->generate_random_nonce_name( 'alleyvate_login_nonce' ) );

/**
* Filters the nonce action name.
*
* @param string $nonce_action The action name.
*/
$this->nonce_action = (string) apply_filters( 'alleyvate_nonce_action', 'alleyvate_login_action' );

/**
* Filters the lifetime of the nonce, in minutes.
*
* @param int $timeout The lifetime of the nonce, in minutes. Default 30.
*/
$this->nonce_timeout = ( (int) apply_filters( 'alleyvate_nonce_timeout', 30 ) ) * 60;
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved

add_filter( 'nonce_life', fn() => $this->nonce_timeout );
}

/**
* Validates the passed nonce is valid. Returns a user object on valid login,
* or void on invalid nonce.
*
* @param \WP_User|\WP_Error|null $user The result of previous login validations. An instance of
* WP_User if all validations have passed previously.
* @param string $username The username used to try to login.
* @param string $password The password used to try to login.
* @return \WP_User|\WP_Error
*/
public function validate_login_nonce( $user, $username, $password ) {
/*
* If the filter is returning a \WP_Error, then validation has already failed.
* No need to check the nonce.
*/
if ( $user instanceof \WP_Error ) {
return $user;
}

/*
* We can't be sure when this filter will be triggered. Since we always need
* this filter triggered last, in the case of `$user` coming through as null
* lets run authentications again, and make sure.
*
* In a perfect world, this block will never run, but if it needs to be run,
* we want it to run.
*/
if ( null === $user ) {
// Remove this check to avoid infinite loops.
remove_filter( 'authenticate', [ $this, 'validate_login_nonce' ], 9999 );
$user = apply_filters( 'authenticate', $user, $username, $password ); // phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound
}

// We're sure about this value now. Exit early.
if ( ! $user instanceof \WP_User ) {
return $user;
}

/*
* Now that we've manually run all of the authentication filters,
* and we know we have a valid login attempt, remove all the filters
* so we don't risk bypassing our nonce validation down the line.
*/
remove_all_filters( 'authenticate' );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of remove_all_filters on the 'authenticate' hook could still pose a security risk by removing all authentication checks. Please ensure that this action does not compromise the security of the system.


// If no post data exists, no nonce data exists, which means our login is invalid.
if ( empty( $_POST ) ) {
return new \WP_Error(
'nonce_failure',
__( 'Login attempt timed out. Please try again.', 'alley' )
);
}
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved

$nonce = false;

if ( ! empty( $_POST[ $this->nonce_name ] ) ) {
$nonce = sanitize_key( $_POST[ $this->nonce_name ] );
}

if ( ! $nonce ) {
return new \WP_Error(
'nonce_failure',
__( 'Login attempt timed out. Please try again.', 'alley' )
);

}

$nonce_validation = wp_verify_nonce( $nonce, $this->nonce_action );

if ( ! $nonce_validation ) {
return new \WP_Error(
'nonce_failure',
__( 'Login attempt timed out. Please try again.', 'alley' )
);
}

return $user;
}

/**
* Randomize the nonce name using the data from the $_SERVER super global, and a provided salt.
*
* @param string $name The salt value.
* @return string
*/
private function generate_random_nonce_name( string $name ): string {
$parts = [ $name ];
if ( ! empty( $_SERVER ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of $_SERVER variables should be accompanied by isset checks to prevent PHP notices if these variables are not set.

- if ( ! empty( $_SERVER ) ) {
+ if ( isset( $_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_X_FORWARDED_FOR'], $_SERVER['HTTP_CLIENT_IP'] ) ) {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if ( ! empty( $_SERVER ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders
if ( isset( $_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_X_FORWARDED_FOR'], $_SERVER['HTTP_CLIENT_IP'] ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders

Using user-controlled headers such as 'HTTP_X_FORWARDED_FOR' and 'HTTP_CLIENT_IP' in the nonce generation process could be a security risk if these headers are spoofed. Consider using server-side generated data that cannot be manipulated by the user.

foreach ( [ 'REMOTE_ADDR', 'HTTP_X_FORWARDED_FOR', 'HTTP_CLIENT_IP' ] as $key ) {
$value = ! empty( $_SERVER[ $key ] ) ? sanitize_key( $_SERVER[ $key ] ) : '';
$parts[] = "{$key}={$value}";
}
}
return hash( 'sha256', implode( '-', $parts ) );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
}
}
1 change: 1 addition & 0 deletions src/alley/wp/alleyvate/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ function load(): void {
'disable_sticky_posts' => new Features\Disable_Sticky_Posts(),
'disable_trackbacks' => new Features\Disable_Trackbacks(),
'disallow_file_edit' => new Features\Disallow_File_Edit(),
'login_nonce' => new Features\Login_Nonce(),
'redirect_guess_shortcircuit' => new Features\Redirect_Guess_Shortcircuit(),
'user_enumeration_restrictions' => new Features\User_Enumeration_Restrictions(),
];
Expand Down
Loading