1
\$\begingroup\$

Could someone review this PHP code to see if my pages are coded safely to not allow common attacks (SQL injection, XSS, etc.)? I would be thankful, of course, for anyone to point out other problems in my code as well.

// getpaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini'); 
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Prepare sql and bind parameters.
$stmt = $conn->prepare("SELECT key_salt, aes_iv, cipher_text, mac_tag FROM pastes WHERE paste_id =:pasteid");
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->execute();

// Only output results if 1 row is found.
if ($stmt->rowCount() == 1)
{
    $row = $stmt->fetch();
    echo $row['key_salt'];
    echo "|";
    echo $row['aes_iv'];
    echo "|";
    echo $row['cipher_text'];
    echo "|";
    echo $row['mac_tag'];
}

$conn = null;
?>

// savepaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini');
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Make sure that the values written to the database only include valid base64 characters.
// These are letters (upper and lower case), numbers, and /+=
$pattern = "@^[a-zA-Z0-9/+=]+$@";
if ((!preg_match($pattern, $_POST["pasteID"])) or 
    (!preg_match($pattern, $_POST["keySalt"])) or
    (!preg_match($pattern, $_POST["aesIV"])) or 
    (!preg_match($pattern, $_POST["cipherText"])) or
    (!preg_match($pattern, $_POST["macTag"])))
{
    echo "Invalid input characters.";
    exit(1);
}

// Depending on action variable, either do an insert or update.
if ($_POST["action"] == "insert")
{
    $pre_stmt = "INSERT INTO pastes VALUES (:pasteid, :keysalt, :aesiv, :ciphertext, :mactag)";
}
else if ($_POST["action"] == "update")
{
    $pre_stmt = "UPDATE pastes SET key_salt=:keysalt,aes_iv=:aesiv,cipher_text=:ciphertext,mac_tag=:mactag WHERE paste_id=:pasteid";
}
else
{
    echo "Invalid form action.";
    exit(1);
}

// Prepare sql and bind parameters.
$stmt = $conn->prepare($pre_stmt);
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->bindParam(':keysalt', $_POST["keySalt"]);
$stmt->bindParam(':aesiv', $_POST["aesIV"]);
$stmt->bindParam(':ciphertext', $_POST["cipherText"]);
$stmt->bindParam(':mactag', $_POST["macTag"]);
$stmt->execute();
$count = $stmt->rowCount();
echo $count;

$conn = null;
?>

// deletepaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini'); 
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Prepare sql and bind parameters.
$stmt = $conn->prepare("DELETE FROM pastes WHERE paste_id = :pasteid");
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->execute();
$count = $stmt->rowCount();
echo $count;

$conn = null;
?> 
\$\endgroup\$
2
\$\begingroup\$

Prepared statements prevent any form of SQL injection so that is not an issue with your code.

Although it may not be required in your case, I see no user control which would prevent anyone from filling your database with junk, or even deleting all records from it.

As for XSS, the code provided shows no protection from it in any way. XSS protection implies data validation, sanitizing, and output escaping but none of those are done here. You should refer to http://www.sitepoint.com/php-security-cross-site-scripting-attacks-xss/ or other similar tutorials to learn more on XSS protection.

Even if XSS wasn't a source of worry, you should never accept user input as is without extensive validation.

Another thing I noticed is that you're repeating the same code in all your scripts for the configuration. For maintainability, that greatly justifies placing it in another script that gets included by them such as config.php which would include:

$db_conn_config = parse_ini_file('../private/db_conn.ini'); 
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
\$\endgroup\$
  • \$\begingroup\$ Julie, thanks. Regarding XSS, did you see the code in savepaste.php that does a regx check on the user input before saving it to the database? It only allows a-zA-Z0-9/+= so wouldn't this prevent someone from putting malicious data (such as JavaScript code) into the database that another user might then retrieve when calling getpaste.php? \$\endgroup\$ – Ralph P Apr 30 '16 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.