-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');
    }
}


?>

closed as too broad by Anders, Serge Ballesta, schroeder May 12 '17 at 8:43

Please edit the question to limit it to a specific problem with enough detail to identify an adequate answer. Avoid asking multiple distinct questions at once. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

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

  • 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.