Is my file upload script secure ?

[PHPFAN10]

Hi,

 

I am playing around with file uploads, not really done anything in terms of file uploads before with php.

 

I created the following script and wanted to know if it is secure enough ?

 

It could be improved like with resize image if over allowed size and check to ensure an image name does not already exist in image directory but want to keep it simple but most importantly secure, this is only just for learning purposes so i can learn more about php file uploads.

 

could someone tell me if it's secure enough or has any vulnerabilities ?

 

I have tested myself in FireFox with LiveHTTPHeaders and i manually altered the headers and it seems to be secure but being no pro i don't want to think i have it secure or overlooked something.

 

<?php
  // check if form has been submitted
  if (isset($_FILES['image'])) {
      // initialize errors array
      $errors = array();
      // allowed extensions array
      $allowed_ext = array('jpg', 'jpeg', 'png', 'gif');
      // allowed extensions mime type array
      // a malicious user can tamper with headers so checking header is useless but is here
      // as an additional security check although it cannot be relied upon
      $allowed_ext_mime_type = array('image/jpg', 'image/jpeg', 'image/png', 'image/gif');
      // maximum file size (bytes) => 2MB = 2097152
      $maximum_file_size = 2097152;
      // maximum width and height of image
      $max_image_height = 500;
      $max_image_width = 500;
      // file upload directory
      $file_upload_dir = '../images/upload/';
      
      // file information
      $file_name = $_FILES['image']['name'];
      $file_type = $_FILES['image']['type'];
      $file_ext = strtolower(end(explode('.', "$file_name")));
      $file_size = $_FILES['image']['size'];
      $file_tmp = $_FILES['image']['tmp_name'];
      
      if ($_FILES['image']['name'] == "") {
          $errors[] = 'Please select an image file to upload';
      }
      
      if ($_FILES['image']['error'] == UPLOAD_ERR_FORM_SIZE) {
          $errors[] = 'File size is to large. Maximum file size is 2MB';
      }
      
      if ($_FILES['image']['name'] != "" && !in_array($file_ext, $allowed_ext) && !in_array($file_type, $allowed_ext_mime_type)) {
          $errors[] = 'Only jpg, png and gif image formats are allowed';
      }
      
      if ($file_size > 2097152) {
          $errors[] = 'File size is to large. Maximum file size is 2MB';
      }
      
      if ($_FILES['image']['name'] != "" && !preg_match('/^([a-zA-Z0-9_-])*.([a-zA-Z]){2,100}$/i', $_FILES['image']['name'])) {
          $errors[] = 'File name must be alphanumeric but may also contain <strong>- _</strong>';
      }
      
      // if no validation errors found safe to continue
      if (empty($errors)) {
          // check if file is uploaded via http post
          if (is_uploaded_file($file_tmp)) {
              // give file a new name
              $file_new_name = sha1(uniqid(time(), true));
              
              // get image dimensions
              $image_dimensions = list($width, $height) = getimagesize($file_tmp);
              
              // check image width and height are not to large
              if (($width > $max_image_width) or ($height > $max_image_height)) {
                  echo 'Image dimensions must not exceed ' . $max_image_width . 'px x ' . $max_image_height . 'px';
                  exit;
              }
              
              // file directory and new filename
              $file = $file_upload_dir . $file_new_name . '.' . $file_ext;
              
              // if uploaded via http post move uploaded file to directory
              if (move_uploaded_file($file_tmp, $file)) {

                  $image_display = '<img class="profileimgright profileimgframe" src="' . $file . '" width="' . $width . '" height="' . $height . '" alt="profile image" />';                  
                  $success_text = '<p><b>' . htmlentities($file_name) . '</b> was uploaded successfully</p>';

                  // else if file could not be moved display an error
              } else {
                  echo 'A problem occured during file upload. Please try again';
              }
              
              // file was not uploded via http post, possibly malicious so display an error
              } else
              {
                  echo 'A problem occured during file upload. Please try again';
              }
          } else {
              // errors found, display them
              foreach ($errors as $error) {
                  $SiteErrorMessages .= "$error <br />";
              }
          }
      }
?>

<h1>Image Upload</h1>

<?php
// if errors found display them here
if( isset( $SiteErrorMessages ) ) {
    echo SiteErrorMessages();
}

// if image uploaded successfully display image and success text
if( isset( $image_display, $success_text ) ) {
    echo $image_display;
    echo $success_text;
}
?>

<form action="<?php echo basename(__FILE__); ?>"  method="post" enctype="multipart/form-data" id="frmcontact">
    <fieldset>
        <input type="file" name="image" class="textboxcontact" />
        <input type="submit" value="Upload" class="submitcontact"  />
        <input type="hidden" name="MAX_FILE_SIZE" value="2097152" />
    </fieldset>
</form>

 

Thanks

PHPFAN

[AyKay47]

1. Looks to be a rather well organized code, with good logic

 

2. what are you trying to accomplish with the preg_match?

 

preg_match('/^([a-zA-Z0-9_-])*.([a-zA-Z]){2,100}$/i

 

doesn't look quite right...

 

3. Also in your code here

 

foreach ($errors as $error) {
                  $SiteErrorMessages .= "$error <br />";
              }
          }
      }
?>

<h1>Image Upload</h1>

<?php
// if errors found display them here
if( isset( $SiteErrorMessages ) ) {
    echo SiteErrorMessages();
}

 

I'm not sure where your SiteErrorMessages function is coming from, but to echo the errors you can simply place the foreach loop where that function is now..like this

 

if(!empty($errors)){
      foreach($errors as $error){
            echo "$error <br />";
      }
}

[PHPFAN10]

1. Looks to be a rather well organized code, with good logic

 

2. what are you trying to accomplish with the preg_match?

 

preg_match('/^([a-zA-Z0-9_-])*.([a-zA-Z]){2,100}$/i

 

doesn't look quite right...

 

 

Hi,

 

Basically i will look into that further as i got the regex online somewhere. Basically i wanted the file name to be no longer than 100 chars in length, and that the extension part of the file like .jpg only contains the . (dot) and alphabetical characters and that only alphanumeric and - _ are allowed in file name.

 

3. Also in your code here

 

foreach ($errors as $error) {
                  $SiteErrorMessages .= "$error <br />";
              }
          }
      }
?>

<h1>Image Upload</h1>

<?php
// if errors found display them here
if( isset( $SiteErrorMessages ) ) {
    echo SiteErrorMessages();
}

 

I'm not sure where your SiteErrorMessages function is coming from, but to echo the errors you can simply place the foreach loop where that function is now..like this

 

if(!empty($errors)){
      foreach($errors as $error){
            echo "$error <br />";
      }
}

 

The SiteErrorMessages(); function is used globally on my localsite and it basically styles the errors in a red error box that also displays an X image to the left of errors. This is just for the purpose of styling all errors using a global function instead of having to use the same code over and over in each web page.

 

Thanks for the comment. Was good to see you say it looks well organized and with good logic. I will see if anyone else replies incase there is something i have overlooked/forgotton but thanks for taking the time to reply.

 

Cheers! 

PHPFAN

[AyKay47]

not a problem, as far as your regex is concerned, this will not work as expected for what you are looking for it to do. You need to escape the period "." and write it a little different..

 

preg_match('/^([a-zA-Z0-9_-]{2,100})\.(jpg|gif|png|jpeg)$/i

//this will isolate the extensions names, allowing only those

[PHPFAN10]

not a problem, as far as your regex is concerned, this will not work as expected for what you are looking for it to do. You need to escape the period "." and write it a little different..

 

preg_match('/^([a-zA-Z0-9_-]{2,100})\.(jpg|gif|png|jpeg)$/i

//this will isolate the extensions names, allowing only those

 

WOW! Thank you AyKay47,

 

This looks much better and easier to deal with!

 

THANKS! 

PHPFAN