Hello All, I just finished writing a simple script in PHP to allow a user to upload a MP3 file to my server via an upload form. I was wondering if I could get some comments on the code of the script, especially security related. Can anyone spot any security errors or flaws in the upload script or is it secure in your opinion? Here's the script code: <?php $target_path = "uploads/"; $flag = 0; // Safety net, if this gets to 1 at any point in the process, we don't upload. $filename = $_FILES['uploadedfile']['name']; $filesize = $_FILES['uploadedfile']['size']; $mimetype = $_FILES['uploadedfile']['type']; $filename = htmlentities($filename); $filesize = htmlentities($filesize); $mimetype = htmlentities($mimetype); $target_path = $target_path . basename( $filename ); if($filename != ""){ echo "Beginning upload process for file named: ".$filename."<br>"; echo "Filesize: ".$filesize."<br>"; echo "Type: ".$mimetype."<br><br>"; } //First generate a MD5 hash of what the new file name will be //Force a MP3 extention on the file we are uploading $hashedfilename = md5($filename); $hashedfilename = $hashedfilename.".mp3"; //Check for empty file if($filename == ""){ $error = "No File Exists!"; $flag = $flag + 1; } //Now we check that the file doesn't already exist. $existname = "uploads/".$hashedfilename; if(file_exists($existname)){ if($flag == 0){ $error = "Your file already exists on the server! Please choose another file to upload or rename the file on your computer and try uploading it again!"; } $flag = $flag + 1; } //Now we check the file's extention and make sure we are really uploading an MP3 file... //First do a blacklist approach and weed out all bad filetypes $blacklist = array(".php", ".phtml", ".php3", ".php4", ".js", ".shtml", ".pl" ,".py",".cgi",".php5"); foreach ($blacklist as $file) { if(preg_match("/$file\$/i", $filename)) { if($flag == 0){ $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; } $flag = $flag + 1; } } //Now do a whitelist approach to allow only safe files... $whitelist = array(".mp3"); foreach ($whitelist as $file) $testname = $filename; //Check the file extention, last 4 characters of filename $last4 = substr($testname, -4); { if(!preg_match("/$file\$/i", $last4)) { if($flag == 0){ $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; } $flag = $flag + 1; } } //Now we check the filesize. If it is too big or too small then we reject it //MP3 files should be at least 1MB and no more than 6.5 MB if($filesize > 6920600){ //File is too large if($flag == 0){ $error = "The file you are trying to upload is too large! Your file can be up to 6.5 MB in size only. Please upload a smaller MP3 file or encode your file with a lower bitrate."; } $flag = $flag + 1; } if($filesize < 1048600){ //File is too small if($flag == 0){ $error = "The file you are trying to upload is too small! Your file has been marked as suspicious because our system has determined that it is too small to be a valid MP3 file. Valid MP3 files must be bigger than 1 MB and smaller than 6.5 MB."; } $flag = $flag + 1; } //Check the mimetype of the file if($mimetype != "audio/x-mp3" and $mimetype != "audio/mpeg"){ if($flag == 0){ $error = "The file you are trying to upload does not contain expected data. Are you sure that the file is an MP3?"; } $flag = $flag + 1; } //All checks are done, actually move the file... if($flag == 0){ if(move_uploaded_file($_FILES['uploadedfile']['tmp_name'], $target_path)) { //Change the filename to MD5 hash and FORCE a MP3 extention. if(file_exists("uploads/".$filename)){ //Rename the file to an MD5 version rename("uploads/".$filename, "uploads/".$hashedfilename); echo "The file ". basename( $filename ). " has been uploaded. Your file is <a href='uploads/$hashedfilename'>here</a>."; } else{ echo "There was an error uploading the file, please try again!"; } } else{ echo "There was an error uploading the file, please try again!"; } } else { echo "File Uploaded Failed!<br>"; if($error != ""){ echo $error; } } ?> PHP: Here's the .htaccess code for the "uploads" directory: php_value engine Off AddHandler cgi-script .php .pl .py .jsp .asp .htm .shtml .sh .cgi Options -ExecCGI <FilesMatch "\.(htaccess|htpasswd|ini|phps|fla|psd|log|sh|php|php3|php4|php5|pl|cgi)$"> Order Allow,Deny Deny from all </FilesMatch> # diguise all file extensions as mp3 ForceType audio/mpeg Code (markup): Any thoughts on the security of this script would really be appreciated and helpful. Thanks, BMR777
absolutely not 1) it has xss exploits. use htmlspecialchars or htmlentities on stuff you're showing the user 2) it uses a blacklist. That is retarded. Use a whitelist. There's always one malicious format you'll miss what the fuck are you doing there? if you're only going to allow mp3, just check if it ends in .mp3, not what it isn't. Also, why are you checking for these anywhere in the name? and why are you using regex when you don't have to? a whitelist, good boy. Why is it retarded, though? it matches for .mp3 anywhere, it should just check if the last 4 characters are .mp3 I suggest you get somebody who knows what they're doing
Ok, I have modified the script for your two suggestions. I have wrapped the data shown to the user in htmlentities as well as modified the whitelist to check only the last 4 characters of the filename, rather than anywhere in the filename, to determine if the file is an mp3. Can you please glance over the modified version and tell me if it still needs any work or if you believe it to be secure? Thanks <?php $target_path = "uploads/"; $flag = 0; // Safety net, if this gets to 1 at any point in the process, we don't upload. $filename = $_FILES['uploadedfile']['name']; $filesize = $_FILES['uploadedfile']['size']; $mimetype = $_FILES['uploadedfile']['type']; $filename = htmlentities($filename); $filesize = htmlentities($filesize); $mimetype = htmlentities($mimetype); $target_path = $target_path . basename( $filename ); if($filename != ""){ echo "Beginning upload process for file named: ".$filename."<br>"; echo "Filesize: ".$filesize."<br>"; echo "Type: ".$mimetype."<br><br>"; } //First generate a MD5 hash of what the new file name will be //Force a MP3 extention on the file we are uploading $hashedfilename = md5($filename); $hashedfilename = $hashedfilename.".mp3"; //Check for empty file if($filename == ""){ $error = "No File Exists!"; $flag = $flag + 1; } //Now we check that the file doesn't already exist. $existname = "uploads/".$hashedfilename; if(file_exists($existname)){ if($flag == 0){ $error = "Your file already exists on the server! Please choose another file to upload or rename the file on your computer and try uploading it again!"; } $flag = $flag + 1; } //Now we check the file's extention and make sure we are really uploading an MP3 file... //First do a blacklist approach and weed out all bad filetypes $blacklist = array(".php", ".phtml", ".php3", ".php4", ".js", ".shtml", ".pl" ,".py",".cgi",".php5"); foreach ($blacklist as $file) { if(preg_match("/$file\$/i", $filename)) { if($flag == 0){ $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; } $flag = $flag + 1; } } //Now do a whitelist approach to allow only safe files... $whitelist = array(".mp3"); foreach ($whitelist as $file) $testname = $filename; //Check the file extention, last 4 characters of filename $last4 = substr($testname, -4); { if(!preg_match("/$file\$/i", $last4)) { if($flag == 0){ $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; } $flag = $flag + 1; } } //Now we check the filesize. If it is too big or too small then we reject it //MP3 files should be at least 1MB and no more than 6.5 MB if($filesize > 6920600){ //File is too large if($flag == 0){ $error = "The file you are trying to upload is too large! Your file can be up to 6.5 MB in size only. Please upload a smaller MP3 file or encode your file with a lower bitrate."; } $flag = $flag + 1; } if($filesize < 1048600){ //File is too small if($flag == 0){ $error = "The file you are trying to upload is too small! Your file has been marked as suspicious because our system has determined that it is too small to be a valid MP3 file. Valid MP3 files must be bigger than 1 MB and smaller than 6.5 MB."; } $flag = $flag + 1; } //Check the mimetype of the file if($mimetype != "audio/x-mp3" and $mimetype != "audio/mpeg"){ if($flag == 0){ $error = "The file you are trying to upload does not contain expected data. Are you sure that the file is an MP3?"; } $flag = $flag + 1; } //All checks are done, actually move the file... if($flag == 0){ if(move_uploaded_file($_FILES['uploadedfile']['tmp_name'], $target_path)) { //Change the filename to MD5 hash and FORCE a MP3 extention. if(file_exists("uploads/".$filename)){ //Rename the file to an MD5 version rename("uploads/".$filename, "uploads/".$hashedfilename); echo "The file ". basename( $filename ). " has been uploaded. Your file is <a href='uploads/$hashedfilename'>here</a>."; } else{ echo "There was an error uploading the file, please try again!"; } } else{ echo "There was an error uploading the file, please try again!"; } } else { echo "File Uploaded Failed!<br>"; if($error != ""){ echo $error; } } ?> PHP:
I would suggest applying htmlentities when you're giving it back, not on the actual variable. Applying it to the filesize is just unneccessary altogether. I would remove the blacklist altogether, the whitelist is taking care of that The preg_match is still retarded, and you're overcomplicating the whole thing as well replace: //Now do a whitelist approach to allow only safe files... $whitelist = array(".mp3"); foreach ($whitelist as $file) $testname = $filename; //Check the file extention, last 4 characters of filename $last4 = substr($testname, -4); { if(!preg_match("/$file\$/i", $last4)) { if($flag == 0){ $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; } $flag = $flag + 1; } } Code (markup): with: $whitelist = array(".mp3"); foreach ($whitelist as $ending) { if(substr($filename, -(strlen($ending))) != $ending) { $error = "The file type you are trying to upload is not allowed! You can only upload MP3 files to the server!"; $flag++; } } Code (markup):
The problem as I see it is that you are just checking filename and mimetype, which frankly are insanely easy to spoof. You also have the issue that while very unlikely, it is entirely possible to have two different files with the SAME MD5 - this could bite you in the kiester far down the road. What I would do is just check that the final extension is in fact .mp3, store the files as $filename.md5($filename).'.store' - because you have the extension that is non-executable what the file actually is doesn't really matter. To prevent code execution server side, I would use an .htaccess to route all requests in the media storage directory to a php file, then have that php file output the requested file's contents. This would prevent code from being executed server-side in the first place - the worst that would happen if someone uploaded a file that was not a .php is that it could run client side instead of server-side - which shouldn't happen so long as your php sends the correct mime-type as a header and sends the file with the .mp3 extension. Your code also has some logic flaws, like checking the same condition twice. For example: if($filename != ""){ when a few lines later you check: if($filename== ""){ Run an else, save yourself the conditional. I would actually comparmentalize this into a function call, then instead of wasting a variable on storing 'bad' conditions, you could simply return what happened/went wrong as a string, that way you don't keep trying to execute code when you already know there's an error. You might also want to consider actually checking the file itself for a valid MP3 header - that would be a much better solution than extension/mime-type. http://www.mpgedit.org/mpgedit/mpeg_format/MP3Format.html http://upload.wikimedia.org/wikipedia/commons/0/01/Mp3filestructure.svg Reading in the first four bytes of the file and parsing them could also be used to display more information about the file like bitrate, encoding, etc, etc. You'd also want to check for "tag" since it could start with a id3 tag.
Thanks Kyosys for the help and the code. Deathshadow, I like your idea of using a PHP script to retrieve the files, but in this case this is not practical for me as I wish to have a flash mp3 player that retrieves the uploaded files for playback, so the files need to be there directly. I actually want to integrate the uploader into a music-site script at some point. The finished script probably won't show the user the file's location at all, rather it will pipe it into a flash MP3 player that appears on the user's profile. Also, I took a look at the info you provided about checking the file headers. What would be the best way to do that in PHP? Another thing I found online was that I could check the first few characters of the file to help determine if it's an MP3. It looks like most, if not all, MP3 files begin with the text characters ID3, so I put this code in the uploader to check for that: //Check that the file really is an MP3 file by reading the first few characters of the file $f = fopen($_FILES['uploadedfile']['tmp_name'],'r'); $s = fread($f,3); fclose($f); if($s != "ID3"){ if($flag == 0){ $error = "The file you are attempting to upload does not appear to be a valid MP3 file."; } $flag++; } PHP: I don't know if you meant something different though. Also, going back to your point about the PHP loader, shouldn't the code in my .htaccess file that forces the MP3 mimetype on all files in the directory be enough to prevent against a malicious file executing server side? I mean, I'm wondering if a malicious file could even run on the server if I force the MP3 mimetype for all files in the directory via .htaccess. With the .htaccess code I've tried uploading some PHP files to the protected directory via FTP and running them but with the .htaccess code the browser wants to download them as .mp3s rather than executing them server-side, so is there then a way to bypass this or is this secure? Thanks, BMR777
It should do the trick - honestly I didn't see it because it was below the scrollbar and I didn't scroll down. As to the check, checking for 'ID3' or 'TAG' will only work on MP3's that have ID3 tags - though that may be sufficient a check. So long as the users are aware their MP3's have to have ID3 tags set, then you should be good to go.