Clarification of my understanding of SQL injection

Clarification of my understanding of SQL injection

by David Mudrák -
Number of replies: 6
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Plugins guardians Picture of Testers Picture of Translators

Hi.

I would like to learn more about the actual impacts of SQL injection risks in Moodle.

Sometimes when a security issue related to SQL injection is reported, it mentions potential risks such as updating existing database records, deleting them or inserting new ones. I would like to understand how exactly it could work with SELECT queries.

Moodle's own documentation on SQL injection seems to spread some fear, uncertainty, and doubt in its example which I don't think is actually right.

From how I read the code, all Moodle database drivers throw exception whenever there is an attempt to execute multiple statements ("cannot insert multiple commands into a prepared statement").

So let's say we have a code like this:

// DO NOT DO THIS, IT IS AN EXAMPLE OF BADLY WRITTEN CODE.
$id = $_GET['id'];

$r = $DB->get_records_sql('SELECT * FROM {config} WHERE id = ' . $id

Now. I do understand how the attacker can submit values such as 3 OR 1=1 or 0 UNION SELECT ... etc and make that statement return other / more records, data from other tables and things like that. Depending on the actual code, this can easily leak the content that would not be normally available. Such as someone else's assignment submission, quiz attempt data, admin password reset token and so on. These all are valid and serious security issues and must be fixed (typically by the input validation and placeholders in the query).

I would like to know if there is a way how SQL injection bugs in read statements could be potentially abused for directly modifying the database. By directly, I mean that the attacker can of course amend the returned data, and the following code can perform certain actions based on the data (such as iterating through them in a loop and doing other actions etc). That risk is clear to me. What I cannot see is a way how the attacker could replace these SELECT statements with some UPDATE / CREATE / DELETE ones.

Please don't get me wrong, my intention is not to lower the importance or impact of SQL injections in SELECT queries. I would just like to be sure I understand the actual mechanics well. Thank you for sharing your experience.

Average of ratings: Useful (2)
In reply to David Mudrák

Re: Clarification of my understanding of SQL injection

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I have often wondered this myself. I have always though that the developer must have to be massively careless to make this a serious threat but I'm probably being naive.
In reply to David Mudrák

Re: Clarification of my understanding of SQL injection

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
XKCD nailed this years ago: https://xkcd.com/327/

They key part of the exploit there is not the DROP TABLE, but rather the '; In your case the key argument is an int, not a string, so the hack attempt would need to be

.../davids_dangerous_script.php?id=0; DELETE FROM mdl_user

(with URL encoding. I left it unencoded for readability.)

Generally, sripts that seek out potential SLQ injections try a whole bunch of initial strings in the value, like
;
';
');
'));
etc.
In reply to Tim Hunt

Re: Clarification of my understanding of SQL injection

by David Mudrák -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Plugins guardians Picture of Testers Picture of Translators

Yes Tim. And that's my point. I don't think this would pass the guard we have in every driver's execute() method:

        if (strpos($sql, ';') !== false) {
            throw new coding_exception('moodle_database::execute() Multiple sql statements found or bound parameters not used properly in query!');
        }

Again - I know that read access is enough to take over the control over the server so every SQL injection is serious. But I think we should stop using Bobby Table as the example of the actual risk. Unless I am missing something, and that is why I opened this.

In reply to David Mudrák

Re: Clarification of my understanding of SQL injection

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Well, Moodle puts additional hurdles in the way of people who want to try to exploit any holes discovered. That is good, but it still misses the point:

Don't write code like this. Use placeholders, and use optional/required_param

$id = required_param('id', PARAM_INT);
$r = $DB->get_records_sql('SELECT * FROM {config} WHERE id = ?', [$id]);

Or, since id is a primary key:

$id = required_param('id', PARAM_INT);
$r = $DB->get_record('config', ['id' => $id]);

That way, SQL injection is just not possible. This is the way you shoudl think about coding.

Whether Moodle's defence-in-depth is good enough to mitigate terribly-written code might be a theoretically interesting dicussion, but it should be of no practical importance. Code like that should not get written, and when it does, it should be caught during review.
Average of ratings: Useful (1)
In reply to Tim Hunt

Re: Clarification of my understanding of SQL injection

by David Mudrák -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Plugins guardians Picture of Testers Picture of Translators

might be a theoretically interesting dicussion

And that was my whole intention here. I am very well aware of all the benefits of using the API and input validation etc. And yet, I prefer to just have deep understanding how things actually work.

In reply to David Mudrák

Re: Clarification of my understanding of SQL injection

by Diane Soini -
Don't assume that SELECT queries by themselves are benign. You can bring down a database with a select query.

For moodle dev, use the moodle database API. I think they call it Data Manipulation API. For other dev, look into using the PHP filter_var functions as well as something like PDO where you use placeholders and parameter types like moodle uses.