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

Reconsider URL signatures #5018

Closed
javiereguiluz opened this issue Feb 2, 2022 · 30 comments · Fixed by #5084
Closed

Reconsider URL signatures #5018

javiereguiluz opened this issue Feb 2, 2022 · 30 comments · Fixed by #5084

Comments

@javiereguiluz
Copy link
Collaborator

I'm opening this debate after some private discussions in Slack.

The question is: are the EasyAdmin signed URLs a truly useful feature ... or are they cumbersome and unneeded because they provide a false sense of extra security?


Backend URLs are signed by default (you can disable that) via the UrlSigner based on (copied from) Symfony: https://github.com/EasyCorp/EasyAdminBundle/blob/4.x/src/Router/UrlSigner.php

We don't sign all query params, because it's "technically impossible". We don't sign things like search queries and filters, because they are dynamic and would need signing URLs in JavaScript (which currently we can't do).

The idea is that by signing the main query params, we're protected against these:

     * Instead of signing the entire URL, including all its query parameters,
     * sign only a few parameters that can be used to attack a backend by:.
     *
     *   * Enumerating all entities of certain type (EA::ENTITY_ID)
     *   * Accessing all application entities (EA::CRUD_CONTROLLER_FQCN)
     *   * Accessing any CRUD controller method (EA::CRUD_ACTION)
     *   * Accessing any application route (EA::ROUTE_NAME)
     *   * Meddling with the parameters of any application route (EA::ROUTE_PARAMS)

However, and considering that all real backends are already security-protected, is this true? An already-logged user in the backend can access to all entities (except if some voter prevents them accessing some entity). They can access to all entities via pagination and searching, so not allowing them to change the URL query param won't prevent them from accessing to those entities.

It's probably the same for the other parameters. E.g. not allowing them to change the route param is a minor extra protection, because if that URL is really important, it will have its own security restriction defined, so the user won't be able to access to it anyways, even if they can change the route name in the URL.


The best cloud services such as AWS, Azure and Google Cloud only sign URLs for features related to downloading resources/files ... to protect and time-limit the download of those resources.


So, should we stop signing backend URLs?

@jmsche
Copy link
Contributor

jmsche commented Feb 2, 2022

I'm not sure I understand the issue here.

The feature is already present and can be disabled, so why should it be dropped?

@ioniks
Copy link

ioniks commented Feb 2, 2022

Same as @jmsche.

When you go to entity show view for exemple, you can"t modify Id in URL and go to different ID, you have to go list and click on other show link to have this signature.

I think remove the referrer url from the query url is more needed than this or make more pretty the url.

This feature can be desativated by default if you want.

@javiereguiluz
Copy link
Collaborator Author

@ioniks about "pretty URLs", our opinion is that it doesn't matter because this is a backend. About "referrer", we definitely need it to provide a nice navigation flow through the backend (we can't rely on HTTP Referrer Header because some browsers block it).

@javiereguiluz
Copy link
Collaborator Author

@jmsche the question is not about enabling/disabling but about ... "is this really protecting anything? Does this really prevent you doing something that you cannot do in other ways?"

If we think this is truly useless, let's remove it. Less code to maintain and many less problems for users (including the current impossibility of generating dynamic URLs using JavaScript).

@ioniks
Copy link

ioniks commented Feb 2, 2022

@javiereguiluz i think a config for have referrer in url or not is better, but it's not the question here ;)

@jmsche
Copy link
Contributor

jmsche commented Feb 2, 2022

I don't think this is useless, but if you have to drop it for a good reason I'm okay with this.

@michaelKaefer
Copy link
Contributor

I am for removing it. Nobody has found any real use case or attack where this feature protects the application in the discussion so far. Even for a signed URL we still must check for permissions (for routes, actions, entities,..). And, the Symfony security features already provide almost everything I ever needed..

Removing it means less code, less complexity, less maintainance, less issues hopefully..

I also agree to: “provides a false sense of extra security“ - unless somebody can give a real example where this really can provide security.

@john-dufrene-dev
Copy link
Contributor

john-dufrene-dev commented Feb 2, 2022

@michaelKaefer
I think it's a not a good idea to remove URL signature. Lot's of CMS use this feature for security reasons.

First of all, it is simply possible to deactivate it if you do not wish to use it.

Then, I use it in several projects and it allows to protect the urls, we cannot access another entity if we use a UUID identifier and change it directly in URL.

@javiereguiluz
Copy link
Collaborator Author

@john-dufrene-dev but, if you search in the backend for that other UUID (or part of it) you'll get that other entity and you can see its details.

In other words, can anyone prove that signing URLs protects some resource in some way that you cannot access it via a different way (e.g. via pagination, search or filtering)?

@john-dufrene-dev
Copy link
Contributor

john-dufrene-dev commented Feb 2, 2022

@javiereguiluz Okay, I understand what do you mean, sorry my english not the best.

Url signature in my opinion makes it possible to protect a change of parameters directly in the url and therefore the injection of malicious code.

Indeed to block access to resources with filters, search or other it is necessary to use a security system like Voter.

But I don't understand why just disabled this feature if you don't need it ?
By default could be disabled too

@javiereguiluz
Copy link
Collaborator Author

Why remove this (if we reach the conclusion that it doesn't protect anything at all)? Because having this feature not only means maintaining it ... but it also complicates other features; even making them impossible (e.g. because we can't generate routes dynamically using JavaScript).

@Crell
Copy link

Crell commented Feb 2, 2022

I am not a security expert by any means, but I don't really see what benefit it offers. I would be fine seeing it go away.

@ioniks
Copy link

ioniks commented Feb 2, 2022

I am not a security expert by any means, but I don't really see what benefit it offers. I would be fine seeing it go away.

If another user send you a link with delete thing and change the ID for exemple the id of your account or the admin account.
You can broke your app.

@michaelKaefer
Copy link
Contributor

michaelKaefer commented Feb 2, 2022

@ioniks This sounds like a CSRF attack, I think you should be protected against it by using Symfony forms out of the box. I did not test it now but I think it is like that. For deleteions I'm not sure now if forms are used or if there is any CSRF protection in EA (I would strongly guess there is), I could check that the next weeks. Also I'm not sure if deletions are possible in EA with a GET request (= clicking on a link), I did not test it now.

@fabpot
Copy link
Contributor

fabpot commented Feb 2, 2022

I would love to see this "feature" removed. I seems it gives a false sense of security, security that should already be covered by Symfony via security, access control, XSS, CSRF, ...

@parijke
Copy link
Contributor

parijke commented Feb 2, 2022

It is switched off in all my projects so okay for me as well

@recchia
Copy link

recchia commented Feb 3, 2022

i agree in removing it, there are other specific ways to protect resources

@fdiedler
Copy link

fdiedler commented Feb 4, 2022

For me, it does not really protect your backend. There are several ways to protected again URL hacks and using EA permissions and Symfony security (voters by example) is sufficient, even if you can hack the URL, you will not be able to perform the action if you don't have rights to do it.

Just a thing, passing the referrer in the URL seems ugly, why not passing the referrer using sessions ?
Same for the crudController crudControllerFqcn=App\Controller\Admin\UserCrudController maybe just do crudControllerFqcn=UserCrudController and find a way to retrieve the namespace App\Controller\Admin if possible.

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Feb 4, 2022

The fact that the admin area is secured by some roles doesn't prevent CSRF attack, so I would not count firewalls as an argument in favor of removing the signature.

a few parameters that can be used to attack a backend by [...]

This is what roles protect against: having a non allowed user do those things. But signing the query doesn't help at all in this regards - the firewall is enough.

To me, the signature is absolutely required to prevent against CSRF attack because easyadmin allows changing the state of things via GET requests. This is a critical design issue in EA to me, and the signature mitigates the issues this opens.

My personal conclusion is: yes, let's remove these signatures, but only after we ensure that all requests with side effects go through a POST.

@michaelKaefer
Copy link
Contributor

michaelKaefer commented Feb 4, 2022

@javiereguiluz This is already solved, or? The actions new, edit and delete use POST requests already (still they use query parameters like in a GET request but the method is POST) - or am I missing something? Plus: all 3 use CSRF tokens by default (I just checked it).

I will try to write some functional tests the next days to check it.

@javiereguiluz
Copy link
Collaborator Author

@michaelKaefer yes, everything should be OK: delete action has been using forms for long, edit/new used POST requests since the begining I think. The only issue was the toggles, which made a GET request to change the value of a property, but you fixed that in #4981. We also use CSRF tokens everywhere, so I think all issues were solved 👍

@richard4339
Copy link
Contributor

I don't feel qualified to give an opinion, but if they are kept, I'd definitely support making disabling the signatures easier via configuration.

@javiereguiluz
Copy link
Collaborator Author

@richard4339 as explained in https://symfony.com/bundles/EasyAdminBundle/current/dashboards.html#dashboard-configuration you can disable signatures just by calling one method on your dashboard:

    public function configureDashboard(): Dashboard
    {
        return Dashboard::new()
            // ...
            ->disableUrlSignatures();
    }

@richard4339
Copy link
Contributor

@richard4339 as explained in https://symfony.com/bundles/EasyAdminBundle/current/dashboards.html#dashboard-configuration you can disable signatures just by calling one method on your dashboard:

    public function configureDashboard(): Dashboard
    {
        return Dashboard::new()
            // ...
            ->disableUrlSignatures();
    }

Thanks @javiereguiluz I'd completely missed that.

@michaelKaefer
Copy link
Contributor

@javiereguiluz Thank you for your infos! In my fix #4981 I missed to fix the CSRF problem so I think this must be done before removing the URL signatures like others pointed out above. I created an issue: #5028. For the actions I did a PR which adds tests.

javiereguiluz added a commit that referenced this issue Feb 9, 2022
This PR was merged into the 4.x branch.

Discussion
----------

Add tests for CSRF attacks

Tests if CSRF attacks are possible in the actions `new`, `edit` and `delete`.

See #5018 (the method is not tested but it is `POST`).

Nice side effect: increases code coverage from 20% to 42% of all lines :)

Commits
-------

6046832 Add tests for CSRF attacks
@michaelKaefer
Copy link
Contributor

The PR was merged: #5028. That means that URL signatures could be removed now I guess. @javiereguiluz Did you decide yet and do you want a PR?

javiereguiluz added a commit that referenced this issue Mar 12, 2022
… (javiereguiluz)

This PR was merged into the 4.x branch.

Discussion
----------

Improved the JavaScript code related to file upload files

Similar to the changes done in #5016, #5017 and #5018.

Commits
-------

6376656 Improved the JavaScript code related to file upload files
@Amunak
Copy link
Contributor

Amunak commented Aug 15, 2022

A non-obvious side effect of removing signatures is that with multiple dashboards you can bypass security set with setEntityPermission on a given Dashboard's configureCrud method.

The documentation never really says that different Dashboards are completely separate (because they aren't), but it definitely feels that way due to how the configuration is set up.

You could have two dashboards (even in different namesapces!) that don't share any controllers, entities or anything, and rely on the Dashboard security through setEntityPermission. This works correctly when you only use internally generated links (except for an edge case where AssociationField automatically links to the target CRUD controller that's supposed to be displayed under a different dashboard than the one that links to it), but you can manually craft a URL with the different dashboard, and since now the configureCrud method is not run (or more specifically, it's run under the other dashboard) this can lead to change in privilege requirements and access to resources the user shouldn't have access to.

Importantly, the documentation also suggests that "securing the whole backend" through security.yaml's access_control is a good idea, which it ... isn't. It should be more explicit that it's a good first line of defense, but one should use Voters for access control and check each action properly, ignoring what dashboard it runs from.

I think the documentation should be more clear about this behavior, and ideally setEntityPermission should not work under dashboard's configureCrud because it's meaningless.

Well that, or even better, force users to explicitly "link" controllers with dashboards, so a given Controller can only be under the dasboard it's allowed to live under. This could also improve linking between Controllers of different Dashboards. Which, as a side note, is problematic - f.e. with the AssociationField.

@michaelKaefer
Copy link
Contributor

IMHO the app dev is responsible for configuring EA - which includes calling setEntityPermission() on every DashboardController where it's necessary.

(For access_control vs. voters: there are apps that don't need any custom voters nor special entity permissions)

@Amunak
Copy link
Contributor

Amunak commented Aug 19, 2022

calling setEntityPermission() on every DashboardController where it's necessary

I'm not sure you understand; when you have different dashboards that are meant to be completely separate with their separate Crud Controllers and also separate permissions, you can still use either Dashboard to render any Crud Controller that exists in your app with no limit, potentially using lower permissions to access a resource that's supposed to be better protected.

That's extremely non-obvious and IMO should be fixed or at the very least explicitly documented.

@michaelKaefer
Copy link
Contributor

Yes I understood it like this. I don't see a problem with this, AFAICS a CRUD controller doesn't belong to one dashboard. Maybe you can add it to the docs if you miss something. Or maybe there are more opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.