SQL Exception Error For Unexpected Input Values

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
--
critical
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: mcoates, Unassigned)

Tracking

Details

(Whiteboard: [infrasec:sqlinject])

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;');
(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.
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?
In that case, the query above would have no results, so the concat will result in an empty string.
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.
Fixed: http://hg.swatinem.de/tbpl-server/rev/5a26b8034975
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Group: webtools-security
(Assignee)

Updated

3 years ago
Product: Webtools → Tree Management
(Assignee)

Updated

3 years ago
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.