2
\$\begingroup\$

I have this code and I would like to know if I filtered my code correctly. I am practicing my security coding for a system that I am working on and I would like to know if I am doing it the right way.

If there is any thing that I can improve to get things more solid I would very much like to know about them.

<?php
require_once 'app/helpers.php';
session_start();
$error = '';


if($_POST){



 $itemtype = filter_input(INPUT_POST, 'itemtype', FILTER_SANITIZE_STRING);
 $itemtype = trim($itemtype);
    $display = filter_input(INPUT_POST, 'itemdisplay', FILTER_SANITIZE_STRING);
    $display = trim($display);
    $brand = filter_input(INPUT_POST, 'brand', FILTER_SANITIZE_STRING);
$brand = trim($brand);
    $model = filter_input(INPUT_POST, 'model', FILTER_SANITIZE_STRING);
    $model = trim($model);
    $spec = filter_input(INPUT_POST, 'spec', FILTER_SANITIZE_STRING);
    $spec = trim($spec);
    $sn = filter_input(INPUT_POST, 'sn', FILTER_SANITIZE_STRING);
     $sn = trim($sn);
    $setname =  filter_input(INPUT_POST, 'setname', FILTER_SANITIZE_STRING);
    $setname = trim($setname);
    $itemstat =  filter_input(INPUT_POST, 'itemstat', FILTER_SANITIZE_STRING);
    $itemstat = trim($itemstat);



    if(empty($itemtype)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> תכניס את הפריט לקבוצה לא יפה! </div>';
    }elseif (empty($display)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> אם לא נציג ניתן לו שם איך יקחו אותו? </div>';
     }elseif (empty($brand)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> סליחה... מי יצר את הפריט? </div>';
    }elseif (empty($model)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> רגע...איזה דגם זה? </div>';
    }elseif (empty($spec)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> לא מגיע שתכתוב עליו כמה מילים? </div>';
    }elseif (empty($sn)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> מספר סידורי זה הכי אחי (ושלא יהיה אותו דבר כמו של פריט אחר...לא נעים..) </div>';
     }elseif (empty($setname)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>  אני חייב להיות בזוגיות...מה שם הסט שלי? </div>';
   }elseif (empty($itemstat)){
        $error = '<div class="alert alert-danger alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>    לאחרונה סיימתי קשר רציני... מה הסטטוס שלי? </div>';
    }else{
        if(!empty(filter_input(INPUT_POST, 'name', FILTER_SANITIZE_STRING)) || !empty(filter_input(INPUT_POST, 'email', FILTER_SANITIZE_STRING)) || !empty($_FILES['file']['name'])) {

            $uploadedFile = '';
            if (!empty($_FILES["file"]["type"])) {
                $fileName = $_FILES['file']['name'];
                $valid_extensions = array("jpeg", "jpg", "png");
                $temporary = explode(".", $_FILES["file"]["name"]);
                $file_extension = end($temporary);
                if ((($_FILES["file"]["type"] == "image/jpg") || ($_FILES["file"]["type"] == "image/jpeg")) && in_array($file_extension, $valid_extensions)) {
                    $sourcePath = $_FILES['file']['tmp_name'];
                    $targetPath = "items-img/" . $fileName;
                    if (move_uploaded_file($sourcePath, $targetPath)) {

                        $uploadedFile = $fileName ;

                    }
                }
            }

        }
      $stm = $link -> prepare("INSERT INTO item (item_desc,display,brand,model,spec,sn,set_name,status,item_pic) VALUES ('$itemtype','$display','$brand','$model','$spec','$sn','$setname','$itemstat','$uploadedFile')");

$stm->execute(array('item_desc' => $itemtype , 'display' => $display ,'brand' => $brand ,'model' => $model ,'item_desc' => $itemtype ,'spec' => $spec ,
    'sn' => $sn ,'set_name' => $setname ,'status' => $itemstat ,'name' => $uploadedFile ));

        $error = '<div class="alert alert-success alert-dismissable">
                    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>    יש לנו פריט חדש! </div>';


}
}
?>


<div>
    <?= $error ?>

</div>
\$\endgroup\$
  • 1
    \$\begingroup\$ Please tell us more about your code. The description doesn't reveal much yet. \$\endgroup\$ – t3chb0t Dec 19 '18 at 11:29
0
\$\begingroup\$

Instead of calling filter_input then trim combine them into a single statement.

$itemtype = trim(filter_input(INPUT_POST, 'itemtype', FILTER_SANITIZE_STRING));
\$\endgroup\$
  • \$\begingroup\$ Could you provide a scenario when to just check the extension is not enough? \$\endgroup\$ – Your Common Sense Dec 19 '18 at 12:41
  • \$\begingroup\$ Take a PHP file or JS file and rename to .jpg or .png or anything else. \$\endgroup\$ – Dave Dec 19 '18 at 12:43
  • \$\begingroup\$ and...? Ok I renamed it, and what next? \$\endgroup\$ – Your Common Sense Dec 19 '18 at 12:44
  • \$\begingroup\$ Nevermind, removed that portion of my answer .... though I still think it's a very good sanity/double check. \$\endgroup\$ – Dave Dec 19 '18 at 12:48
  • 2
    \$\begingroup\$ It is but it is very easy to fool fileinfo as well, a file could be of several types at once, serving as a valid image and no less valid PHP file at the same time. The question is whether you'll be able to execute the file, not what are first dozen bytes which fileinfo sniffs. \$\endgroup\$ – Your Common Sense Dec 19 '18 at 12:58
0
\$\begingroup\$

Your code is vulnerable, at least to SQL injection, Unrestricted File Upload, and Full Path Disclosure. For example, they can be exploited as follows:

  • SQL Injection: file.name='(SQL injection).jpg

  • Full Path Disclosure: file.name=a:b.jpg, file.name=%long_string%.jpg or itemtype=\

  • Unrestricted File Upload means that intruder can upload a PHP file with the “.jpg” extension, which is a “great” gift together with a LFI. Also, I would not trust file names from user and save them as is.

Some additional thoughts about your code:

• SQL injection as well as Full Path Disclosure are result of improper use of prepared statements. To fix this, use bind methods or the following:

$stm = $link->prepare("INSERT INTO item (item_desc, display, brand, ...) VALUES (?, ?, ?, ...)");
$stm->execute([$itemtype, $display, $brand, ...]);

• You can never upload “.png” files or images that have uppercase extensions.

filter_input(INPUT_POST, $var, FILTER_SANITIZE_STRING) is designed to sanitize HTML strings, not MySQL queries. For example, it will not escape characters such as “%”, “_” or “\”.

• The main rule of programming — “do not repeat yourself”. You can significantly improve your code by replacing all variable definitions and conditions:

$vars = [
    'itemtype' => 'Error 1',
    'itemdisplay' => 'Error 2',
    'brand' => 'Error 3',
    /* ... */
];

$error = '';
$values = [];
foreach ($vars as $varname => $varmsg) {
    $value = trim(filter_input(INPUT_POST, $varname FILTER_SANITIZE_STRING));
    if (empty($value)) {
        $error = $varmsg;
        break;
    }
}

if ($error) {
    echo <<<HTML
<div class="alert alert-danger alert-dismissable">
    <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>
    {$error}
</div>
HTML;
} else {
    /* ... */

    $stm = $link->prepare("INSERT INTO item (item_desc,display,brand) VALUES(?,?,?)");
    $stm->execute($values);

    /* ... */
}
\$\endgroup\$
  • \$\begingroup\$ They say it's quite hard to prevent the upload of a PHP code inside of a jpeg file. Either way you didn't offer any solution. I don't think a good review should name a problem but offer no solution. \$\endgroup\$ – Your Common Sense Dec 20 '18 at 11:49
  • \$\begingroup\$ @YourCommonSense You can very easily append PHP to the image, and it will remain perfectly valid (for example, you can do it with file_put_contents('image.jpg', '<?php', FILE_APPEND)). As for solutions, they are not specific to this code. It is better to learn about them on specialized sites (owasp.org) or publish new relevant questions. \$\endgroup\$ – Victor Dec 20 '18 at 12:01
  • \$\begingroup\$ I would say the opposite, solutions are essential for this code. Least a link in just a general direction offers any help. \$\endgroup\$ – Your Common Sense Dec 20 '18 at 12:02
  • 1
    \$\begingroup\$ You want to say, that for every question we should explain what are SQL injection, Unrestricted File Upload, and Full Path Disclosure vulnerabilities? And every time to provide full snippets how to protect against them? Or maybe we should waste our time to search good links for each of them? Hm... I think, my task was to direct the author to the right path and tell him about vulnerabilities. I hope he will appreciate it. At least because no one told him about these vulnerabilities. \$\endgroup\$ – Victor Dec 20 '18 at 12:16

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.