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 15 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
141 changes: 141 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,141 @@
<?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.
*
* Heavily inspired by `wp-login-nonce` by `elyobo`
*
* @link https://github.com/elyobo/wp-login-nonce
*/
final class Login_Nonce implements Feature {

/**
* The name to use for the nonce.
*
* @var string
*/
private const NONCE_SALT = 'wp_alleyvate_login_nonce';

/**
* The action to use for the nonce.
*
* @var string
*/
public const NONCE_ACTION = 'alleyvate_login_action';

/**
* The nonce lifetime. Stored in seconds.
*
* @var int
*/
public const NONCE_TIMEOUT = 1800;

/**
* Boot the feature.
*/
public function boot(): void {
add_action( 'login_init', [ self::class, 'action__add_nonce_life_filter' ] );
add_action( 'login_head', [ self::class, 'action__add_meta_refresh' ] );
add_action( 'login_form', [ self::class, 'action__add_nonce_to_form' ] );
add_action( 'after_setup_theme', [ self::class, 'action__pre_validate_login_nonce' ], 9999 );
}

/**
* Add a meta refresh to the login page, so it will refresh after the nonce timeout.
*/
public static function action__add_meta_refresh(): void {
echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( self::NONCE_TIMEOUT ) );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Add the nonce field to the form.
*/
public static function action__add_nonce_to_form(): void {
wp_nonce_field( self::NONCE_ACTION, self::generate_random_nonce_name() );
}

/**
* Initializes the nonce fields. Is only run on `login_init` to restrict nonce data to login page.
*/
public static function action__add_nonce_life_filter(): void {
add_filter( 'nonce_life', fn() => self::NONCE_TIMEOUT );
Copy link
Member

Choose a reason for hiding this comment

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

Could this check the nonce name & action and only set the timeout for this particular nonce? This way it could avoid changing the nonce timeout of any other nonces that may be added to this page. (May be an unlikely edge case, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but I'm not sure if its necessary, since this is only added on the login_init action, which would restrict it to the login page.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I have a hard time imagining the issue here, although being as precise as possible may help prevent some future compatibility issue that we can't predict. I guess possibilities could be other plugins adding a nonce or WP core adding a nonce.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving this as-is. If core ends up adding a nonce to the login screen, this feature would become irrelevant, and we could remove it :)

kevinfodness marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Validates the login nonce as early as possible to avoid login attempts.
*/
public static function action__pre_validate_login_nonce(): void {
/*
* If this request is not specifically a login attempt on the wp-login.php page,
* then skip it.
*/
if (
'wp-login.php' !== $GLOBALS['pagenow'] ||
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
empty( $_POST ) ||
(
isset( $_POST['wp-submit'] ) &&
'Log In' !== $_POST['wp-submit']
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
)
) {
return;
}

$nonce_life_filter = fn() => self::NONCE_TIMEOUT;

/*
* Nonce life is used to generate the nonce value. If this differs from the form,
* the nonce will not validate.
*/
add_filter( 'nonce_life', $nonce_life_filter );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved

$nonce = false;
if ( ! empty( $_POST[ self::generate_random_nonce_name() ] ) ) {
$nonce = sanitize_key( $_POST[ self::generate_random_nonce_name() ] );
}

if (
! $nonce ||
! wp_verify_nonce( $nonce, self::NONCE_ACTION )
) {
// This is a login with an invalid nonce. Throw an error.
http_response_code( 403 );
wp_die();
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
return;
}

/*
* Clean up after ourselves.
*/
remove_filter( 'nonce_life', $nonce_life_filter );
}

/**
* Randomize the nonce name using the data from the $_SERVER super global, and a provided salt.
*
* @return string
*/
public static function generate_random_nonce_name(): string {
$parts = [ self::NONCE_SALT ];
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
102 changes: 102 additions & 0 deletions tests/alley/wp/alleyvate/features/test-login-nonce.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<?php
/**
* Class file for Test_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
*
* @phpcs:disable WordPress.WP.GlobalVariablesOverride.Prohibited, Generic.CodeAnalysis.EmptyStatement.DetectedCatch
*/

namespace Alley\WP\Alleyvate\Features;

use Mantle\Testing\Concerns\Refresh_Database;
use Mantle\Testing\Exceptions\WP_Die_Exception;
use Mantle\Testkit\Test_Case;

/**
* Tests for the login nonce.
*/
final class Test_Login_Nonce extends Test_Case {
use Refresh_Database;

/**
* Feature instance.
*
* @var Login_Nonce
*/
private Login_Nonce $feature;

/**
* Set up.
*/
protected function setUp(): void {
parent::setUp();

$this->feature = new Login_Nonce();

/*
* Prime the response code to 200 before running nonce validations.
*/
http_response_code( 200 );
}

/**
* Test that login nonces are required to login successfully.
*/
public function test_logins_require_nonce() {
global $pagenow;

$this->feature->boot();

$_POST = [
'wp-submit' => 'Log In',
];

$pagenow = 'wp-login.php';

try {
Login_Nonce::action__pre_validate_login_nonce();
} catch ( WP_Die_Exception $e ) {
// Do nothing.
}

$this->assertSame( 403, http_response_code() );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Test that using nonces allow successful logins.
*/
public function test_logins_work_with_nonce() {
global $pagenow;
$this->feature->boot();

$nonce_life_filter = fn() => Login_Nonce::NONCE_TIMEOUT;

/*
* Nonce life is used to generate the nonce value. If this differs from the form,
* the nonce will not validate.
*/
add_filter( 'nonce_life', $nonce_life_filter );
$_POST = [
'wp-submit' => 'Log In',
Login_Nonce::generate_random_nonce_name() => wp_create_nonce( Login_Nonce::NONCE_ACTION ),
];

remove_filter( 'nonce_life', $nonce_life_filter );

$pagenow = 'wp-login.php';

try {
Login_Nonce::action__pre_validate_login_nonce();
} catch ( WP_Die_Exception $e ) {
// Do nothing.
}

$this->assertSame( 200, http_response_code() );
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
anubisthejackle marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading