2
\$\begingroup\$

I have been developing a little, private Blog Site to make notes/ stories of pen and paper rpg games available for me and my players.

This is my first project. As I am new to coding in PHP and MySQL I'd very much appreciate your opinions on the code I have written. I am interested in any feedback you can give me - but especially on the security of the script I have written. The input form is actually secured by a randomly created username and password authentication via .htaccess. (Side note: the website uses proper encryption for the user input including HSTS and ranks as an A+ on ssllabs.com.)

I have been reading much lately on script injection and SQL injection (for example on https://phpdelusions.net/pdo/sql_injection_example) and using variables to create a query was often considered unsafe. Still - in my inexperiencedness - I've made a script thats using the (static) keys of the $_POST Array to determine the database columns and placeholders for values which are used in my PDO query. Afterwards I bind each submited value to its PDO placeholder.

I do not expect to enter any html Input, which is why I decided to use the stripslashes and htmlspecialchars funcions on the input values before binding it to the PDO querys and I haven't used any str_replace function to escape backtics since every value is directly bound to the designated PDO placeholder.

if  (isset ($_POST['session_submit'])) {
    $table = 'sessions';

    foreach ($_POST as $column=>$value){
        $column = input_behandlung_sessions($column);
        $secure_value = input_behandlung_sessions($value);
        $formular_daten[$column] = $secure_value;
    }   

    $kampagne = $formular_daten['kampagne'];    
    $count = $pdo->prepare('SELECT COUNT(kampagne) FROM sessions WHERE kampagne = :kampagne');
    $count->bindParam(':kampagne', $kampagne, PDO::PARAM_STR);
    $count->execute();
    $rowcount = $count->fetchColumn(0);
    $newrowcount = ++$rowcount;     

    unset($formular_daten['session_submit']);

    foreach ($formular_daten as $column=>$value){
        if ($value == "") unset($formular_daten[$column]);
    }

    foreach ($formular_daten as $column=>$value){
        $non_empty_columns[] = $column;
        $columns_param = ':'.$column;
        $array_columns_param[] = $columns_param;
        $to_bind[$columns_param] = $value;
    }

    $text_non_empty_columns = implode (", ", $non_empty_columns);
    $parameter_columns = implode (', ', $array_columns_param);  

    $sql_string = 'INSERT INTO '.$table.' (session_uid, '.$text_non_empty_columns.') VALUES ('.$newrowcount.', '.$parameter_columns.')';    
    $stmt = $pdo->prepare($sql_string);
    foreach ($to_bind as $key=>$param){     
        $stmt->bindValue($key, $param);
    }
    $stmt->execute();

    unset($_POST);

}   

function input_behandlung_sessions($data) {
  $data = stripslashes($data);
  $data = htmlspecialchars($data);
  return $data;
}

And here is the form used to submit the data (in case you are interested).

echo '
<form method="post" action="#v-pills-session">
      <div class="row">
          <div class="form-group col-md-2 col-sm-6"><label for="formGroupExampleInput">Kampagne: </label></div>
          <div class="col-md-4 col-sm-6"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="kampagne" placeholder="Kürzel der Kampagne eintragen"></div> 
          <div class="form-group col-md-2 col-sm-6"><label for="formGroupExampleInput">Kapitel-Nr.: </label></div>
          <div class="col-md-4 col-sm-6"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="session_story_arc" placeholder="Nummer des Kapitels eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput">Name: </label></div>
          <div class="col-md-11 col-sm-12"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="session_name" placeholder="Hier den Namen der Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"<label for="formGroupExampleInput2">Spieler: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" required="true" class="form-control" id="formGroupExampleInput2" name="session_charaktere_aktiv" placeholder="Hier die Namen der aktiven Charaktere eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">Quests: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" id="formGroupExampleInput2" name="session_quests" placeholder="Hier die Quests in dieser Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">NPCs</label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" required="true" id="formGroupExampleInput2" name="session_npc" placeholder="Hier die Namen der NPCs aus dieser Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">Orte: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" required="true" id="formGroupExampleInput2" name="session_orte" placeholder="Hier die Orte eintragen, in welchen die Abenteure unterwegs waren"></div>
      </div>
      <div class="form-group">
          <input required="true" type="submit" name="session_submit" class="form-control btn-primary" id="formGroupExampleInput2" placeholder="Submit">
      </div>
</form>';
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Nov 7 '18 at 17:01
  • \$\begingroup\$ Thank you for your hint towards the title - I hope it's better now, If not please leave me another comment! \$\endgroup\$ – chainsaw42 Nov 7 '18 at 17:12
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 7 '18 at 17:37
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I hope that I do not violate another rule by answering to your comment but still - I'm sorry, I clearly seem to have lost track of some of the rules, will do better with future questions! \$\endgroup\$ – chainsaw42 Nov 7 '18 at 17:51
  • \$\begingroup\$ I've actually written something similar but I use SHOW COLUMNS FROM $table to generate the schema from the DB and then filter the post array by that. It gets a bit tricky when dealing with multiple tables though. \$\endgroup\$ – ArtisticPhoenix Nov 8 '18 at 19:36
0
\$\begingroup\$

Let's look at that second loop:

foreach ($formular_daten as $column=>$value){
    if ($value == "") unset($formular_daten[$column]);
}

Some argue it is bad practice to omit the curly brackets, since anyone who needs to add a second statement when the condition is true might forget that there isn't a block here.

Additionally, array_filter() could be used to replace the entire loop:

$formular_daten = array_filter( $formular_daten , 'strlen' );

(credit goes to the post by anonymous)

Additionally, instead of adding values to $formular_daten in the first foreach and then having another foreach loop to unset any value that is equal to an empty string, you could just only add values to the array that are not equal to an empty string...

foreach ($_POST as $column=>$value){
    $column = input_behandlung_sessions($column);
    $secure_value = input_behandlung_sessions($value);
    if ($secure_value) { // equivalent to !== ""
        $formular_daten[$column] = $secure_value;
    }
}  

And the unset call for the session_submit could be removed by not setting it in the first loop...

unset($formular_daten['session_submit']);

So if the modified foreach above is used, that unset could be eliminated by updating the condition for setting the array value:

if ($secure_value && $column !== 'session_submit') {
    $formular_daten[$column] = $secure_value;
}

While it appears that you would need to have that loop before the check for count based on the value at $POST['kampagne], you could potentially move the code from that last foreach up into the first foreach loop, which would require looping through the data only once...


You could consider using ? placeholders instead of named placeholders - see Example 2 on the PDO prepare documentation (and compare to the example above it).


Update

I would like to speak to what @Your Common Sense mentioned:

I don't know why you call keys of the $_POST Array "static" (and what does it mean at all)

Not only can be $_POST be modified within the PHP script (because it is a superglobal variable - not a Superglobal constant), but your script can be called by techniques other than submitting the form - including but not limited to:

  • making a separate page that has a <form> tag with an action= attribute matching the URL corresponding to your script
  • making an XMLHttpRequest/AJAX request matching the URL corresponding to your script
  • making requests with tools like Postman

And any of those means can be used to send various values in the POST data.

\$\endgroup\$
3
\$\begingroup\$

I don't know why you call keys of the $_POST Array "static" (and what does it mean at all) but the article you linked to (which I wrote) explicitly demonstrates that $_POST Array keys are unsafe and using them in the query untreated makes your code wide open to SQL injection.

And no, your input_behandlung_sessions() is not the proper treatment. It makes anything but a secure value. Calling its output "secure" is a mockery: htmlspecialchars() has nothing to do with SQL injections, while stripslashes() is just a pointless function that does absolutely nothing useful at all.

As a result, your code is exemplary vulnerable.

You must use the solution offered in the article - a white list of predefined field names. So your code regarding the INSERT query must be something like this:

$formular_daten = []; // variables must be defined before use
$allowed = ['session_story_arc', 'session_name']; //add other fields

$fieldnames = "`session_uid`"; // a list of column names
$values = [$newrowcount]; // an array with values

foreach ($allowed as $column) { // allowed fields ONLY!
    if (!empty($_POST[$column])) { // if a column exists and not empty in a single line
        $fieldnames .= ", `$column`";
        $values[] = $_POST[$column];
    }
}
$parameter_columns = str_repeat('?,', count($values) - 1) . '?';

$sql_string = "INSERT INTO `$table` ($fieldnames) VALUES ($parameter_columns)";
$stmt = $pdo->prepare($sql_string);
$stmt->execute($values);

the code in the end is not only safe but also uses less loops to process.

On a side note, your method to get a session_uid is extremely clumsy and error prone. Depending on the purpose of this field, either make it integer auto_increment or some unpredictable unique value like one generated by session_create_id()

\$\endgroup\$
0
\$\begingroup\$

When you expect get reviewed from poeple speaking English, try to use only English in your code (and in comments).

Why go through formular_daten twice when you can check if the value is empty and, if not do the processing in a single loop?

\$\endgroup\$
  • \$\begingroup\$ I changed the variables to english and deleted the forms since they were half german and I do not expect them to be relevant. Additionally I've merged the two loops like you suggested - thank you very much for your answer! \$\endgroup\$ – chainsaw42 Nov 7 '18 at 17:36
0
\$\begingroup\$

Considering the suggestions from @Sam Onela and the devastating comment of @Your Common Sense I re-read the article on SQL-Injection and looked up some basic examples on YouTube to understand the importance of the code you provided (like html-form-exploits and sql-injection). I recommend this to everyone else new to this.

The confusion regarding session_uid is understandable. The purpose of this was to be able to sort sessions of different campagins in chronological order and to be able to link quests to sessions. Therefore I changed the field to session_date and the format to "date". The quests will be linked to the session_id (which was not shown in the code, as it is an int, auto_increment column).

I changed the code suggested by @Your Common Sense a bit (a little closer to the example in the article listed in my question) since I had an easier time troubleshooting errors in the PDO statement. Thank you very much for all your input - here's the updated, hopefully better, code:

PS: Could someone point out to me why the backtics in the keys are still being escaped after specifying $allowed keys (like in the solution from phpdelusion)?

if  (isset ($_POST['session_submit'])) {
$table = 'sessions';

$allowed = ["session_story_arc", "session_name", "session_date", "session_charaktere_aktiv", "session_quests", "session_npc", "session_orte", "kampagne"];
$pdo_str_column = $pdo_str_values = $pdo_str_value = "";
$params = [];

foreach ($allowed as $key) {
    if (($_POST[$key]) != ''){
        $pdo_str_column .= "`".str_replace("`", "``", $key)."`, ";
        $pdo_str_value = ":".str_replace("`", "``", $key);
        $pdo_str_values .= ":".str_replace("`", "``", $key).", ";
        $params[$pdo_str_value] = $_POST[$key];
    }
}

$pdo_str_column = rtrim ($pdo_str_column, ", ");  
$pdo_str_values = rtrim ($pdo_str_values, ", "); 

$sql_string = 'INSERT INTO '.$table.' ('.$pdo_str_column.') VALUES ('.$pdo_str_values.')';

$stmt = $pdo->prepare($sql_string);
foreach ($params as $key=>$value) {
        $stmt->bindValue($key, $value);
}
$stmt->execute();

unset($_POST);
unset($params);

}   
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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