573839 - SQL Exception Error For Unexpected Input Values

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Michael Coates [:mcoates] (acct no longer active)]

Issue

An SQL exception is thrown by /history for particular values of the "olderthan" parameter. It is unclear if this vulnerability could be used to perform SQL injection attacks since the vulnerability will occur without changing the data type or adding special characters. However the presence of an SQL exception is a clear indication that a problem is present that could lead to SQL injection.

Examples:
Valid Request
http://tbpl.swatinem.de/SeaMonkey/history?olderthan=1277243661&atleast=18

Generates Exception:
http://tbpl.swatinem.de/SeaMonkey/history?olderthan=123&atleast=16

Generates Exception:
http://tbpl.swatinem.de/SeaMonkey/history?olderthan=abc&atleast=16

Exception:
Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') AND r.tree = 'SeaMonkey' ORDER BY endtime ASC' at line 1' in /var/www/tbpl/index.php:195
Stack trace:
#0 /var/www/tbpl/index.php(195): PDOStatement->execute()
#1 {main}
  thrown in /var/www/tbpl/index.php on line 195


Note: These requests are initiated by JavaScript code when visiting http://tbpl.swatinem.de/. They can be viewed by using a web proxy and manually requested within the browser.


Recommended Remediation

The code in question constructs an SQL statement using string concatenation for the $pushids variable. This approach is vulnerable to SQL injection if user controlled data is ever present within $pushids.  Modify this code to either use static values or to use bind parameters to safely handle dynamic values.

index.php
Line: 193
$runsstmt = $db->prepare('SELECT DISTINCT r.id, logfile, m.name as machine, starttime, endtime, state FROM runs AS r JOIN runstopushes AS rp ON r.id = rp.run JOIN pseudomachines AS m ON m.id = r.machine WHERE rp.push IN ('.implode(', ', $pushids).') AND h = :tree ORDER BY endtime ASC;');

Comment 1

[Arpad Borsos [:Swatinem]]

(In reply to comment #0)

> The code in question constructs an SQL statement using string concatenation for
> the $pushids variable. This approach is vulnerable to SQL injection if user
> controlled data is ever present within $pushids.

The var is generated out of sql ids, so it should be safe. As far as I know, there is no way to use bind parameters with arrays and sql "IN" statements.

Comment 2

[Michael Coates [:mcoates] (acct no longer active)]

In that case lets add strict input validation on the input parameters so that we can gracefully handle these values without resulting in a SQL exception.  Where you able to determine why a different numeric value, such as "123" would also cause the exception?

Comment 3

[Arpad Borsos [:Swatinem]]

In that case, the query above would have no results, so the concat will result in an empty string.

Comment 4

[Michael Coates [:mcoates] (acct no longer active)]

The goal is to use input validation checks to determine if the URL arguments are valid. If they aren't, then we abort the SQL statements all together.  This will accomplish two things:

1. No worries of SQL injection since the bad data would never make it to the SQL statement. (And we are binding all other parameters)
2. No SQL error message returned to our users.  This is best practice from a security perspective.

Comment 5

[Arpad Borsos [:Swatinem]]

Fixed: http://hg.swatinem.de/tbpl-server/rev/5a26b8034975