Loïc Faugeron Technical Blog

🤘 The Parameter Paladin storms forth from the OWASP bastion, raising the impenetrable shield of Prepared Statements, against the shadow army of malicious injections, that seek to corrupt the sacred data temples! 🔥

In this series, we're dealing with BisouLand, an eXtreme Legacy application (2005 LAMP spaghetti code base). So far, we have:

1. 🐋 got it to run in a local container
2. 💨 written Smoke Tests
3. 🎯 written End to End Tests
4. 🧹 created and applied Coding Standards

This means we can run it locally (http://localhost:8080/), and have some level of automated tests.

But it's currently riddled with deprecated calls to mysql_query(), so let's upgrade the codebase to PDO!

• SQL Injection vulnerabilities
• Docker
• Connection
• Queries
• Fixes
• Conclusion

SQL Injection vulnerabilities

Before we start, a note on security vulnerabilities.

The BisouLand SQL queries are full of input values directly concatenated in the SQL queries, like here:

// SignUp form: inscription.php
$result = mysql_query(
    'INSERT INTO membres (pseudo, mdp, confirmation, timestamp, lastconnect, amour)'
    ."VALUES ('{$pseudo}', '{$hmdp}', '1', ".time().', '.time().", '300')"
);

This would clearly be a SQL injection risk, as someone could submit the following username:

Eisenberg', '938c2cc0dcc05f2b68c4287040cfcf71', '1', 1759645024, 1759645024, '42000'); --

Which would create the user as expected, but with 42k Love Points instead of the expected 300:

INSERT INTO membres (pseudo, mdp, confirmation, timestamp, lastconnect, amour)
VALUES ('Eisenberg', '938c2cc0dcc05f2b68c4287040cfcf71', '1', 1759645024, 1759645024, '42000');
-- ', '938c2cc0dcc05f2b68c4287040cfcf71', '1', 1759645024, 1759645024, '300')

However looking a bit closer at the code, we can see that most of the time the user input is validated and sanitised, for example in the sign-up form the username:

• cannot be too long
• can only be alphanumerical characters
• addslashes() escapes ', ", \ and NULL bytes
• htmlentities() escapes HTML characters like <, >, '

Which effectively prevents SQL Injection:

// SignUp form: inscription.php
$pseudo = htmlentities(addslashes($_POST['Ipseudo']));
$mdp = htmlentities(addslashes($_POST['Imdp']));

$taille = strlen(trim($_POST['Ipseudo']));
if ($taille >= 4 && $taille <= 15) {
    // ...
    $result = mysql_query(
        'INSERT INTO membres (pseudo, mdp, confirmation, timestamp, lastconnect, amour)'
        ." VALUES ('{$pseudo}', '{$hmdp}', '1', ".time().', '.time().", '300')"
    );
}

Regardless, using PDO with prepared statements will make sure we don't accidentally introduce SQL vulnerabilities.

Docker

The first thing we're going to do is update the apps/monolith/Dockerfile, as we'll now need the PDO extension and its MySQL companion, instead of the MySQL solo extension:

# ...

# Install system dependencies and PHP extensions in single layer
RUN docker-php-ext-install pdo pdo_mysql \
    && a2enmod rewrite

# ...

This change will require us to update the container:

# in apps/monolith
make down
make build
make up

Connection

Then we have to take care of the mysql_pconnect, located in apps/monolith/phpincludes/bd.php:

<?php

include __DIR__.'/../config/parameters.php';

function bd_connect()
{
    mysql_pconnect(
        DATABASE_HOST.':'.DATABASE_PORT,
        DATABASE_USER,
        DATABASE_PASSWORD
    );
    mysql_select_db(DATABASE_NAME);
}

This creates a persistent connection, for the current HTTP request.

We're going to replace it with PDO (PHP Data Object, what a weird name), the out of the box Database Abstraction Layer:

<?php

include __DIR__.'/../config/parameters.php';

function bd_connect()
{
    static $pdo = null;

    if (null === $pdo) {
        $dsn = 'mysql:host='.DATABASE_HOST.';port='.DATABASE_PORT.';dbname='.DATABASE_NAME.';charset=utf8mb4';
        $options = [
            // Throw exceptions on error
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            // Return query results as associative arrays (column name => value)
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            // Use prepared statements at the database layer, for better security and performance
            PDO::ATTR_EMULATE_PREPARES => false,
        ];
        $pdo = new PDO($dsn, DATABASE_USER, DATABASE_PASSWORD, $options);
    }

    return $pdo;
}

We then need to change the usage of the function in the codebase:

• from bd_connect() to $pdo = bd_connect()

The bd_connect has been transformed into a Singleton, it will always return the same instance of PDO (for the duration of the HTTP Request).

I'd normally frown at this but this is a temporary measure: my intention is to later in the series introduce a Dependency Injection Container, and instead of having those $pdo = bd_connect() statements, we'll have $pdo = $container->get(PDO::class);.

Queries

It's then a game of finding and replacing all mysql_* functions:

• from mysql_query() to $pdo->prepare(); $pdo->execute() or $pdo->query()
• from mysql_fetch_assoc() to $pdo->fetch()
• from mysql_result() to $pdo->fetchColumn()
• from mysql_num_rows() to $pdo->rowCount() or $pdo->fetchColumn()
• from mysql_insert_id() to $pdo->lastInsertId()

For example in apps/monolith/web/phpincludes/inscription.php:

$result = mysql_query(
    'INSERT INTO membres (pseudo, mdp, confirmation, timestamp, lastconnect, amour)'
    ."VALUES ('{$pseudo}', '{$hmdp}', '1', ".time().', '.time().", '300')"
);
if (false === $result) {
    echo 'Error: '.mysql_error();
}
$id = mysql_insert_id();

Becomes:

$stmt = $pdo->prepare(
    'INSERT INTO membres (pseudo, mdp, confirmation, timestamp, lastconnect, amour)'
    .' VALUES (:pseudo, :mdp, :confirmation, :timestamp, :lastconnect, :amour)'
);
$stmt->execute([
    'pseudo' => $pseudo,
    'mdp' => $hmdp,
    'confirmation' => 1,
    'timestamp' => time(),
    'lastconnect' => time(),
    'amour' => 300,
]);
$id = $pdo->lastInsertId();

There were 25 PHP files with ~250+ of such calls, by the way.

And before we forget, with prepared statements we no longer need calls to addslashes(), so we can also removed them.

Fixes

We shouldn't stop there. There's an issue we've encountered in a previous article, where the code was using '' for the id column in INSERT statements.

This was relying on string to integer conversion, which MySQL supported until version 5.7.

After a quick look, I've found 5 more instances of these, which I've now fixed.

And last but not least, still on the topic of string to integer conversion, there are other integer fields that are receiving strings:

1. web/phpincludes/inscription.php:
   • '1' is used for the confirmation field, should be 1 (TINYINT field)
   • '300' is used for the amour field, should be 300 (INT field)
2. web/phpincludes/lire.php: '1' is used for status (INT field)
3. web/phpincludes/attaque.php: '0' is used for finaller (INT field)
4. web/news/liste_news.php: '0' is used for timestamp_modification (INT field)
5. web/phpincludes/cerveau.php: '1' is used for confirmation (TINYINT field)
6. web/phpincludes/membres.php: '1' is used for confirmation (TINYINT field)

Conclusion

This has been a lot of effort, and there's seemingly not a lot to show for it.

But by switching from the MySQL extension to PDO, we've unlocked something amazing: we will be able to upgrade to the latest version of PHP, which will provide a big performance boost as well as many security patches.

And we will be able to switch to PostgreSQL!

⁉️ What do you mean, "PHP is slow and we should use go / rust"?