How SQL Injection happens in PHP PDO queries and how to fix it

Introduction

In the PatitoOnlineJudge repository, we discovered a critical SQL injection vulnerability in ContestRepository.php at line 117. While most methods in this file correctly used prepared statements to interact with the database, the getOfficialContests() method took a dangerous shortcut: it directly interpolated the $site_id parameter into a SQL query string using PHP's variable interpolation. This single inconsistency created a textbook SQL injection vulnerability that could have allowed attackers to execute arbitrary SQL commands against the contest database.

What makes this vulnerability particularly concerning is that it appears in a repository that otherwise demonstrates good security practices. The same file contains numerous other methods that correctly use pdo->prepare() and parameter binding. This highlights how a single oversight—using pdo->query() instead of pdo->prepare()—can undermine an entire application's security posture.

The Vulnerability Explained

Let's examine the vulnerable code from line 133 of ContestRepository.php:

public function getOfficialContests($site_id)
{
    $stmt = $this->pdo->query("SELECT * FROM contest, contest_site
        WHERE contest.defunct = 'O'
        AND contest_site.contest_id = contest.contest_id
        AND contest_site.site_id = $site_id
        ORDER BY contest.contest_id DESC");
    return $stmt->fetchAll(PDO::FETCH_ASSOC);
}

The problem lies in line 136: AND contest_site.site_id = $site_id. Here, the $site_id variable is directly interpolated into the SQL string using PHP's variable interpolation syntax. When PHP processes this code, it replaces $site_id with its actual value before sending the query to the database. This means the database never knows that $site_id was supposed to be a separate data value—it just sees one complete SQL string.

The Attack Scenario

An attacker controlling the $site_id parameter could exploit this vulnerability to inject arbitrary SQL. Here's a concrete example:

Instead of passing a legitimate site ID like 123, an attacker could pass:

123 OR 1=1 UNION SELECT username, password, email, NULL, NULL, NULL, NULL FROM users --

The resulting query would become:

SELECT * FROM contest, contest_site
WHERE contest.defunct = 'O'
AND contest_site.contest_id = contest.contest_id
AND contest_site.site_id = 123 OR 1=1 UNION SELECT username, password, email, NULL, NULL, NULL, NULL FROM users --
ORDER BY contest.contest_id DESC

This injected SQL would:
1. Return all contests (via OR 1=1)
2. Append user credentials from the users table (via UNION SELECT)
3. Comment out the rest of the original query (via --)

Real-World Impact

For the PatitoOnlineJudge application, this vulnerability could allow attackers to:

• Extract sensitive data: Access contest information marked as private, user credentials, or administrative data
• Modify contest records: Change contest dates, participants, or results
• Delete data: Drop tables or truncate contest records
• Bypass authentication: Extract password hashes or manipulate user roles
• Escalate privileges: Modify their own user records to gain administrative access

The severity is amplified because this method specifically queries the contest_site table, which likely controls which contests are visible on which sites—a critical access control mechanism.

The Fix

The fix implements the standard defense against SQL injection: prepared statements with parameter binding. Here's the corrected code:

Before (Vulnerable):

public function getOfficialContests($site_id)
{
    $stmt = $this->pdo->query("SELECT * FROM contest, contest_site
        WHERE contest.defunct = 'O'
        AND contest_site.contest_id = contest.contest_id
        AND contest_site.site_id = $site_id
        ORDER BY contest.contest_id DESC");
    return $stmt->fetchAll(PDO::FETCH_ASSOC);
}

After (Secure):

public function getOfficialContests($site_id)
{
    $stmt = $this->pdo->prepare("SELECT * FROM contest, contest_site
        WHERE contest.defunct = 'O'
        AND contest_site.contest_id = contest.contest_id
        AND contest_site.site_id = :site_id
        ORDER BY contest.contest_id DESC");
    $stmt->execute([':site_id' => $site_id]);
    return $stmt->fetchAll(PDO::FETCH_ASSOC);
}

What Changed

Three specific changes were made to line 133, 136, and the addition of line 138:

1. Line 133: pdo->query() → pdo->prepare()
   - This tells PDO to prepare the SQL statement as a template rather than executing it immediately

2. Line 136: $site_id → :site_id
   - The direct variable interpolation is replaced with a named placeholder (:site_id)
   - The placeholder acts as a marker where the parameter value will be inserted

3. Line 138 (new): $stmt->execute([':site_id' => $site_id]);
   - The actual $site_id value is passed separately through the execute() method
   - PDO handles the value as pure data, never as SQL code

How This Prevents SQL Injection

When using prepared statements with parameter binding:

1. The SQL structure is sent to the database first (with placeholders)
2. The database compiles and optimizes the query structure
3. Parameter values are sent separately and treated exclusively as data
4. No matter what characters the $site_id contains (quotes, semicolons, SQL keywords), they're never interpreted as SQL commands

Even if an attacker passes 123 OR 1=1 --, the database treats the entire string as a literal value to compare against contest_site.site_id. The query would look for a site_id that exactly matches the string "123 OR 1=1 --" (which doesn't exist), rather than executing the injected SQL logic.

Key Takeaways

• The getOfficialContests() method in ContestRepository.php used direct string interpolation ($site_id) instead of parameter binding, creating a critical SQL injection vulnerability at line 136
• Switching from pdo->query() to pdo->prepare() with :site_id placeholder eliminated the vulnerability by separating SQL structure from data values
• Inconsistent security patterns are dangerous: while other methods in the same file correctly used prepared statements, this single method's shortcut created an exploitable weakness
• Input validation is not a substitute for prepared statements: even with validation, always use parameterized queries as the primary SQL injection defense
• Static analysis tools can catch these patterns early: implementing automated security scanning would have flagged this vulnerability before it reached production

How Orbis AppSec Detected This

• Source: The $site_id parameter passed to the getOfficialContests() method from untrusted input (likely HTTP request parameters)
• Sink: Direct variable interpolation in the SQL query string at line 136: AND contest_site.site_id = $site_id passed to pdo->query()
• Missing control: No prepared statement or parameter binding; the variable was directly interpolated into the SQL string instead of using a placeholder and execute()
• CWE: CWE-89 (Improper Neutralization of Special Elements used in an SQL Command)
• Fix: Replaced pdo->query() with pdo->prepare(), changed $site_id to :site_id placeholder, and added execute([':site_id' => $site_id]) to bind the parameter safely

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The SQL injection vulnerability in ContestRepository.php demonstrates how a single inconsistent security practice can create critical exposure, even in a codebase that otherwise follows secure patterns. By replacing direct string interpolation with prepared statements and parameter binding, the fix eliminates the vulnerability at its root cause. This case reinforces a fundamental principle: always use prepared statements for SQL queries with dynamic values, regardless of how simple the query appears or how much you trust the input source. The small effort of using prepare() and execute() instead of query() provides complete protection against one of the most dangerous web application vulnerabilities.