Introduction

Hello Mambo folks,

I'm writing my diploma thesis on the introduction of security-related innovations into Open Source web applications. For my research I want to review the Mambo code for SQL injection and Cross-Site Scripting vulnerabilities. I already did a review of the "administrator/" backend for raw SQL code (see below). The review resulted in annotations in the code that mark (apparently) critical spots. My research hypotheses states that enriching code with annotations during reviews leads to more valuable reviews and eventually more secure code. This should be especially valuable for changes that affect multiple spots of the code. (see the section on annotations below).

The "problem"

You're using raw SQL statements throughout the code. You seem to be able to tackle the intricacies of escaping arguments and preventing SQL injection in general. It is, though, best practice to keep database access confined to a special layer. This makes filtering, escaping etc. easier as it is only done in one place. Also, the repsonsibility for sanitation is clear and not split up to different places in the code (sometimes creating confusion as to where to filter what). Failure to consistently handle SQL queries easilt results in successful SQL injection attacks.

You already have an abstraction in database.php for inserting and updating objects, which is definitely the way to go. But there are many other places where a query is not directly bound to an object. These queries happen outside the responsibility of the database "layer" (database.php).

I suggest that the use of SQL should be (in the best case, but a partial solution is more realistic for now) restricted to the database layer. An INSERT e.g. would look like this: insert($table, $values) where values is an assoc with "column" -> "value". This separation would clearly detach arguments from the static parts of the query,
give semantics to the arguments (e.g. the method could check if the table exists before trying to execute the query), make replacing the backend database easier and also smoothes out the transition to using a real framework (since database access is already centralized).

To make this transition easier I annotated the SQL statements that could be moved to the abstraction layer with (in my opinion) little effort. You can find it at http://www.noroute.de/downloads/research/mambo465_administrator_annotations_trivialonly.diff.

What it looks like

"@RawSQLUse, trivial_implementation, SELECT" is an example from my first round of annotations for Mambo. All annotations from my first session start with "@RawSQLUse" since this was the issue I looked for. With this consistent naming scheme, you can easily find all occurences of the annotations (using your favorite text editor for example).

And now what?

Move the statements to the abstraction layer! It's easier than you think! The "trivial_implementation" marker states that moving this query to an abstraction layer would need a new method in the database abstraction layer (probably in database.php) but this method need not contain complex program logic. Below I summarized the new methods needed. I made a patch for the implementation of insert(). You can find it at http://www.noroute.de/downloads/research/mambo465-insert-impl.diff.

new Low-Level methods

To be able to replace all SQL I annotated in the first session,
INSERT, UPDATE, DELETE and SELECT would need an abstraction method
that supports the following SQL features:

• columns/aggregate selection with AS (SELECT foo AS bar)
• WHERE selection with equality and AND (WHERE foo='bar' and baz='bar')
• WHERE selection with IN (WHERE 'foo' IN bar)
• GROUP BY, ORDER BY, LIMIT
• table selection with AS (FROM table AS t)

the method signatures would look like the following:

• insert($table, $values)
• update($table, $values, $where, $in = false)
• delete($table, $where, $in = false)
• select($columns, $tables, $where, $in = false, $limit = false, $order_by = false, $group_by = false)

$values, $where, $in, $order_by are assocs, $group_by is a list. The semantics are as follows:
$values: column->value, separated by ','
$where: column->value, separated by AND, passing false omits WHERE clause
$in: column->values, separated by AND, passing false omits IN clause
$order_by: column->ASC/DESC, separated by ',', passing false omits GROUP BY cluse
$group_by: columns, separated by ','

Annotations

Annotations provide several benefits over traditional review
techniques:

1. annotations make issues visible in the code, creating more incentive to fix them (have clean code)
2. counting annotations provides instant feedback on how much of the issue is fixed (allows for partially fixing an issue)
3. annotations motivate reviews since there is a visible (and lasting) result in the version control system (you get creditfor commits)

What is it about "SELECT" and "CONCEPT"?

All annotations include the type of SQL clause they use. This way you can easily search for all e.g. DELETE statements after you have written a DELETE abstraction method and start moving them to the abstraction layer. Neat, huh?

I added "CONCEPT" to an annotation as a hint that I think that there's a bigger concept behind this query and the abstraction could make for an "intentional" method name. For example an SQL clause like "DELETE FROM #__modules WHERE id IN ($cids)" could be abstracted to "removeModules($module_ids)". This would make the code easier and remove the burden of knowing details about how to remove a module from the caller. This is a really simple example. The advantages will be much bigger for complex queries.