Improving upon the Security of my Export Remote Database to Local PHP API

Question

I am working on an in-house application, and I am working on how our application communicates with our remote server.

I have a PHP script that acts as an API to call to our MySQL DB and dump its contents.

I've been doing research on best security practices for this type of scenario, and I believe I've hit the key points. Do you have any insight on what could further improve the security?

<?php
$dbUser = 'redacted';
$dbPass = 'redacted';
$dbName = 'redacted';
if (empty($_SERVER['HTTPS']) || $_SERVER['HTTPS'] === 'off') {
 http_response_code(403);
 echo "Secure connection required.";
 exit;
}
if ($_SERVER['REQUEST_METHOD'] !== 'GET') {
 http_response_code(405); // Disallow
 echo "Invalid request method.";
 exit;
}
$secretToken = "redacted";
$headers = getallheaders();
$clientToken = isset($headers['X-API-KEY']) ? $headers['X-API-KEY'] : '';
if (!hash_equals($secretToken, $clientToken)) {
 error_log("Unauthorized API access attempt from " . $_SERVER['REMOTE_ADDR'] . " at " . date("Y-m-d H:i:s"));
 http_response_code(401);
 echo "Unauthorized.";
 exit;
}
$tempFile = tempnam(sys_get_temp_dir(), 'dump_');
if ($tempFile === false) {
 http_response_code(500);
 echo "Error: Could not create temporary file.";
 exit;
}
$tables = [
 'users',
 'tables redacted'
];
$escapedTables = array_map('escapeshellarg', $tables); // Extra layer of security even though everything is predefined
$tableList = implode(' ', $escapedTables);
$escapedDbUser = escapeshellarg($dbUser);
$escapedDbPass = escapeshellarg($dbPass);
$escapedDbName = escapeshellarg($dbName);
$escapedTempFile = escapeshellarg($tempFile);
$cmd = "mysqldump --compact --skip-comments -u{$dbUser} -p{$dbPass} {$dbName} {$tableList} > {$escapedTempFile}";
exec($cmd, $output, $returnVar);
if ($returnVar !== 0) {
 http_response_code(500);
 echo "Error: Could not generate mysqldump.";
 unlink($tempFile);
 exit;
}
$dumpData = file_get_contents($tempFile);
if ($dumpData === false) {
 http_response_code(500);
 echo "Error: Could not read dump file.";
 unlink($tempFile);
 exit;
}
// Encrypt response even if valid request
$method = 'aes-256-cbc';
$key = hash('sha256', $secretToken, true);
$ivLength = openssl_cipher_iv_length($method);
$iv = openssl_random_pseudo_bytes($ivLength);
if ($iv === false) {
 http_response_code(500);
 echo "Error: Failed to generate IV.";
 unlink($tempFile);
 exit;
}
$encryptedData = openssl_encrypt($dumpData, $method, $key, OPENSSL_RAW_DATA, $iv);
if ($encryptedData === false) {
 http_response_code(500);
 echo "Error: Encryption failed.";
 unlink($tempFile);
 exit;
}
$finalData = $iv . $encryptedData;
unlink($tempFile);
header('Content-Type: application/octet-stream');
header('Content-Disposition: attachment; filename="remote_dump.enc"');
header('Cache-Control: no-store, no-cache, must-revalidate');
header('Pragma: no-cache');
echo $finalData;
exit;
?>

Answer 1

passwords don't belong in a source code repo

$dbPass = 'redacted';

Read it from a DB_PASS env var, from a password vault, or even from a config.txt file that isn't checked in. Do be sure to rev the password, so the one in your git history no longer matters for DB security.

Similarly for the secret token shared with the web client.

The fixed-time hash_equals() comparison looks good.

shell escapes

The escapeshellarg() is probably fine, but it frankly makes me nervous. I would prefer to verify that all args are "simple", e.g. matching a \w+ regex, and do not require quoting. A typical pitfall in shopworn code is some text might accidentally go through two levels of unquoting, and hence need to be escaped twice. It's the sort of thing that automated unit tests need to be very careful about exercising.

$escapedDbPass = escapeshellarg($dbPass);
...
$cmd = "mysqldump --compact --skip-comments -u{$dbUser} -p{$dbPass} {$dbName} {$tableList} > {$escapedTempFile}";

I don't understand what's going on there.

I guess you require the password shall not contain any SPACE characters? But you didn't write that down and the code does not e.g. check with a regex. Didn't you want to put escaped values into $cmd? But that brings us back to details like, we probably prohibit username from containing a SPACE or a quote char, so escaping likely isn't relevant for that.

We're not using shell_exec(), so that's good, we're keeping things relatively simple

exec($cmd, $output, $returnVar);

That call can return False if it fails, but regrettably we ignore that. For example $PATH might omit an executable mysqldump.

cleanup handler

$tempFile = tempnam(sys_get_temp_dir(), 'dump_');
 ...
 unlink($tempFile);

Having created the temp file, now you need to carefully remember to unlink it at the end, which seems error prone. There are multiple calls to unlink(). Some poor maintenance engineer will come along months later, add some code, and forget to unlink.

Consider wrapping some of this code in try / catch / finally, so there's just a single unlink() call at the end. An alternate approach would add __destruct() to a temp file manager class, for similar effect.

Generally speaking, this function is Too Long. Break out helper methods, such as validating that we have a well-formed authorized request.

memory footprint

$dumpData = file_get_contents($tempFile);

Wow, that seems surprising! Wouldn't you rather have a mysqldump | encrypt shell pipeline, and make this PHP script responsible for repeatedly reading a small (perhaps 100 KiB) binary buffer and sending crypted contents to the web client?

• KIKO Software: Nowhere did Cory Green say that git was used, so that's a big assumption. All we know is that it is an "in-house" application. Just mentioning a git repository as a reason why you suggest this "best practice" would probably be better.
• Cory Green: Great thorough examination, thank you. In production we will definitely be using .env variables for loading credentials. The API key being used as the encryption cipher as a hardcoded string will change.

Answer 2

I only have a few things to add to J_H's answer:

Do not repeat yourself

You error out of the code eight times, and each time you write:

http_response_code(xxx);
echo "Error message";
exit;

This could be a function, so you don't need to repeat these three lines:

function fatalError($message, $responseCode = 500)
{ 
 http_response_code($responseCode);
 echo $message;
 exit;
}

This is a script with a single purpose, so not using functions or classes can be justified, but here I think you break the DRY principle.

Encrypting twice

Sending your whole database in response to one fixed header token doesn't seem a safe thing to do, but that's what you want to do, so who are we to criticize that? After creating this obvious security risk, it is a bit weird that you encrypt the response twice. First your code insists on using HTTPS for encrypted communication and then you use OpenSSL in your code to encrypt the output again.

In a bad way, I should add. You use the same token, for the encryption, as was used to authorize the API access. Remember, this token was supplied by the user. Not even an experienced hacker would except that, so it's obscure, but it's not safe.

If you do want to use double encryption, do it in a safe way, with a token for the passphrase that is not sent by the user. This is called: safety by design. Also remember that using tokens, that are always the same, is not very safe. They can be compromised by a single leak.

temporary file

I think the whole idea of storing the database dump in a temporary file is flawed. You basically create a copy of the database in a place that accessible by anybody with access to the server. That can't be good. Since all you do is read the file into memory, you didn't actually need to store it there anyway.

What makes it even worse is that you already have the $ouput variable in your exec(), but you don't use it. So change it to:

$cmd = "mysqldump --compact --skip-comments -u{$dbUser} -p{$dbPass} {$dbName} {$tableList}";
exec($cmd, $dumpData, $returnVar);

and you don't need the temporary file anymore.

Code corrections

• You use escapeshellarg(), but you never actually use the variables you create with it. I don't think that's what you intended.

• The line: $clientToken = isset($headers['X-API-KEY']) ? $headers['X-API-KEY'] : '';, can be rewritten as: $clientToken = $headers['X-API-KEY'] ?? '';, which looks a lot cleaner. See: Null Coalescing Operator

Final thoughts

Writing code that is truly secure is difficult, as you can see from my remarks above. The devil is in the details. There are some obvious mistakes in your code, signifying that you're not very experienced. Should you be writing this code?

More so, what you do is insecure by nature. Do you really have to do this? Perhaps you can add some additional features, such as only allowing access from certain IP addresses?

For more security lessons have a look here: https://www.hacksplaining.com/lessons