Bypass getLoginAttempt indefinitely by logging in from a different IP address

[ghost]

What version of OpenCart are you reporting this for?
3.x

Describe the bug
It's possible to indefinitely bypass the getLoginAttempt check.

To Reproduce

1. Go to the account/login or checkout/login
2. Attempt failed logins with email = test@test.zzz from a first IP address = 111.111.111.111, let's say total = 2 times, which is below the config_login_attempts = 5.
3. Then switch IP address = 222.222.222.222 and attempt again with the same email = test@test.zzz, always with a different IP address from the first IP address, for total = indefinite number of times.
4. $json['error']['warning'] is never triggered, so you can continue to brute force the login.

Expected behavior
In catalog/model/customer, addLoginAttempt() will add multiple rows for the same email if failed login attempts are from multiple IP addresses. Because of this:
SELECT ..... WHERE username = ...... AND ip =

But getLoginAttempts() only returns the first row of the query.

So with the example above with indefinite failed login attempts, total for the second query row will rise indefinitely, but the validation always gets total = 2 from the first query row.

On a successful login attempt, deleteLoginAttempts() will delete all rows with the same email.

How to Fix
Option 1 - log only a single row

1. Delete all rows from customer_login to make sure there are no duplicate entries for the same email address.
2. In addLoginAttempt(), Remove "AND ip =" from the query, so that logging in from a different IP address will overwrite the single instance row of the same email.
3. Then only the IP of the last failed attempt will be recorded, and total will be cumulative, no matter how many IP addresses the failed attempts are made from.

Option 2 - log multiple rows

1. In getLoginAttempts(), change the return to $query->rows, to return all multiple rows of the same email.
2. In catalog/controller/account/login and checkout/login, change to a foreach loop to check all rows.

//adds totals of multiple rows
$attempt_total = 0;
foreach ($login_info as $avalue) {
	$attempt_total += $avalue['total'];
}
unset($avalue);


//optionally send an alert email to admin when total attempts equals config_login_attempt, so that it only sends one time. Needs to occur before the next addLoginAttempt, otherwise this number may be skipped.
if ($attempt_total == $this->config->get('config_login_attempts')) {
	$subject = sprintf('%s - Too many login attempts for a customer', $this->config->get('config_name'));
			
	$message  = sprintf('There were too many login attempts for customer %s', $this->request->post['email']) . "\n\n";
	$message .= 'Please try again after 1 hour.' . "\n\n";
	$message .= sprintf('From this IP address: %s', $this->request->server['REMOTE_ADDR']) . "\n\n";

	$mail = new Mail();
	$mail->protocol = $this->config->get('config_mail_protocol');
	$mail->parameter = $this->config->get('config_mail_parameter');
	$mail->hostname = $this->config->get('config_smtp_host');
	$mail->username = $this->config->get('config_smtp_username');
	$mail->password = html_entity_decode($this->config->get('config_smtp_password'), ENT_QUOTES, 'UTF-8');
	$mail->port = $this->config->get('config_smtp_port');
	$mail->timeout = $this->config->get('config_smtp_timeout');				
	$mail->setTo($this->config->get('config_email'));
	$mail->setFrom($this->config->get('config_email'));
	$mail->setSender($this->config->get('config_name'));
	$mail->setSubject(html_entity_decode($subject, ENT_QUOTES, 'UTF-8'));
	$mail->setText(html_entity_decode($message, ENT_QUOTES, 'UTF-8'));
	$mail->send();
}


//if totals is greater than limit, check if any row was last updated in past hour
if ($attempt_total >= $this->config->get('config_login_attempts')) {
	foreach ($login_info as $bvalue) {
		if (strtotime('-1 hour') < strtotime($bvalue['date_modified'])) {
			$json['error']['warning'] = $this->language->get('error_attempts');
			
			//addLoginAttempt only necessary for 3.0.3.7 and older if you want to continue recording failed logins over config_login_attempts because the other addLoginAttempt only occurs on password check which doesn't trigger if json. master branch checks password anyway, so addLoginAttempt is always triggered.
			$this->model_account_customer->addLoginAttempt($this->request->post['email']);
			
			break;
		}
	}
}
unset($bvalue);

[danielkerr]

it goes off the email not ip.

[danielkerr]

your just wasting peoples time. leave it to the professionals

[ADDCreative]

@danielkerr please look at this again.

If the IP is different another row is being added.

opencart/upload/catalog/model/account/customer.php

Lines 127 to 130 in 0f81df9

	$query = $this->db->query("SELECT * FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower((string)$email)) . "' AND `ip` = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "'");
	if (!$query->num_rows) {
	$this->db->query("INSERT INTO `" . DB_PREFIX . "customer_login` SET `email` = '" . $this->db->escape(utf8_strtolower((string)$email)) . "', `ip` = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "', `total` = '1', `date_added` = '" . $this->db->escape(date('Y-m-d H:i:s')) . "', `date_modified` = '" . $this->db->escape(date('Y-m-d H:i:s')) . "'");

Yet only the first row is returned.

opencart/upload/catalog/model/account/customer.php

Line 139 in 0f81df9

return $query->row;

[ghost]

it goes off the email not ip.

Both email and IP has to match , otherwise an additional row is added.

It's clearly in the code "AND ip = "

But only the first row is returned. So if your current IP is different from the first row, and the first row "total" is below the trigger limit, the error is never triggered.

My solution above returns all rows, and then adds the totals of all rows to check if the totals is above the limit, and if so then checks every row if any were updated within the last 1 hour.

This way, it records all IPs that fail login for the same email.

The other solution was to take out "AND ip =", but then you will only record the IP of the last failed attempt. And if multiple rows of the same email already exists and you don't first delete all of the rows, then those emails would still never trigger if the first row total is less than the fail limit. actually subsequent fails might add 1 to the total for all rows of the same email, and the first row gets written to even if only the first row is returned, so this might fix existing rows too.

[danielkerr]

just forget it. its not worth it.

[ghost]

The fix above for option 2 foreach works. I've tested it several times. It's just swapping a few lines.

You can verify the bypass on the Opencart demo page. There are 2 methods, either before or after the time lock is triggered.

The demo sets config_login_attempts = 5

https://www.opencart.com/?route=account/login

Bypass A - before lock is triggered

1. Fail the login 1 time with the first IP address, under the limit of 5, with a new unused email.
   example email: ok-aaa

2. Switch to a different IP address, and fail the login 10 more times with the same email, never triggers the time lock.
   test the same email: ok-aaa

Bypass B - after lock is triggered

1. Fail the login 6 times with the first IP address, over the limit of 5, with a new unused email, triggers 1 hour lock.
   example email: ok-bbb

2. Wait 61 minutes, switch to a different IP address and fail the login 10 more times with the same email, never triggers the time lock.
   test the same email: ok-bbb

[stalker780]

What about adding optional recaptcha to login page?

[ADDCreative]

A quick fix would to maybe sum the total in the getLoginAttempts query.

opencart/upload/catalog/model/account/customer.php

Line 137 in ad38c10

$query = $this->db->query("SELECT * FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower($email)) . "'");

$query = $this->db->query("SELECT SUM(`total`) AS `total`, MAX(`date_modified`) AS `date_modified`  FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower($email)) . "'");

[ghost]

If it works, it would definitely be simpler.
You could also create a similar function to return matches by IP address if you want to also time lock based on number of failed login attempts by the same IP, in addition to the failed attempts by email check. But this would only be useful if multiple customer emails are known and being attempted.

$query = $this->db->query("SELECT SUM(`total`) AS `total`, MAX(`date_modified`) AS `date_modified`  FROM `" . DB_PREFIX . "customer_login` WHERE `ip` = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "'");

[ghost]

@ADDCreative if your quick fix works (I haven't check but it looks like it may), then based on the fix, I wrote additional code to also check if the remote IP exceeds fail limit from any emails.

And a function to send a notification email if either a email or IP exceeds the fail limit, only once per hour, if the customer email exists. This requires adding a new date column last_notify to the table customer_login.

I haven't yet checked if these work.

catalog/model/account/customer.php

public function getLoginAttempts($email) {
	$query = $this->db->query("SELECT SUM(`total`) AS `total`, MAX(`date_modified`) AS `date_modified`, MAX(`last_notify`) AS `last_notify` FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower($email)) . "'");

	return $query->row;
}

public function getLoginAttemptsByRemoteIP() {
	$query = $this->db->query("SELECT SUM(`total`) AS `total`, MAX(`date_modified`) AS `date_modified`, MAX(`last_notify`) AS `last_notify` FROM `" . DB_PREFIX . "customer_login` WHERE `ip` = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "'");

	return $query->row;
}
	
public function notifyLoginAttempt($email) {
	$subject = sprintf('%s - Too many login attempts for a customer', $this->config->get('config_name'));
			
	$message  = sprintf('There were too many login attempts for customer %s', $email) . "\n\n";
	$message .= 'Please try again after 1 hour.' . "\n\n";
	$message .= sprintf('From this IP address: %s', $this->request->server['REMOTE_ADDR']) . "\n\n";

	$mail = new Mail();
	$mail->protocol = $this->config->get('config_mail_protocol');
	$mail->parameter = $this->config->get('config_mail_parameter');
	$mail->hostname = $this->config->get('config_smtp_host');
	$mail->username = $this->config->get('config_smtp_username');
	$mail->password = html_entity_decode($this->config->get('config_smtp_password'), ENT_QUOTES, 'UTF-8');
	$mail->port = $this->config->get('config_smtp_port');
	$mail->timeout = $this->config->get('config_smtp_timeout');				
	$mail->setTo($this->config->get('config_email'));
	$mail->setFrom($this->config->get('config_email'));
	$mail->setSender($this->config->get('config_name'));
	$mail->setSubject(html_entity_decode($subject, ENT_QUOTES, 'UTF-8'));
	$mail->setText(html_entity_decode($message, ENT_QUOTES, 'UTF-8'));
	$mail->send();
		
	$this->db->query("UPDATE " . DB_PREFIX . "customer_login SET last_notify = '" . $this->db->escape(date('Y-m-d H:i:s')) . "' WHERE LOWER(email) = '" . $this->db->escape(utf8_strtolower($email)) . "' OR ip = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "'");
}

For catalog/controller/checkout/login.php

// Check how many login attempts have been made.
$login_info = $this->model_account_customer->getLoginAttempts($this->request->post['email']);
$login_info_remote_ip = $this->model_account_customer->getLoginAttemptsByRemoteIP();
		
if ((!empty($login_info) && $login_info['total'] >= $this->config->get('config_login_attempts') && strtotime('-1 hour') < strtotime($login_info['date_modified'])) || (!empty($login_info_remote_ip) && $login_info_remote_ip['total'] >= $this->config->get('config_login_attempts') && strtotime('-1 hour') < strtotime($login_info_remote_ip['date_modified']))) {

	$json['error']['warning'] = $this->language->get('error_attempts');
			
	//optional send notification email to administrator
	if (!(!empty($login_info['last_notify']) || !empty($login_info_remote_ip['last_notify'])) || !(!(!empty($login_info['last_notify']) && strtotime('-1 hour') > strtotime($login_info['last_notify'])) || !(!empty($login_info_remote_ip['last_notify']) && strtotime('-1 hour') > strtotime($login_info_remote_ip['last_notify'])))) {
		
		if (!empty($this->model_account_customer->getTotalCustomersByEmail($this->request->post['email']))) {
			$this->model_account_customer->notifyLoginAttempt($this->request->post['email']);
		}
	}
	
	//duplicate addLoginAttempt is only for 3.0.3.7 and older, not for master branch
	$this->model_account_customer->addLoginAttempt($this->request->post['email']);
}

For catalog/controller/account/login.php from 3.0.3.7 and older, instead of $json['error']['warning'], replace with $this->error['warning']

[ghost]

@ADDCreative I tested your 1 line solution and it fixed the bypass for me.

I added customer id to the table so that I can quickly sort which ones have accounts that exist, and also I can add an extra line to deleteCustomer() in admin/model/sale/customer to delete the rows from the customer_login table.

If the original intent was to also limit by IP, my example worked too. However, successful login should only delete rows from the table by matching email, do not delete by IP. Otherwise, creating a spam account with the same IP and logging into it would clear the IP from the table and remove the limit.

But by limiting with IP, when an IP fails with multiple emails, for example with misspelled email, and succeeds in logging into only one of the emails, the other rows still remain in the table and would reduce the limit forever for that IP. I couldn't think of a solution to this disadvantage, other than to not limit by IP, or by manually clearing the table when responding to the notification.

Also, my example with an admin notification works well, only 1 notice per hour, per email or IP, if the account exists.

It was simple to copy over to admin user login. And doable for forgotten password reset with a new table.

[danielkerr]

fine we can add delete with ip

[TheCartpenter]

If the email class needs to be used, an event would have to be created and also validating the notifications with $this->config->get prior to send the emails.

[prabhat-webkul]

A quick fix would to maybe sum the total in the getLoginAttempts query.

opencart/upload/catalog/model/account/customer.php

Line 137 in ad38c10

$query = $this->db->query("SELECT * FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower($email)) . "'");

$query = $this->db->query("SELECT SUM(`total`) AS `total`, MAX(`date_modified`) AS `date_modified`  FROM `" . DB_PREFIX . "customer_login` WHERE LOWER(`email`) = '" . $this->db->escape(utf8_strtolower($email)) . "'");

This fix will work but if we are checking all entries then let's make it simple by having only one entry in DB
added a PR #10172

[ADDCreative]

@prabhatkumaroc082 If you are going to fix it that way, best to remove the ip column totally. Otherwise it will be set to the first IP used and will be confusing to anyone investigating an issue.

opencart/upload/admin/model/user/user.php

Line 113 in 8c06097

$this->db->query("INSERT INTO " . DB_PREFIX . "customer_login SET email = '" . $this->db->escape(utf8_strtolower((string)$username)) . "', ip = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "', total = 1, date_added = '" . $this->db->escape(date('Y-m-d H:i:s')) . "', date_modified = '" . $this->db->escape(date('Y-m-d H:i:s')) . "'");

opencart/upload/install/opencart.sql

Line 1154 in 8c06097

`ip` varchar(40) NOT NULL,

[prabhat-webkul]

@ADDCreative
You are right, but let's leave it as it is for future amendments or website customizations. we are currently testing PR #10172 and will be added for release 3.0.3.8

[WebkulOpencart]

PR merged #10172