SQL injection in external database enrol plugin?

SQL injection in external database enrol plugin?

by David Loscutoff -
Number of replies: 5

We use a modified version of the external database plugin for our enrolments.  Looking through the code, I found several things that raised concerns for me:

  1. User-entered fields like $localcoursefield are concatenated into queries with no validation (lib.php, 341, e.g.).
  2. The db_get_sql function does not validate the $table, $fields, or $sort arguments; the first two of these can contain user input.
  3. The dbsetupsql config option allows a user to execute any SQL statement against the external database.

Granted, the only users who have access to these input fields are site admins.  However, I'm thinking: first, what if someone hacks an admin's password; second, allowing SQL injection is generally a bad thing anyway.

Am I seeing this right?  I'd like to get confirmation before starting a Tracker issue.

Average of ratings: -
In reply to David Loscutoff

Re: SQL injection in external database enrol plugin?

by David Loscutoff -

Upon looking at it again, I realized that #1 isn't true: the local course field is selected from a drop-down, so users cannot enter arbitrary values.

However, #2 is still valid.  For example, line 366 calls db_get_sql, passing array($coursefield) as the $fields argument.  But $coursefield is simply the remotecoursefield config entry with whitespace trimmed.  This config option is a text box that allows arbitrary input.

Also, #3 remains unchanged.

Average of ratings: -
In reply to David Loscutoff

Re: SQL injection in external database enrol plugin?

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Hi David,

IMHO, you are seeing it right. #2 could probably be tightened, but I'd say #3 is not going to be so easily removed. Unfortunately some people need to execute some external-db-specific SQL code before Moodle can get data in a proper encoding. And catering for all the posible alternatives from the Moodle side is not that easy.

Saludos.
Iñaki.
Average of ratings: -
In reply to Iñaki Arenaza

Re: SQL injection in external database enrol plugin?

by David Loscutoff -

Thanks for the confirmation, Iñaki.  Next question: if #3 (the obvious hole) is not easily fixable, is it still worth opening a Tracker issue for #2 (less obvious)?

(Regarding #3, my first thought is to make sure at least that the DROP keyword isn't present in the setup SQL... but I suppose someone more clever than me could find a way to get around that.)

David

Average of ratings: -
In reply to David Loscutoff

Re: SQL injection in external database enrol plugin?

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Hi David,

yes opening a tracker issue for #2 is a good idea. Even if only the admins can set those config values, making it a bit more difficult to shoot oneself in the foot is always a good idea smile

Regarding #3, I've only worked with Postgresql, MySQL and Oracle, and know that only a very specific SQL command is necessary in each case when working with a standard setup (thus we would be able to whitelist them). But I can't speak for the other twenty or so supported database engines (if one can call CSV files a database engine at all).

Saludos.
Iñaki.
Average of ratings: -