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.

Sign up for GitHub

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

Add logging function when failed to authenticate #462

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+66 −6
Open

Add logging function when failed to authenticate #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b69449
Added logging function
Lucisu Oct 4, 2022
07dc6e1
Fix two_factor_log_failure filter description
Lucisu Oct 4, 2022
969e4d2
Add Two_Factor_Provider class to hooks
Lucisu Oct 4, 2022
96b7cc0
Add failures logging to remaining providers
Lucisu Oct 4, 2022
bf6bd6a
Add user_login to failed authentication log message
Lucisu Oct 15, 2022
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
8 changes: 7 additions & 1 deletion providers/class-two-factor-backup-codes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,13 @@ public function authentication_page( $user ) {
*/
public function validate_authentication( $user ) {
$backup_code = isset( $_POST['two-factor-backup-code'] ) ? sanitize_text_field( wp_unslash( $_POST['two-factor-backup-code'] ) ) : false;
return $this->validate_code( $user, filter_var( $backup_code, FILTER_SANITIZE_STRING ) );
$success = $this->validate_code( $user, filter_var( $backup_code, FILTER_SANITIZE_STRING ) );
Copy link

Choose a reason for hiding this comment

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

I think we place logging logic in the core class where we validate the authentication provider:

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

Line 1174 in c725c9b

if ( true !== $provider->validate_authentication( $user ) ) {

Are we getting benefits from adding logging logic here?

Copy link

Choose a reason for hiding this comment

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

We should not worry about the format of logging data or internal; the log function should dump logging data in an error log file.


if ( ! $success ) {
$this->log_failure( $user, $backup_code );
}

return $success;
}

/**
Expand Down
8 changes: 7 additions & 1 deletion providers/class-two-factor-email.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,13 @@ public function validate_authentication( $user ) {
// Ensure there are no spaces or line breaks around the code.
$code = trim( sanitize_text_field( $_REQUEST['two-factor-email-code'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, handled by the core method already.

return $this->validate_token( $user->ID, $code );
$success = $this->validate_token( $user->ID, $code );

if ( ! $success ) {
$this->log_failure( $user, $code );
}

return $success;
}

/**
Expand Down
10 changes: 8 additions & 2 deletions providers/class-two-factor-fido-u2f.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,16 @@ public function validate_authentication( $user ) {

self::update_security_key( $user->ID, $reg );

return true;
$success = true;
} catch ( Exception $e ) {
return false;
$success = false;
}

if ( ! $success ) {
$this->log_failure( $user, $response );
Copy link

Choose a reason for hiding this comment

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

This can cause an error because $response is an object, but the function accepts string as the second argument.

}

return $success;
}

/**
Expand Down
37 changes: 37 additions & 0 deletions providers/class-two-factor-provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,43 @@ public function pre_process_authentication( $user ) {
*/
abstract public function validate_authentication( $user );

/**
* Logs the failed authentication.
*
* @param WP_User $user WP_User object of the user trying to login.
* @param string|false $code The code used to authenticate, if available.
*
* @since 0.9.0
*
* @return void
*/
public function log_failure( $user, $code = false ) {
Copy link

Choose a reason for hiding this comment

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

We should support only string as a second param. Is there any benefit we are getting by supporting boolean as an alternative value?

/**
* This action is triggered when a Two Factor validation fails.
*
* @param WP_User $user WP_User object of the user trying to login.
* @param string|false $code The code used to authenticate, if available.
* @param Two_Factor_Provider $this The Provider class used during the authentication.
*/
do_action( 'two_factor_user_login_failed', $user, $code, $this );

/* translators: %1$d: the user's ID %2$s: the code used to authenticate */
$log_message = sprintf( esc_html__( 'The user %1$s (ID: %2$d) failed to login using the code "%3$s"', 'two-factor' ), esc_html( $user->user_login ), $user->ID, esc_html( $code ) );
Copy link

Choose a reason for hiding this comment

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

We do not need to reveal the user id if we print the user name in the message.


/**
* This filter is triggered to checke whether it's needed to log the authentication failure.
*
* @param boolean $should_log Whether or not the authentication failure should be logged.
* @param WP_User $user WP_User object of the user trying to login.
* @param string|false $code The code used to authenticate, if available.
* @param string $log_message The generated log message.
* @param Two_Factor_Provider $this The Provider class used during the authentication.
*/
if ( apply_filters( 'two_factor_log_failure', true, $user, $code, $log_message, $this ) ) {
error_log( $log_message );
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we sent an email to admin_email instead of logging it the error log by default?

The two_factor_user_login_failed could be used to send it to other destinations such as error log or custom logging utility as needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There appears to be a few instance where WP core is calling error_log() so we should be OK to use it too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jeffpaul, @georgestephanis What are your thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My only uncertainty is if it's a good idea to include the bad code in this. I don't have any strong opposition, but I'm slightly concerned that if it's one-digit off, or enough times combined with the timestamp being one tick off, something may be able to be reversed.

But I'm fine with email or not (someone trying to brute force the two factor code may be ... uhh ... a lot of emails though) -- but I definitely think error_log should be included.

Also, appreciate the filter so a third-party catcher could also listen for failures for alternate logs.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe support the admin_email but have it disabled and filterable to enable? I think I'd want it enabled on sites I'm using, but I could see where that could get VERY chatty for someone with a large amount of users?

Copy link
Member

Choose a reason for hiding this comment

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

It might also depend on the provider; for TOTP it's probably not uncommon to make a typo, and getting an email for that seems like overkill. Emailing after 3 failed attempts could be more reasonable (as long as there's a cap on how often it's sent), but IMO #477 seems like a better way to address brute force attacks.

If a backup code fails, though, I can see it being more useful to email, because it's very out of the ordinary. Related #476.

WebAuthn keys (#427) could also be worth emailing about, since they should be less prone to accidental failure, especially with user verification/PINs disabled.

I don't think gaining access to several TOTPs would let you reverse the secret, but I'm not sure how useful including it is either.

Copy link
Author

@Lucisu Lucisu Oct 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm afraid that moving the logging function out of the providers will not allow many customizations from them. I can think of some examples where it's useful to allow providers to set their own log data:

  1. An email provider can send the email address used to log in instead of the user ID.
  2. Define their own "failed amount" before adding the log.
  3. Adding their own log methods.

}
}

/**
* Whether this Two Factor provider is configured and available for the user specified.
*
Expand Down
9 changes: 7 additions & 2 deletions providers/class-two-factor-totp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,14 +288,19 @@ public function admin_notices( $user_id ) {
* @return bool Whether the user gave a valid code
*/
public function validate_authentication( $user ) {
$success = false;
if ( ! empty( $_REQUEST['authcode'] ) ) {
return $this->is_valid_authcode(
$success = $this->is_valid_authcode(
$this->get_user_totp_key( $user->ID ),
sanitize_text_field( $_REQUEST['authcode'] )
);
}

return false;
if ( ! $success ) {
$this->log_failure( $user, ! empty( $_REQUEST['authcode'] ) ? sanitize_text_field( $_REQUEST['authcode'] ) : false );
}

return $success;
}

/**
Expand Down