getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Bruteforce protection is not filtering by IP #2518

Closed robinscholz closed 3 years ago

robinscholz commented 4 years ago

Describe the bug
It seems that once a user is disabled by storage/accounts/.login there is no way to access Kirby with that user, even from another IP address.

If using a user to access the REST API from i.e. a different frontend, this makes it rather easy to DoS attack the application.

To Reproduce
Steps to reproduce the behavior:

  1. Try logging in from a phone (no wifi) with wrong credentials 10 times in a row
  2. Try logging in from a computer with wrong credentials 10 times in a row
  3. See error

Expected behavior
The login should only be disabled for the specific IP adress that gets saved.

Kirby Version
3.2.3

Desktop (please complete the following information):

robinscholz commented 4 years ago

https://github.com/getkirby/kirby/blob/6bd14bc8099d6c9dbe67ef0582b8c0a2d4c0888c/src/Cms/Auth.php#L182

robinscholz commented 4 years ago

It seems that the bruteforce protection is either looking for an IP match or an e-mail match, which makes it really easy to disable a specific user login.

afbora commented 4 years ago

I don't think IP based filtering is the right solution đŸ€”

https://owasp.org/www-community/controls/Blocking_Brute_Force_Attacks

robinscholz commented 4 years ago

@afbora It‘s already implemented, just not in connection with the email used for the login attempt. Might be worth it to completely open up this discussion though.

lukasbestle commented 4 years ago

The reason why we also block by account is to ensure that a single account can't as easily be brute-forced by a botnet.

As this is a security feature, I think that preventing brute force attacks is more important than avoiding login issues for legitimate users. The DoS issue can only happen if you know the account's email address, so it would need to be a targeted attack. In which case you are probably screwed anyway and it's likely good to get pointed to that attack by the Panel not working properly anymore.

robinscholz commented 4 years ago

In my opinion the Kirby team should invest some time in a more robust API request verification (instead of BasicAuth) , which would prevent simple DOS attacks that abuse the currently implemented brute-force protection.

Then again, the current brute-force protection is also not very sophisticated compared to the mesasurements listed in the link @afbora posted.

lukasbestle commented 4 years ago

more robust API request verification (instead of BasicAuth)

I'm not sure how an authentication method can be any more robust than Basic Auth. In the end you always need to map user credentials (email and password) to some kind of session token. Basic Auth is very very good at this.

Making this any more complex won't prevent other people from locking out legitimate users – matching always needs to be done by the email address the user or attacker entered. Also, as I said, the "attacker can lock out legitimate users" issue is a non-issue in my opinion.

Then again, the current brute-force protection is also not very sophisticated compared to the mesasurements listed in the link @afbora posted.

Kirby is not a centralized SaaS service where thousands of users are registered with user data they also use elsewhere and where a central support team would need to unlock all of those accounts. The standards for protection levels depend on the requirements, which are different in Kirby from the ones in the linked document. Because if you do too much, you will create a complexity that can't be managed anymore.

If you compare Kirby to other CMSs, you will see that not every CMS has brute-force protection at all. So I think we are already on a good path.

Please also note that the IP/account blacklist is not the only protection we have in place. We also delay requests with invalid credentials to make brute-force attacks less effective in general.

robinscholz commented 4 years ago

This non issue as you call it, basically renders Kirby useless as a headless CMS. The whole API can easily be disabled by locking out a user. If the API authentification would be seperated from the access to the panel and the brute-force protection of users with write access, this problem could be avoided.

For now I have to disable the brute-force prevention for all users, even those who are allowed to write data via the panel. This is a shitty trade off.

It also means that the KQL plugin by @bastianallgeier can‘t really be used in a lot of cases.

Not sure why you deem this unsolvable. @bastianallgeier asked me to open this issue after we discussed the initial problem.

lukasbestle commented 4 years ago

If the API authentification would be seperated from the access to the panel and the brute-force protection of users with write access, this problem could be avoided.

Everything you can do in the Panel can be done via the API – the Panel uses the API itself internally after all. So I don't see how a separation of API and Panel security logic would help here. The weakest point (the API in this case) would define the security level of the whole installation.

For now I have to disable the brute-force prevention for all users, even those who are allowed to write data via the panel.

Are you actually being attacked by such a lockout attack at the moment? Or what is the reason you need to disable the brute-force protection? To be honest, I have never seen such an attack or heard of it so far. That's why I wrote that I see it as a non-issue.

It also means that the KQL plugin by @bastianallgeier can‘t really be used in a lot of cases.

Why that specifically?

Not sure why you deem this unsolvable. @bastianallgeier asked me to open this issue after we discussed the initial problem.

I'm reopening this so that @bastianallgeier can add his thoughts. Based on the information I have at the moment, I still think this is an issue we can't and don't need to solve.

robinscholz commented 4 years ago

Are you actually being attacked by such a lockout attack at the moment? Or what is the reason you need to disable the brute-force protection? To be honest, I have never seen such an attack or heard of it so far. That's why I wrote that I see it as a non-issue.

Yes this actually happened to a site I built (headless Kirby with a Nuxt.js Frontend). It is also mentioned in the article linked above, so it seems to be a known problem.

Maybe it would already help if the brute-force protection could be disabled per account. Additionally it'd be great if panel access could be restricted per account as well.

Thanks for reopening the issue!

lukasbestle commented 4 years ago

So do I understand you correctly that you use the Kirby API with a hardcoded user and password in the frontend of a site? That's not supported and therefore not recommended – it's more reliable and secure to use content representations to provide responses for your content. As content representations don't need authentication at all, this issue won't happen.

robinscholz commented 4 years ago

Yes, I use the Kirby REST API to fetch site data for a frontend. Not only on this site that was DOS attacked, but on a few others as well.

Content representations would mean creating a publicly consumable API, which might not be wanted. It would also mean more work, since the output would have to be created from scratch.

It’s news to me, that the API-utilizing approach is supposedly not supported. Most of it was discussed at length in Slack a while ago. There is also a plugin created for that specific purpose that is listed in the plugin directory of the Kirby website (https://github.com/robinscholz/better-rest). @bastianallgeier was even kind enough to help out with a few things for this plugin.

Kirby as a headless CMS (via the REST API) was also discussed at length in the forum: https://forum.getkirby.com/t/kirby-3-as-headless-cms-for-vue-nuxt/12142/73

Again, it'd be quite simple to fix this by disabling brute-force protection on a per-user basis. This is also something the Kirby KQL plugin would benefit from.

lukasbestle commented 4 years ago

You are right that we discussed this "API for headless use" topic several times already. You know my opinion on this:

The Kirby 3 API makes it easy to access site data from other servers or systems, but shouldn't be accessed directly from the web frontend. For that, we explicitly have the content representation feature, which is tailored for exactly that use-case – it allows to define endpoints to return exactly the structured data you need for every kind of frontend.

Content representations would mean creating a publicly consumable API, which might not be wanted.

Could you explain why not? If you access the API from the frontend of your web application, the access is by definition public.

Again, it'd be quite simple to fix this by disabling brute-force protection on a per-user basis. This is also something the Kirby KQL plugin would benefit from.

I disagree. If the issue can be fixed by not triggering the protection in the first place, that's going to be a lot more reliable and secure than to disable the feature using configuration that will not only make Kirby more complex, but also less secure.

The proper fix in this case is to use the API for what it is meant to do (authenticated access to the Kirby backend from the Panel or when automating processes between systems), not for frontend access. Hardcoding credentials in frontend code is a hack that we don't support as it's difficult to get right (after all you need to be 100 % sure that your role permissions are locked down etc.). Unauthenticated API access is planned, but not there yet. Until we have that feature, use the content representations that are an official feature.

That's all I have to say about this issue.

robinscholz commented 4 years ago

The Kirby 3 API makes it easy to access site data from other servers or systems, but shouldn't be accessed directly from the web frontend. For that, we explicitly have the content representation feature, which is tailored for exactly that use-case – it allows to define endpoints to return exactly the structured data you need for every kind of frontend.

If this is how the whole teams’ view, it should be clearly communicated in your docs. As of now you can find the following sentence at the very top of the API docs: “You can use it for your SPA or mobile applications built on top of Kirby.” To me this sounds like the opposite of what you are saying.

Could you explain why not? If you access the API from the frontend of your web application, the access is by definition public.

Dealing in hypotheticals here. For example you might not want to create an API that is consumable by other sites, creating unwanted traffic.

I disagree. If the issue can be fixed by not triggering the protection in the first place, that's going to be a lot more reliable and secure than to disable the feature using configuration that will not only make Kirby more complex, but also less secure.

This can probably be done in ~20 lines of code, so I don‘t see what’s so complex about it. It would also be disabled by default, so as secure as it is now.

The proper fix in this case is to use the API for what it is meant to do (authenticated access to the Kirby backend from the Panel or when automating processes between systems), not for frontend access. Hardcoding credentials in frontend code is a hack that we don't support as it's difficult to get right (after all you need to be 100 % sure that your role permissions are locked down etc.).

Sure, its the developer’s responsibility to get this right, not yours, so it shouldn’t concern you.

Until we have that feature, use the content representations that are an official feature.

Access to the REST API appears to be an official feature as well. Otherwise it wouldn’t be documented so extensively, would it?

All in all, your position sounds rather condescending. In the end, you can’t really tell people how to use Kirby. I know you do not think highly of frontend frameworks, like Vue or React, but they absolutely have their place in modern web development. Utilising Kirby’s REST API or @bastianallgeier’s KGL plugin turn Kirby into a headless CMS with basically zero effort, which makes it a great option for a modern stack. I agree that content representations are also an option, but from an economic point of view they make less sense, especially for a large variety of content.

texnixe commented 4 years ago

Access to the REST API appears to be an official feature as well. Otherwise it wouldn’t be documented so extensively, would it?

Yes, access to the REST API is an official features. But access to the API needs to be authenticated, session based for JavaScript applications and using Basic Authentication for server-side stuff. Or at least, that is what I learned.

While BasicAuth can also be used for frontend authentication, this is not really intended usage.

I think that's probably what @lukasbestle was trying to say.

This is definitely not ideal and there should be a different way to consume the API, maybe via an API key if some sort of protection is required.

@robinscholz How do you currently disable bruteforce protection?

robinscholz commented 4 years ago

Yes, access to the REST API is an official features. But access to the API needs to be authenticated, session based for JavaScript applications and using Basic Authentication for server-side stuff. Or at least, that is what I learned.

The frontend is a Nuxt.js app, so a combination of SSR and SPA. I'd happily work with the CSRF method, but as I understand there is no possible way of creating the token outside of Kirby, correct?

This is definitely not ideal and there should be a different way to consume the API, maybe via an API key if some sort of protection is required.

Agreed.

@robinscholz How do you currently disable bruteforce protection?

Currently I don’t. Just keeping a close eye on the log file to see if anyone is trying again. Not exactly a long term solution.

afbora commented 3 years ago

I am closing this issue as it is predicted to be unsolvable.

lukasbestle commented 3 years ago

I'm reopening this issue so that @bastianallgeier can comment on it before we as a team actually close it.

My stance on this is still the same, but I'm interested in Bastian's opinion.

distantnative commented 3 years ago

@bastianallgeier ping

bastianallgeier commented 3 years ago

Sorry for coming back to this so much later.

If we want to step up our game as a true headless CMS option, we need to start thinking about ways to improve access to the API for client and server side applications. KQL is a nice step in the right direction to offer the kind of flexibility needed to access all kinds of data, but the bigger issue is our access management.

We can only solve the auth block issue when we start introducing different ways to access our API. I think there should be a separation between content consumption and content creation in the first place. Ideally, we would offer KQL as native content consumption layer instead of our REST GET endpoints with fine-grained control which data can be read by which user. Simple read-only users could "authenticate" with just a public API key and rate limiting would be enabled to avoid DDoS attacks.

For the consumption of more critical data or mutation endpoints, you would still need a fully authenticated user.

lukasbestle commented 3 years ago

I agree that fine-grained control over KQL permissions as well as a native KQL implementation would be really great. It's nothing we can implement quickly, but I think it would be useful for many such use cases.

@robinscholz What do you think about this idea? Would that be a good fit for you as well?

robinscholz commented 3 years ago

I agree that it boils down to access management. I shouldn’t have to create a real user that can theoretically access the panel just to be able to fetch data from or even post data to the CMS.

I've played around with KQL in the past and found it to be quite powerful, but sadly hard to fully understand/learn. Error handling back then was also basically non existent, which made debugging complex queries rather difficult. I do think it would be a decent option if these problems are addressed.

I haven’t worked with Kirby or KQL in over a year, so my feedback might be outdated!

bastianallgeier commented 3 years ago

@robinscholz just out of curiosity. Was this issue a reason why you haven't been using Kirby in over a year?

robinscholz commented 3 years ago

@bastianallgeier I've switched jobs and haven’t had much time or interest in working on website projects that would benefit from Kirby.

However, I did start to look for alternatives due to this problem, since most of the frontend web development I do is Vue/Nuxt. If I'd go down the headless kirby route again, I'd probably use content representations at the moment.

bastianallgeier commented 3 years ago

We are closing this in favour of better headless options in a future release.