5
\$\begingroup\$

Is this code well protected, and if not, could you tell me how it might be exploited and how to secure it?

if(isset($_POST['vrsta_predmeta']) AND !empty($_POST['vrsta_predmeta']) AND 

isset($_POST['res_text']) AND isset($_POST['glavni_dug']) AND isset($_POST['res']) AND isset($_POST['zaklj']) AND isset($_POST['povjerilac']) AND isset($_POST['duznik']) AND isset($_POST['predmet_zaveden'])){


$racunob =  trim($_POST['rac']);
$obrazlozenje = trim($_POST['obr']);     
$ob_text = trim($_POST['res_ob']);  
$res_text = trim($_POST['res_text']);
$vrsta_pre = trim($_POST['vrsta_predmeta']);
$izvrsenje = trim(strtolower($_POST['res']));
$obrazac = trim($_POST['zaklj']);
$povjerilac = $_POST['povjerilac'];
$duznik = $_POST['duznik'];
$datum= trim($_POST['predmet_zaveden']);
 foreach($povjerilac as $key){
 $lica = $db -> prepare("INSERT INTO p_lica(povjerilac, doc_br, dokument_vlasnik) VALUES('$key', '$dok_broj', '$ses_val')");



}       


foreach($duznik as $key1){
 $lica1 = $db -> prepare("INSERT INTO d_lica(duznik,doc_br, dokument_vlasnik) VALUES('$key1', '$dok_broj', '$ses_val')");


}

$insert_dok = $db -> prepare("INSERT INTO document_tbl(dokument_vlasnik,dokument_broj,vrsta_dokumenta,zakljucak, resenje_izvrsenja,datum,resenje_text,obrazlozenje,obtext,racunob) VALUES('$ses_val','$dok_broj', '$vrsta_pre','$obrazac','$izvrsenje','$datum','$res_text','$obrazlozenje','$ob_text','$racunob')");
if($lica -> execute() AND $insert_dok -> execute() AND $lica1 -> execute()){


   $lica -> close();
   $lica1 -> close();
   $insert_dok -> close();


  echo '<script>new Messi(\'Dokument uspjesno dodat.\', {title: \'Obavjestenje\', titleClass: \'success\', buttons: [{id: 0, label: \'Close\', val: \'X\'}]});</script>';
header('location:login.php');
  }else{
   echo '<script>new Messi(\'Dokument uspjesno dodat.\', {title: \'Obavjestenje\', titleClass: \'anim warning\', buttons: [{id: 0, label: \'Close\', val: \'X\'}]});</script>'; 
}

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

No, this code is not secure, as you are using prepared statements wrong. See here for correct usage of mysqli prepared statements (you should not ever have user supplied input in the prepare statement, it is only for SQL syntax; add the user input via bind later on).

For example, your code might look like this:

$sql = $db->prepare('INSERT INTO p_lica (povjerilac, doc_br, dokument_vlasnik) VALUES(?, ?, ?)'); // note that there is no user input in prepare
$sql->bind_param('iss', $key, $dok_broj, $ses_val); // s for string, i for integer. use d for double, b for blob

foreach($povjerilac as $key) { // set values for $key, which is bound to the first ? parameter     
    $sql->execute(); // insert new row with current key
}
$sql->close();

(see also here or here)

Misc

  • formatting: your code is very badly formatted. If this is just a copy-paste error it's ok, but otherwise, you should definitely fix it.
  • variable names: the standard is to use english names, if you can.
  • your header call seems wrong. From the documentation: Remember that header() must be called before any actual output is sent. You should also use a complete URL.
\$\endgroup\$
  • \$\begingroup\$ Ok, thanks for response. If I won't use bind_param how then code should look like? \$\endgroup\$ – Vladimir Oct 27 '14 at 15:12
  • \$\begingroup\$ @Vladimir: Like a blank sheet of paper: if you're not going to use prepared statements, then don't INSERT user data. Use prepared statements or don't query. Full stop. \$\endgroup\$ – Elias Van Ootegem Oct 27 '14 at 15:23
  • \$\begingroup\$ @Vladimir why wouldn't you use bind_param? You should. I added an example. \$\endgroup\$ – tim Oct 27 '14 at 15:25
  • \$\begingroup\$ I tought it's secured also without using bind_param. Shuold I use bind_param for all variables or I can use just for few? Also sould I use bind_param with select and update or what? \$\endgroup\$ – Vladimir Oct 27 '14 at 15:28
  • \$\begingroup\$ @Vladimir like I said in my answer, you shouldn't ever have user input in prepare. With that in mind, you don't have all that many other options. And you should use bind for all variables, and also for select and update. \$\endgroup\$ – tim Oct 27 '14 at 15:32
2
\$\begingroup\$

Use PDO to secure your SQL, it's built in the default PHP and work with 12 database types. it also makes it a lot easier when it comes to making prepared statements.

PDO:

$params = array(
    ':username' => 'test', 
    ':email' => $mail, 
    ':last_login' => time() - 3600
);

$pdo->prepare('
    SELECT * FROM users
    WHERE username = :username
    AND email = :email
    AND last_login > :last_login');

$pdo->execute($params);

MySQLi:

$query = $mysqli->prepare('
    SELECT * FROM users
    WHERE username = ?
    AND email = ?
    AND last_login > ?');

$query->bind_param('sss', 'test', $mail, time() - 3600);
$query->execute();

As you can see, A big bonus is that you can use :name instead of ?


Here some tutorial about PDO

W3Schools tutorial
Tuts+ Tutorial (Recommended)

\$\endgroup\$

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.