Add logging function when failed to authenticate #462

Lucisu · 2022-10-04T03:32:10Z

I've added the method is_valid_authcode to the class Two_Factor_Provider so providers can use it when failing to validate the authentication.

By default, it uses the error_log function, but there are actions and filters that can be used to extend it.

Related issue: #459

Lucisu · 2022-10-04T19:27:55Z

It's done.

For me, it would make more sense to call a method inside the Two_Factor_Provider class to validate the authentication, which would call the child provider method validate_authentication, getting its result and logging the failure in the parent method if needed, instead of adding to every new provider the code:

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

Please, let me know what you think about this.

kasparsd

How about we move the logging to the main conditional here:

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

Lines 857 to 867 in a112a0a

    
           if ( true !== $provider->validate_authentication( $user ) ) { 
        
           	do_action( 'wp_login_failed', $user->user_login ); 
        
           	$login_nonce = self::create_login_nonce( $user->ID ); 
        
           	if ( ! $login_nonce ) { 
        
           		wp_die( esc_html__( 'Failed to create a login nonce.', 'two-factor' ) ); 
        
           	} 
        
           	self::login_html( $user, $login_nonce['key'], $_REQUEST['redirect_to'], esc_html__( 'ERROR: Invalid verification code.', 'two-factor' ), $provider ); 
        
           	exit; 
        
           }

so that (1) we don't need to modify each provider to add this and (2) providers are not responsible for logging.

We could also update the validate_authentication() methods to return WP_Error objects which could provide their own error messages which can then be used by the logger as necessary.

kasparsd · 2022-10-12T09:57:16Z

providers/class-two-factor-provider.php

+		 * @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 );


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.

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

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

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.

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?

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.

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:

An email provider can send the email address used to log in instead of the user ID.

Define their own "failed amount" before adding the log.

Adding their own log methods.

georgestephanis · 2022-10-14T15:04:22Z

That sounds fair.

…

On Fri, Oct 14, 2022 at 10:49 AM Jeffrey Paul ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In providers/class-two-factor-provider.php <#462 (comment)>: > + 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 with ID %1$d failed to login using the code "%2$s"', 'two-factor' ), $user->ID, esc_html( $code ) ); + + /** + * 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 ); 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? — Reply to this email directly, view it on GitHub <#462 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHFXX7FOCMFLQZ6KZS2DIDWDFXHTANCNFSM6AAAAAAQ4FHI74> . You are receiving this because you were mentioned.Message ID: ***@***.***>

iandunn · 2022-10-14T21:32:08Z

Once #427 lands, logging should be added there too.

providers/class-two-factor-provider.php

ravinderk

@Lucisu, I left a few suggestions. Feel free to ask questions.

ravinderk · 2023-05-02T09:57:04Z

providers/class-two-factor-fido-u2f.php

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


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

ravinderk · 2023-05-02T09:57:35Z

providers/class-two-factor-provider.php

+		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 ) );


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

ravinderk · 2023-05-02T10:00:43Z

providers/class-two-factor-provider.php

+	 *
+	 * @return void
+	 */
+	public function log_failure( $user, $code = false ) {


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

ravinderk · 2023-05-02T10:12:23Z

providers/class-two-factor-backup-codes.php

@@ -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 ) );


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?

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

Added logging function

3b69449

jeffpaul linked an issue Oct 4, 2022 that may be closed by this pull request

Log or alert on failed 2FA codes #459

Open

jeffpaul assigned Lucisu Oct 4, 2022

jeffpaul added the Needs Code Review label Oct 4, 2022

jeffpaul added this to the 0.9.0 milestone Oct 4, 2022

Lucisu added 3 commits October 4, 2022 16:06

Fix two_factor_log_failure filter description

07dc6e1

Add Two_Factor_Provider class to hooks

969e4d2

Add failures logging to remaining providers

96b7cc0

kasparsd reviewed Oct 12, 2022

View reviewed changes

iandunn reviewed Oct 14, 2022

View reviewed changes

providers/class-two-factor-provider.php Outdated Show resolved Hide resolved

iandunn mentioned this pull request Oct 14, 2022

Notify user/admin when sensitive events occur #476

Open

5 tasks

Add user_login to failed authentication log message

bf6bd6a

iandunn mentioned this pull request Nov 28, 2022

Log 2FA activities WordPress/wporg-two-factor#20

Open

iandunn mentioned this pull request Dec 8, 2022

Prevent other plugins from accidentally circumventing two factor authentication #385

Closed

iandunn mentioned this pull request Feb 17, 2023

Broadcast events for logging, email notifications, etc #526

Draft

ravinderk suggested changes May 2, 2023

View reviewed changes

iandunn mentioned this pull request Jan 30, 2024

Timestamp for QR and Backup Code keygen #593

Closed

jeffpaul modified the milestones: 0.9.0, 0.10.0 May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging function when failed to authenticate #462

Add logging function when failed to authenticate #462

Lucisu commented Oct 4, 2022

Lucisu commented Oct 4, 2022

kasparsd left a comment

kasparsd Oct 12, 2022

kasparsd Oct 12, 2022

kasparsd Oct 12, 2022

georgestephanis Oct 12, 2022

jeffpaul Oct 14, 2022

iandunn Oct 14, 2022

Lucisu Oct 15, 2022 •

edited

Loading

georgestephanis commented Oct 14, 2022 via email

iandunn commented Oct 14, 2022

ravinderk left a comment

ravinderk May 2, 2023

ravinderk May 2, 2023

ravinderk May 2, 2023

ravinderk May 2, 2023

ravinderk May 2, 2023

	if ( true !== $provider->validate_authentication( $user ) ) {
	do_action( 'wp_login_failed', $user->user_login );

	$login_nonce = self::create_login_nonce( $user->ID );
	if ( ! $login_nonce ) {
	wp_die( esc_html__( 'Failed to create a login nonce.', 'two-factor' ) );
	}

	self::login_html( $user, $login_nonce['key'], $_REQUEST['redirect_to'], esc_html__( 'ERROR: Invalid verification code.', 'two-factor' ), $provider );
	exit;
	}

Add logging function when failed to authenticate #462

Are you sure you want to change the base?

Add logging function when failed to authenticate #462

Conversation

Lucisu commented Oct 4, 2022

Lucisu commented Oct 4, 2022

kasparsd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucisu Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

georgestephanis commented Oct 14, 2022 via email

iandunn commented Oct 14, 2022

ravinderk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucisu Oct 15, 2022 •

edited

Loading