2
\$\begingroup\$

I am using PDO prepared statements to store user entered input and store that input to the database. The code will store a text input and 5 uploaded images.

Will this code protect from data SQL injection and XSS attack? Will it be secure? If not, what are some things I should make secure?

    <?php

$db_username = 'sanoj';
$db_password = '123456';
$newname = md5(rand() * time());
if (isset($_FILES['files'])) {
    $uploadedFiles = array();
    foreach ($_FILES['files']['tmp_name'] as $key => $tmp_name) {
        $errors = array();
        $file_name = md5(uniqid("") . time());
        $file_size = $_FILES['files']['size'][$key];
        $file_tmp = $_FILES['files']['tmp_name'][$key];
        $file_type = $_FILES['files']['type'][$key];

        if ($file_type == "image/gif") {
            $sExt = ".gif";
        } elseif ($file_type == "image/jpeg" || $file_type == "image/pjpeg") {
            $sExt = ".jpg";
        } elseif ($file_type == "image/png" || $file_type == "image/x-png") {
            $sExt = ".png";
        }
        if (!in_array($sExt, array('.gif', '.jpg', '.png'))) {
            $errors[] = "Image types alowed are (.gif, .jpg, .png) only!";
        }
        if ($file_size > 2097152000) {
            $errors[] = 'File size must be less than 2 MB';
        }
        $desired_dir = "user_data/";
        if (empty($errors)) {
            if (is_dir($desired_dir) == false) {
                mkdir("$desired_dir", 0600);        // Create directory if it does not exist
            }
            if (move_uploaded_file($file_tmp, "$desired_dir/" . $file_name . $sExt)) {
                $uploadedFiles[$key] = array($file_name . $sExt, 1);
            } else {
                echo "Couldn't upload file " . $_FILES['files']['name'][$key];
                $uploadedFiles[$key] = array($_FILES['files']['name'][$key], 0);
            }
        } else {

        }
    }

    foreach ($uploadedFiles as $key => $row) {
        if (!empty($row[1])) {
            $codestr = '$file' . ($key + 1) . ' = $row[0];';
            eval($codestr);
        } else {
            $codestr = '$file' . ($key + 1) . ' = NULL;';
            eval($codestr);
        }
    }
}
$orig_directory = "$desired_dir";    //Full image folder
$thumb_directory = "thumb/";    //Thumbnail folder

/* Opening the thumbnail directory and looping through all the thumbs: */
$dir_handle = @opendir($orig_directory); //Open Full image dirrectory
if ($dir_handle > 1) { //Check to make sure the folder opened
    $allowed_types = array('jpg', 'jpeg', 'gif', 'png');
    $file_type = array();
    $ext = '';
    $title = '';
    $i = 0;

    while ($file_name = @readdir($dir_handle)) {
        /* Skipping the system files: */
        if ($file_name == '.' || $file_name == '..')
            continue;

        $file_type = explode('.', $file_name);    //This gets the file name of the images
        $ext = strtolower(array_pop($file_type));

        /* Using the file name (withouth the extension) as a image title: */
        $title = implode('.', $file_type);
        $title = htmlspecialchars($title);

        /* If the file extension is allowed: */
        if (in_array($ext, $allowed_types)) {

            /* If you would like to inpute images into a database, do your mysql query here */

            /* The code past here is the code at the start of the tutorial */
            /* Outputting each image: */

            $nw = 100;
            $nh = 100;
            $source = "$desired_dir{$file_name}";
            $stype = explode(".", $source);
            $stype = $stype[count($stype) - 1];
            $dest = "thumb/{$file_name}";

            $size = getimagesize($source);
            $w = $size[0];
            $h = $size[1];

            switch ($stype) {
                case 'gif':
                    $simg = imagecreatefromgif($source);
                    break;
                case 'jpg':
                    $simg = imagecreatefromjpeg($source);
                    break;
                case 'png':
                    $simg = imagecreatefrompng($source);
                    break;
            }

            $dimg = resizePreservingAspectRatio($simg, $nw, $nh);
            imagepng($dimg, $dest);
        }
    }

    /* Closing the directory */
    @closedir($dir_handle);
}

try {
#connection 
    $conn = new PDO('mysql:host=localhost;dbname=localtest', $db_username, $db_password);
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $data = $conn->prepare('INSERT INTO agriculture (cacat, mtype, mtitle, image1, image2, image3, image4, image5, description, mcondition, cmodel, price, youare, mname, email, phone, ylocation, ystreet) VALUES (:cacat, :mtype, :mtitle, :image1, :image2, :image3, :image4, :image5, :description, :mcondition, :cmodel, :price, :youare, :mname, :email, :phone, :ylocation, :ystreet)');
    $cacat = filter_input(INPUT_POST, 'cacat', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $mtype = filter_input(INPUT_POST, 'mtype', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $mtitle = filter_input(INPUT_POST, 'mtitle', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $description = filter_input(INPUT_POST, 'description', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $mcondition = filter_input(INPUT_POST, 'mcondition', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $cmodel = filter_input(INPUT_POST, 'cmodel', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $price = filter_input(INPUT_POST, 'price', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $youare = filter_input(INPUT_POST, 'youare', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $mname = filter_input(INPUT_POST, 'mname', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $phone = filter_input(INPUT_POST, 'phone', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $ylocation = filter_input(INPUT_POST, 'ylocation', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $ystreet = filter_input(INPUT_POST, 'ystreet', FILTER_SANITIZE_STRING, FILTER_FLAG_ENCODE_AMP);
    $data->execute(array(':cacat' => $cacat,
        ':mtype' => $mtype,
        ':mtitle' => $mtitle,
        'image1' => $file1,
        'image2' => $file2,
        'image3' => $file3,
        'image4' => $file4,
        'image5' => $file5, ':description' => $description, ':mcondition' => $mcondition, ':cmodel' => $cmodel, ':price' => $price, ':youare' => $youare, ':mname' => $mname, ':email' => $email, ':phone' => $phone, ':ylocation' => $ylocation, ':ystreet' => $ystreet));
#exception handiling
} catch (PDOException $e) {
    echo $e->getMessage();
}

function resizePreservingAspectRatio($img, $targetWidth, $targetHeight) {
    $srcWidth = imagesx($img);
    $srcHeight = imagesy($img);

    // Determine new width / height preserving aspect ratio
    $srcRatio = $srcWidth / $srcHeight;
    $targetRatio = $targetWidth / $targetHeight;
    if (($srcWidth <= $targetWidth) && ($srcHeight <= $targetHeight)) {
        $imgTargetWidth = $srcWidth;
        $imgTargetHeight = $srcHeight;
    } else if ($targetRatio > $srcRatio) {
        $imgTargetWidth = (int) ($targetHeight * $srcRatio);
        $imgTargetHeight = $targetHeight;
    } else {
        $imgTargetWidth = $targetWidth;
        $imgTargetHeight = (int) ($targetWidth / $srcRatio);
    }

    // Creating new image with desired size
    $targetImg = imagecreatetruecolor($targetWidth, $targetHeight);

    // Add transparency if your reduced image does not fit with the new size
    $targetTransparent = imagecolorallocate($targetImg, 255, 0, 255);
    imagefill($targetImg, 0, 0, $targetTransparent);
    imagecolortransparent($targetImg, $targetTransparent);

    // Copies image, centered to the new one (if it does not fit to it)
    imagecopyresampled($targetImg, $img, 0, 0, 0, 0, $targetWidth, $targetHeight, $srcWidth, $srcHeight);

    return $targetImg;
}

?>

SQL

create table `agriculture`(
`id` int(9) NOT NULL auto_increment,
`cacat` varchar(255) NOT NULL default '',
`mtype` varchar(255) NOT NULL default '',
`mtitle` varchar(255) NOT NULL default '',
`image1` varchar(255) NOT NULL default '',
`image2` varchar(255) NOT NULL default '',
`image3` varchar(255) NOT NULL default '',
`image4` varchar(255) NOT NULL default '',
`image5` varchar(255) NOT NULL default '',
`description` varchar(255) NOT NULL default '',
`mcondition` varchar(255) NOT NULL default '',
`cmodel` varchar(255) NOT NULL default '',
`price` varchar(255) NOT NULL default '',
`youare` varchar(255) NOT NULL default '',
`mname` varchar(255) NOT NULL default '',
`email` varchar(255) NOT NULL default '',
`phone` varchar(255) NOT NULL default '',
`ylocation` varchar(255) NOT NULL default '',
`ystreet` varchar(255) NOT NULL default '',
`ipnu` varchar(255) NOT NULL default '',
PRIMARY KEY (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=1 ;
\$\endgroup\$
  • \$\begingroup\$ I rolled back your edit, because it changed the source code, which invalidates already existing answers, which in turn makes it harder for further reviewers to follow. See also What you may and may not do after receiving answers. \$\endgroup\$ – tim Jan 3 '15 at 11:21
  • \$\begingroup\$ but i just add line according to your comment echo "Couldn't upload file " . htmlspecialchars($_FILES['files']['name'][$key], ENT_QUOTES, 'UTF-8') \$\endgroup\$ – sanoj lawrence Jan 3 '15 at 11:23
3
\$\begingroup\$

Not a complete review (do you want all the code reviewed? The comments seem to indicate that maybe not all of it is yours?), just a couple of points I noticed.

Will this code protect from data SQL injection and XSS attack? Will it be secure?

Your image upload code seems secure to me. A couple things I noticed:

  • if a file cannot be uploaded, you echo the filename, which is user controlled. But an attacker cannot use a slash (I tried it with filename=test<script>alert('test');</script>.jpg, which resulted in Couldn't upload file script>.jpg). But even though an attacker cannot include an external javascript file, they can still execute arbitrary code using a file name of for example test<svg onload=alert('test')>.jpg. The only problem for an attacker to solve is how to create an error while uploading the file (the documentation says it will return an error If filename is not a valid upload file, but does not mention what that means). Still, just to be save, I would use htmlspecialchars to avoid XSS.
  • $_FILES['files']['type'] is also user controlled, so it is generally not recommended to use it to check for illegal file extensions. If you would keep the original filename intact, this would be a major problem, but as you are completely regenerating it, this is safe. If at some point you redo your file naming process, you should keep an eye on this.
  • you don't actually need filter_input, as you are using prepared statements, and I don't think that it actually does anything usefull against SQL injection anyways (the documentation says it Strip tags, optionally strip or encode special characters.).
  • mkdir("$desired_dir", 0700): as this is an image directory, there should be no need to execute files. 0600 should be enough.

Empty Else and Error Reporting

In your upload code, you collect all error messages in an array, and then check if this array contains errors. If not, you do nothing. Instead, you should print the error messages to the user, so they know what went wrong.

Eval

Eval is evil, so I would avoid it if I can. In your case it's safe because the input is not user controlled, but there is no reason to use it like you do.

Transforming an array into X variables doesn't seem to server a purpose, just access the array directly. Or, if I am overlooking some reason why this is needed, you could also just hardcode the variables, as only the first five are actually used (you did not post your HTML code, but I'm assuming the user is aware of this).

Splitting up your Code

150 lines of code is too much to leave it in one big block, it would be a lot easier to read if it was split up into functions. For example uploadFile, createThumbnail, storeImage, etc.

Misc

  • $title seems to be unused
  • if a user does not upload a file, should the rest of the code actually execute? If not, return after asking if $_FILES is set.
\$\endgroup\$
  • \$\begingroup\$ thanks it was helpfull so i need to make client-side image name Validator, $_FILES['files']['type'] how do i use this instead \$\endgroup\$ – sanoj lawrence Jan 3 '15 at 6:09
  • \$\begingroup\$ @sanojlawrence like I said, your approach of generating new image names + extensions is quite good. But just in case the requirements change at some point and the image is not renamed anymore, you might want to add extra checks. To check uploaded images, getimagesize() seems to be the most recommended option for extra checks. And your validation of user data should never (only) be client-side, but server-side, and to prevent XSS via image name, filter it before printing it back to a user. \$\endgroup\$ – tim Jan 3 '15 at 8:29
  • \$\begingroup\$ but i already have $newname = md5(rand() * time()); to generate new name \$\endgroup\$ – sanoj lawrence Jan 3 '15 at 8:37
  • \$\begingroup\$ if you can make some change in above code please \$\endgroup\$ – sanoj lawrence Jan 3 '15 at 8:39
  • 1
    \$\begingroup\$ @sanojlawrence you mean regarding my first point with the maybe possible XSS? Yes, you generate a new name, but in this line (the error reporting): echo "Couldn't upload file " . $_FILES['files']['name'][$key], you actually echo the original name. If an attacker can generate an error when uploading files, XSS might be possible. So I would replace it with echo "Couldn't upload file " . htmlspecialchars($_FILES['files']['name'][$key], ENT_QUOTES, 'UTF-8'), just to be extra save. \$\endgroup\$ – tim Jan 3 '15 at 9:54

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.