This bug was reported to the security team, who has cleared this issue to be fixed publicly.
In media_mover_api.module, in the function media_mover_api_get_files() at line 988, the variable $num is embedded directly in the query. The value of this variable comes from the media mover configuration (either $c->mma_process_num, $c->mma_storage_num, $c->mma_complete_num), and the configuration is editable in a form (for example, http://curiosae/en/admin/build/media_mover/config/1/edit).
Furthermore, the form accepts any text for the fields mma_process_num, mma_process_num and mma_process_num. So it it possible to set the value of $c->mma_process_num to something like "1; DROP TABLE nodes".
There is another similar unsafe query in media_mover_api_node_files_fetch(), and maybe elsewhere (I did not review all the code).
The safe way to build a query with arguments in optional parts of the query is to use an array to hold the arguments, and pass this array to db_query() instead of passing the arguments explicitly.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | media_mover-sql-injection-1233162.patch | 2.11 KB | pfournier |
Comments
Comment #1
arthurf commentedThanks for bringing this up. I've committed some fixes which should resolve this to the 6.1.x branch but I have not rolled a new release. Some of the additional modules (node and cck most likely) should probably be reviewed. The 6.2.x branch should also be reviewed as some of these functions are duplicated there.
Because the injection is limited by roll (at least as far as I've been able to look through things) I've demoted the priority to major.
Comment #2
pfournier commentedHere is a patch for 6.x-1.0beta9. This patch is only for the two functions reported above. I did not review all the code.