CodeSOD: Extensioning Yourself

It's bad news when you inherit a legacy PHP CMS. It's worse news when you see that the vast majority of the files were last changed in 2002 (of course there's no source control).

That is the unfortunate position that Elda found herself in.

Like most CMSes, it needed to let users upload files. But we don't want to let users upload any arbitrary file, so we need to check that the file extension is correct. I mean, ideally, we want to check that the file is a valid file of the appropriate type so we don't serve up badfile.exe.mp3, but for an ancient PHP CMS just checking the extension is likely asking a lot. Especially when we see how this code performs that check.

function CheckFormat($extension){ global $extensionPass; global $media_type; global $AllowedFormats; $extensionPass="false"; if(($extension=="mp3")||($extension=="wma")) { $media_type="Audio"; $extensionPass="true"; } if(($extension=="wmv")) { $media_type="Video"; $extensionPass="true"; } $AllowedFormats="mp3, wma, wmv";}

This is some impressively bad code. Instead of a function which returns a value, we have a trio of global variables that all get updated. Also, instead of using booleans, we get to use stringly typed true/false values.

This is also inside of a big, giant file. So we can just scroll down a ways and see how it's actually used:

// ...$Mediafile_name=$_FILES['Mediafile']['name'];// ...$ext_array =explode(".",$Mediafile_name);$last_position = count($ext_array) - 1 ;$extension = $ext_array[$last_position] ; //get photo extension$extension=strtolower($extension);if($extension=="") //couldn't get the extension which means probably the file was too large and so we ran out of memoryCheckFormat($extension);if($extensionPass=="false"){ echo "<H4>Incompatable Audio File</h4>"; echo "The file you uploaded cannot be used as it is in the format <B>$extension<b/>."; echo "Allowed formats are $AllowedFormats.";}else{ // do something with the uploaded file}

So, based on the comment couldn't get the extension, this tells us that they do no error checking at all on the $_FILES['Mediafile'] object. PHP offers methods like isset to check if a value exists, but no, instead of doing that, we'll chop up a string we might not have and only worry if that string turns out to be empty after chopping.

But, that comment is actually making the code harder to read. Let me remove that and fix the formatting so something becomes really, really obvious:

if($extension=="") CheckFormat($extension);

We only check to see if the extension is valid if it's empty. So we're 100% in the territory of "code that doesn't work". Since it doesn't even work, it's probably unfair to point out that the CheckFormat function sets a $media_type global to be "Audio" or "Video", and but the error message doesn't check that, meaning it outputs "Incompatable(sic) Audio File" even when you upload a video.

All that, and they also have a typo in the closing boldface tag inside that error message- <b/>. There's so much inside these small blocks that I think the // ... is doing a lot of covering for some other real ugly code.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!