DatatablesBundle icon indicating copy to clipboard operation
DatatablesBundle copied to clipboard

EditController is a security issue?

Open Nibbels opened this issue 6 years ago • 4 comments

https://github.com/stwe/DatatablesBundle/blob/5cca7f74c24017243616186e6fe5705709a5b98c/Controller/DatatableController.php#L61

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

Nibbels avatar Jun 16 '19 11:06 Nibbels

Can you please provide a pull request?

Seb33300 avatar Jun 16 '19 11:06 Seb33300

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

Nibbels avatar Jun 16 '19 11:06 Nibbels

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.

althaus avatar Nov 14 '19 14:11 althaus

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 avatar Apr 05 '21 10:04 stwe