API SQL Injection

[connortechnology]

API is vulnerable to SQL injections via so called "named parameters" ( https://book.cakephp.org/2.0/en/development/routing.html#named-parameters ).

In the EventsController::index() and MonitorsController::index() methods (located in web/api/app/Controller/EventsController.php and web/api/app/Controller/MonitorsController.php), $this->request->params['named'] is directly being used as conditions for Model::find() calls, and unless named parameters are explicitly whitelisted ( https://book.cakephp.org/2.0/en/development/routing.html#controlling-named-parameters ), arbitrary values can be passed, which means that it's possible to define the key side of the conditions array, which is where CakePHP accepts plain SQL.

Here's a basic example for testing purposes:

/zm/api/events/index/Id = 1 OR 1=:1.json

This will result in an conditions array that looks like

[
    'Id = 1 OR 1=' => 1
]

which will subsequently generate the following SQL:

... WHERE Id = 1 OR 1=1

Named parameters should either be whitelisted on router level, either per route ( https://book.cakephp.org/2.0/en/development/routing.html#per-route-named-parameters ), or globally via Router::connectNamed(), ie every possible column/operator combination (like Id >=, Id <=, Id =, etc...) needs to be registered, or they should be validated against whitelists manually accordingly.

The above example has been tested against the latest release and the master branch, using the release and the development docker files ( https://github.com/ZoneMinder/zmdockerfiles ).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[rhaamo]

@connortechnology can you confirm if this is still the case or it have been fixed at some point and forgotten to update the issue ?

[niloc132]

Definitely a problem - from the demo site (zmuser/zmpass), try this link

https://demo.zoneminder.com/zm/api/events/index/Id%20=%201%20OR%201=:1.json

It should show no rows (since that isn't a valid ID), but instead it shows all rows. Anyone could use this to execute more complex SQL, which potentially could include deleting or modifying events, triggering alarms, adding users, changing permissions, running shell code, etc.

[IdanDavidi]

Any news about this vulnerability?

[connortechnology]

Nothing has been done about it.

Please note that this is only a vulnerability if you do not use authentication. You have to be logged in to do this.. So use authentication.

[mnoorenberghe]

@pliablepixels Does zmNinja do any API queries with OR (or similar) like #2099 (comment) shows? I'm trying to figure out whether we can limit the syntax to one column name and one operator between a / and the : and then for the right of the :, can we limit that to a literal value or should we allow a column name there too?

[pliablepixels]

I don't use OR or similar, but I do use a lot of nested paths like

/StartTime >=:time/EndTime <=:time/Notes%20REGEXP:detected%3A/

[mnoorenberghe]

OK, yeah, that is fine. I won't limit how many path segments there are.

Side note: I just found the docs for CakePHP Complex Find Conditions and it specifically warns about this kind of SQLi:

CakePHP only escapes the array values. You should never put user data into the keys. Doing so will make you vulnerable to SQL injections.

Looks like it's too late to listen to that if we don't want to break all consumers… we'll just have to do our own sanitization.

[mnoorenberghe]

@pliablepixels Do you ever include the table name in your query e.g. Table.Field? I'm wondering if I can exclude periods?

[pliablepixels]

@mnoorenberghe not to my knowlege. That being said, once you are done with the changes I'll pull and test. But to this specific question, I'm pretty sure I don't use table names anywhere. I don't remember, however, if there are other applications of "." inside a query