Uninstallation Not Deleting Plugin Data

[PluginVulnerabilities]

WordPress plugins are supposed to clear out their data when uninstalled. As part of a security review of the plugin, we found that it doesn't currently do that.

It looks like there are 14 metadata keys that can be added to users' metadata that should be removed during uninstallation:

• _two_factor_nonce
• _two_factor_last_login_failure
• _two_factor_failed_login_attempts
• _two_factor_password_was_reset
• _two_factor_enabled_providers
• _two_factor_provider
• _two_factor_backup_codes
• _two_factor_email_token_timestamp
• _two_factor_email_token
• _two_factor_fido_u2f_register_request
• _two_factor_fido_u2f_login_request
• _two_factor_fido_u2f_registered_key
• _two_factor_totp_key
• _two_factor_totp_last_successful_login

There are several methods that can be used to delete those.

This could be classified as a security issue because of what is stored. You suggest reporting security issues to a bounty program, but that only accepts reports on a limited range of vulnerabilities. We raised that problem (and others) with WordPress' handling of reporting security issues through a bug bounty program with another plugin from WordPress in July, without a response or a change in your handling of the situation so far.

[jeffpaul]

My initial reaction here is to not remove these during plugin deactivation but perhaps during deletion/uninstallation we show an "are you sure" sort of message to alert the admin that this sort of data will be removed. @kasparsd does that approach seem ok to you or some alternate handling?

[PluginVulnerabilities]

You are correct they shouldn't be deleted during deactivation, as the relevant Plugin Handbook section mentions.

If an uninstallation method is set up, which would remove those, when clicking the Delete link for the plugin, it would show an alert that says:

Are you sure you want to delete Two Factor and its data?

Right now it says:

Are you sure you want to delete Two Factor?

[kasparsd]

Turns out we're not storing any options so we only need to take care of user meta. I've created a pull request for this #637.

The only consideration is that running the uninstall logic on a large multisite with many users could potentially time out. We could switch to a single query that deletes all user meta by keys to speed that up but I'll wait for a suggestion from @dd32 on this.

[dd32]

We could switch to a single query that deletes all user meta by keys to speed that up but I'll wait for a suggestion from @dd32 on this.

That's not really an option, as you need to delete from object caches and any other system that sync's data based on hooks.

Instead of querying for all users and looping through meta_keys, what you could do is a direct SELECT query for matching rows and then delete by Meta ID's delete_metadata_by_mid().

Personally, I don't see this as a security issue, so if not all data is removed, I don't see that as a critical problem.

[kasparsd]

@dd32 We're already using that method here:

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

Lines 354 to 390 in 8405126

	public static function delete_security_key( $user_id, $keyHandle = null ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
	global $wpdb;
	if ( ! is_numeric( $user_id ) ) {
	return false;
	}
	$user_id = absint( $user_id );
	if ( ! $user_id ) {
	return false;
	}
	$keyHandle = wp_unslash( $keyHandle ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
	$keyHandle = maybe_serialize( $keyHandle ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
	$query = $wpdb->prepare( "SELECT umeta_id FROM {$wpdb->usermeta} WHERE meta_key = %s AND user_id = %d", self::REGISTERED_KEY_USER_META_KEY, $user_id );
	if ( $keyHandle ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
	$key_handle_lookup = sprintf( ':"%s";s:', $keyHandle ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
	$query .= $wpdb->prepare(
	' AND meta_value LIKE %s',
	'%' . $wpdb->esc_like( $key_handle_lookup ) . '%'
	);
	}
	$meta_ids = $wpdb->get_col( $query ); // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
	if ( ! count( $meta_ids ) ) {
	return false;
	}
	foreach ( $meta_ids as $meta_id ) {
	delete_metadata_by_mid( 'user', $meta_id );
	}
	return true;
	}

but that still requires a delete_metadata_by_mid() call for every single meta entry which is probably equally intensive in terms of object cache purges and processing time.

Let's merge it with the current approach and iterate if needed.

[kasparsd]

@dd32 Turns out deleta_metadata() has the final argument $delete_all which allows us to delete all user metas by key similar to how delete_post_meta_by_key() does it for post meta (there isn't a matching function for users).

foreach ( $user_meta_keys as $meta_key ) {
	delete_metadata( 'user', null, $meta_key, null, true );
}

Any concerns with that approach?

[dd32]

Nope, that works for me too! I didn't realise it had the $delete_all parameter, and it properly clears the caches, so all is good by me.