Skip to content
This repository has been archived by the owner on Feb 4, 2023. It is now read-only.

EditController is a security issue? #896

Open
Nibbels opened this issue Jun 16, 2019 · 4 comments
Open

EditController is a security issue? #896

Nibbels opened this issue Jun 16, 2019 · 4 comments
Labels

Comments

@Nibbels
Copy link

Nibbels commented Jun 16, 2019

if (!$this->isCsrfTokenValid('sg-datatables-editable', $token)) {

Hello there,

please look over this. In case I got a valid csrf token from an editable table
I think I can edit any column in any entity using this controller.

Especially if I use the FOSUserBundle I suspect that this might be a way to corrupt the User::class or the Roles::class or the Groups::class entity.

What I would suggest:
1)
I would not transfer the entities name from the frontend. Transfer the service name of the datatable instead. Example: 'service.datatable.animals'
Then get the Entities name from $this->get('service.datatable.animals')->getEntity().
Then look up the row inside the datatable and check if that column is editable. If not throw some error.
Additional security: Add support for some kind of roles check within the edit controller. Maybe the editable_if-closure() (which should be able to be read from the datatable) can be of help here. -> if the closure sais no inside the editcontroller throw an exception.

Or there might be some trick to have more custom csrf tokens which only are valid for a specific datatable and column.

Greetings

@Seb33300
Copy link
Collaborator

Can you please provide a pull request?

@Nibbels
Copy link
Author

Nibbels commented Jun 16, 2019

Yes, that was one thought.
Rightnow there is some Family <-> Daddy <-> Hobby conflict.
I could write a pull request but I do have to build up some programming, symfony and testing enviroment which I do not have as a private person. For now I just read the code and saw the issue.

More urgend questions are:

  • I provided two possible solutions. Which one does the community prefer?
  • Which one might work.
  • Which one might work for most of the applications.
  • Does Symfony / Firewall provide a propper solution/configurable parameter ruleset for this problem which I dont know?

To harden the controller in no time I just pulled it out of the sgdatatables bundle. Then rerouted to the new controller having hardcoded questions what to edit and what not. -> ugly.
But I still have the question about what the prettiest solution might be.

Greetings

@althaus
Copy link

althaus commented Nov 14, 2019

I'm only using the bundle for an admin section, so the issue doesn't bother me, but I see your point.

In general I'd suggest a voter. Something like a SgDatatableVoter which would check each edit request. That (meta)voter could then reroute the decision management to your custom services.

@stwe
Copy link
Owner

stwe commented Apr 5, 2021

I'll leave the ticket open - as a good example.

You can replace or overwrite all classes yourself. Ready-made classes are often just suggestions, especially for the components that contain the logic. Nobody should simply adopt this logic unchecked. The game is called programming and that means THINKING.

Thanks to Nibbels for reporting.

@stwe stwe added the Security label Apr 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants