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

Delete IP from oc_bruteforce_attempts via occ command #3058

Closed
guddl opened this issue Jan 13, 2017 · 20 comments · Fixed by #20005
Closed

Delete IP from oc_bruteforce_attempts via occ command #3058

guddl opened this issue Jan 13, 2017 · 20 comments · Fixed by #20005
Labels
enhancement feature: authentication good first issue Small tasks with clear documentation about how and in which place you need to fix things in. security
Milestone

Comments

@guddl
Copy link

guddl commented Jan 13, 2017

At the moment you have to delete an IP address which was blocked by a brute-force-detection via an SQL command on the database.

https://help.nextcloud.com/t/how-can-i-unblock-an-ip-blocked-through-brute-force-detection/5731

It would be nice to to this via an "occ" command.

@nickvergessen
Copy link
Member

I think it makes sense

@nickvergessen nickvergessen added enhancement feature: authentication security good first issue Small tasks with clear documentation about how and in which place you need to fix things in. labels Jan 13, 2017
@LukasReschke
Copy link
Member

Works for me. :)

@aweinert
Copy link

aweinert commented Mar 6, 2017

Would it make sense to allow deleting an IP from this table via the admin-panel?

@tflidd tflidd mentioned this issue Mar 25, 2017
@LittleNo
Copy link

since the brute force protection is not working properly for all users (for example NATted networks are a big problem) this feature definately should be there

@nickvergessen
Copy link
Member

nickvergessen commented Jun 28, 2017

Maybe this could be part of the app that was developed around bruteforce protection already:
nextcloud/bruteforcesettings#9

@MorrisJobke
Copy link
Member

MorrisJobke commented Sep 6, 2017

Another approach to relax the situation here: #6376 #3156

@MorrisJobke
Copy link
Member

I meant #3156

@Wikinaut
Copy link
Contributor

Any progress here ?
It would be nice to to this via an "occ" command.

@Wikinaut
Copy link
Contributor

Wikinaut commented Jan 2, 2020

ping @jospoortvliet as discussed publically after your 36c3 talk. I regard a final solution of this issue as easy and as important.

@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2020

Would you mind to share your use case? The throttling is only for a certain ip address. This is only a issue for nat or your reverse proxy configuration is invalid (nextcloud do not see the correct ip).

public function resetDelay($ip, $action, $metadata) {
$ipAddress = new IpAddress($ip);
if ($this->isIPWhitelisted((string)$ipAddress)) {
return;
}
$cutoffTime = (new \DateTime())
->sub($this->getCutoff(43200))
->getTimestamp();
$qb = $this->db->getQueryBuilder();
$qb->delete('bruteforce_attempts')
->where($qb->expr()->gt('occurred', $qb->createNamedParameter($cutoffTime)))
->andWhere($qb->expr()->eq('subnet', $qb->createNamedParameter($ipAddress->getSubnet())))
->andWhere($qb->expr()->eq('action', $qb->createNamedParameter($action)))
->andWhere($qb->expr()->eq('metadata', $qb->createNamedParameter(json_encode($metadata))));
$qb->execute();
}

If anyone want's to submit a patch. You can reuse some of the code from there. Happy coding ☕

@Wikinaut
Copy link
Contributor

Wikinaut commented Jan 2, 2020

@kesselb use case? Very stupid simple use case: Nextcloud on a rented server. User/s try to hack a password, get blocked by the mechanism. Same happens, if your clients (not using the dedicated token method but password) use the wrong password - because user had changed that on via the web interface--- so there are many use cases.

As an admin I wish to have a plugin or admin setting which allows to safely delete specific or all blocked IP from the table.

@Wikinaut

This comment has been minimized.

@Wikinaut

This comment has been minimized.

@Wikinaut

This comment has been minimized.

@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2020

Very stupid simple use case: Nextcloud on a rented server. User/s try to hack a password, get blocked by the mechanism.

So? Their IP address is blocked. You can still login.

@Wikinaut
Copy link
Contributor

Wikinaut commented Jan 3, 2020

No. my own IP address is blocked, because my own other devices, in the same network and coming with the same IP to the server - when connected via username/password, not with token - were using the old (wrong) password.

It took me a long time to figure out, what the problem was.

You do really not understand the problem, whereas @jospoortvliet does. Please ask him.

@kesselb
Copy link
Contributor

kesselb commented Jan 3, 2020

You do really not understand the problem, whereas @jospoortvliet does.

That's rude. Please use a more polite language the next time. Anyway I do understand the problem. Probably someone will pick it up. I don't see a strong urge to implement such a feature.

Please refrain from asking for progress. 👍 the issue and subscribe for updates is enough.

@joeried
Copy link
Contributor

joeried commented Mar 17, 2020

I would like to take a look at this issue.
What would be the correct namespace of such a occ command? 'security:bruteforceattemps:reset-for-ip'?

@kesselb You quoted code that can be reused. My first thought would be to extend the resetDelay function so that:

  • metadata and action are optional arguments
  • another optional argument exists, which switches whether the given IP or the whole subnet of the given IP is used/unblocked

Does that make sense?

@kesselb
Copy link
Contributor

kesselb commented Mar 17, 2020

Cool 👍

I think that's much additional complexity for this method.

As start you might write a command to delete a single IP from the list. We can always extend later. Probably not bad to also write a command that lists the current blocks (if there is no such command yet).

@joeried
Copy link
Contributor

joeried commented Mar 17, 2020

It would increase complexity, but on the other hand a new method would be relatively similar to the existing one.
The querry to delete the attemps for one IP would need one different where condition ('ip' instead of 'subnet') and omit two other where conditions
With my proposal I actually wanted to delete only one IP. The proposed flag for switching between subnet and ip would have been needed at this point to switch the where condition from 'subnet' (current usage) to 'ip' (usage for this case).

But I also understand your argument and that was also one of the reasons why I wanted to ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature: authentication good first issue Small tasks with clear documentation about how and in which place you need to fix things in. security
Projects
None yet
Development

Successfully merging a pull request may close this issue.