-1

I'm writing a PHP script that takes input from an HTML5 form, including uploads, and emails them to an administrator using Magento's mail client. I think I've covered the basics pretty well, but I'm sure I'm missing some potential vulnerabilities as I'm not a security expert.

As of right now, the data is going straight to email with the exception of the temporary files from the HTML5 uploads. It isn't being sent to a database, so I'm validating file extension, file size, file number, string length for other inputs, and using PHP's htmlspecialchars() function to avoid HTML injection.

Can anyone point out some things that I might be overlooking here?

<?php

$maxfilesize = 1000;
$maxStringLength = 50;

if(isset($_POST['submit'])){
    $drivers_license = $_FILES['drivers-license'];

    $cfi_cert = $_FILES['cfi-cert'];
    $cfi_multiple = false;
    if(count($_FILES['cfi-cert']['name']) > 1){
        if(count($_FILES['cfi-cert']['name']) > 2){
            die('Invalid input')
        }

        $cfi_multiple = true;

        $cfi_cert1['name'] = $_FILES['cfi-cert']['name'][0];
        $cfi_cert1['type'] = $_FILES['cfi-cert']['type'][0];
        $cfi_cert1['tmp_name'] = $_FILES['cfi-cert']['tmp_name'][0];
        $cfi_cert1['error'] = $_FILES['cfi-cert']['error'][0];
        $cfi_cert1['size'] = $_FILES['cfi-cert']['size'][0];

        $cfi_cert2['name'] = $_FILES['cfi-cert']['name'][1];
        $cfi_cert2['type'] = $_FILES['cfi-cert']['type'][1];
        $cfi_cert2['tmp_name'] = $_FILES['cfi-cert']['tmp_name'][1];
        $cfi_cert2['error'] = $_FILES['cfi-cert']['error'][1];
        $cfi_cert2['size'] = $_FILES['cfi-cert']['size'][1];
    }

    //Remove special characters from text inputs

    $ftn = htmlspecialchars($_POST['ftn']);
    $phone = htmlspecialchars($_POST['phone']);
    $email = htmlspecialchars($_POST['email']);

    if(strlen($email) > $maxStringLength || strlen($phone) > $maxStringLength || strlen($ftn) > $maxStringLength){
        die('Invalid input');
    }

    //Build attachments array, calling image validation function for each

    $attachments = array();

    imageValidationErrorCheck($drivers_license, $maxfilesize);
    $attachments[] = $drivers_license;

    if($cfi_multiple){
        imageValidationErrorCheck($cfi_cert1, $maxfilesize);
        $attachments[] = $cfi_cert1;
        imageValidationErrorCheck($cfi_cert2, $maxfilesize);
        $attachments[] = $cfi_cert2;
    } else {
        imageValidationErrorCheck($cfi_cert, $maxfilesize);
        $attachments[] = $cfi_cert;
    }

    //Use Magento's email client

    $mageFilename = '../app/Mage.php';
    require_once($mageFilename);
    Mage::app();

    $mailTemplate = Mage::getModel('core/email_template');
    $mailTemplate->setSenderName('Test Sender');
    $mailTemplate->setSenderEmail('testsender12345@test.com');
    $mailTemplate->setTemplateSubject('Processing');

    $output .= "Email Address:<br>";
    $output .= $email . "<br><br>";
    $output .= "Phone Number:<br>";
    $output .= $phone . "<br><br>";
    $output .= "FTN:<br>";
    $output .= $ftn . "<br><br>";

    $mailTemplate->setTemplateText($output);

    foreach($attachments as $attachment){
        $mailTemplate->getMail()->createAttachment(
        file_get_contents($attachment['tmp_name']),
        Zend_Mime::TYPE_OCTETSTREAM,
        Zend_Mime::DISPOSITION_ATTACHMENT,
        Zend_Mime::ENCODING_BASE64,
        $attachment['name']
        );
    }

    $mailTemplate->send('testrecipient@test.com');
}

//Validate images

function imageValidationErrorCheck($file, $maxSizeKb){
    $error = '';
    $baseName = basename($file['name']);
    $type = substr($baseName, strrpos($baseName, '.') + 1);
    $sizeInKb = $file['size'] / 1024;

    //Limit size to max file size
    if($sizeInKb > $maxSizeKb){
        die('Invalid input');
    }

    //Check file extension
    $allowedExtensions = array("jpg", "jpeg", "gif", "bmp", "png", "tiff", "pdf", "doc", "docx");
    if(!in_array(strtolower($type), $allowedExtensions)){
        die('Invalid input');
    }
}


?>
1

I wouldn't hand roll something like this. I'd try and find something that's existing and proven.

Case in point

What happens if my filename is maliciousPayload.jpg.php

function imageValidationErrorCheck($file, $maxSizeKb){
$error = '';
$baseName = basename($file['name']);
$type = substr($baseName, strrpos($baseName, '.') + 1);
$sizeInKb = $file['size'] / 1024;

//Limit size to max file size
if($sizeInKb > $maxSizeKb){
    die('Invalid input');
}

//Check file extension
$allowedExtensions = array("jpg", "jpeg", "gif", "bmp", "png", "tiff", "pdf", "doc", "docx");
if(!in_array(strtolower($type), $allowedExtensions)){
    die('Invalid input');
}

Just from looking at what you have here, it's going to bypass your filter.

reference How to check a file's extension in PHP

| improve this answer | |
  • 1
    AFAIK, tmp_name is php-generated (ie: safe [0-9a-f]+-like name), so you won't be able to take a file besides the TMP file where PHP put the uploaded data. It would be vulnerable tho if you saved it to $_FILE['name'] which is client-defined – Xenos May 11 '17 at 14:51
  • @Xenos I'd err on the side of "There's always a way to break it, especially when you hand roll uncommon patterns" – Anthony Russell May 11 '17 at 15:01
  • file_get_content of the tmp_name is a common pattern to me, making it reliable, but that's not the case of the extension part, the strlen part (multibyte), the unchecked existence of $_POST, and probably the $output .= part: these should be pointed out – Xenos May 11 '17 at 15:07

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