Rip up My Code

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
jonny2009
Forum Newbie
Posts: 5
Joined: Mon Mar 23, 2009 2:28 pm

Rip up My Code

Post by jonny2009 »

This is the php code for a Flash based CMS, and I'd like to know about any security loopholes. I'll try to explain the order of things that happens so that everyone knows what's going on and can find any loopholes :)

The login screen is in flash, the typed username and password get sent to a php file for checking, no username or password info ever leaves the server. This is top of the page that the flash redirects to, sending the username and password that was typed as POST vars, the actual username and password are hard coded in:

Code: Select all

 
<?php
 
$userName = "demo";
$password = "demo";
 
session_start();
$_SESSION["secure"] = false;
 
$fullpath = 'http://' . $HTTP_SERVER_VARS[HTTP_HOST] . $HTTP_SERVER_VARS[REQUEST_URI];
$thisfile = basename($fullpath);
$cutoff   = strpos($fullpath, $thisfile);
$thisdir  = substr($fullpath, 0, $cutoff);
 
if ($_POST['uName'] !== $userName || $_POST['pWord'] != $password)
    { header('location:' . $thisdir . 'forbidden.html'); die("Error!"); }
else
    { $_SESSION["secure"] = true; }
 
?>
 
I did the whole $thisdir thing because I've heard safari on mac has issues if you don't. I dunno if that is true, but I figured it was worth doing. From there the session id gets passed to flash, so that every time flash sends a URLRequest to the second php page that I'll post next, it can validate that the user is logged in via the session var. This php page is never directly gone to, just gets requests sent to it from flash, alowing flash to do different operations on the server:

Code: Select all

 
<?php
 
$allowed = array("jpg", "jpeg", "gif", "tiff", "tif", "png", "bmp", "psd", "ai", "eps", "mp3", "wav", "aif", "m3u", "swf", "wmv", "flv", "mov", "f4v", "avi", "html", "htm", "xml", "css", "js", "txt", "xsl","pdf", "fla", "zip", "doc");
 
$folders = array("code_editing_examples/", "preview_examples/", "file_types/");
 
 
set_magic_quotes_runtime(false);
 
function validateExtension($fname, $arr)
{
    $fname = strtolower($fname) ;
    $exts = explode(".", $fname);
    
    return in_array($exts[count($exts)-1], $arr);
}
 
function validateFolder($fname, $arr)
{
    return in_array($fname, $arr);
}
 
function doesntHaveFolders($fname)
{
    $folderCheck = strpos($fname, "/") === false;
    $upLevelCheck = strpos($fname, "..") === false;
    
    return $folderCheck && upLevelCheck;
}
 
$opType = $_POST["action"];
 
session_id($_POST['id']);
session_start();
if ($_SESSION['secure'] !== true)
{
    $opType = "NONE";
    die("|FAILURE|");
}
 
if ($opType == "breakcache")
{
    echo file_get_contents($_POST["file"], "rb");
}
else if ($opType == "readdir")
{
    $folderToRead = $_POST["folder"];
    
    if (validateFolder($folderToRead, $folders))
    {
        if ($handle = opendir($folderToRead))
        {
            while (false !== ($file = readdir($handle)))
            {
                if (true !== is_dir($file) && validateExtension($file, $allowed)) { echo "$file\n"; } 
            }
        }
    }
}
else if ($opType == "trash")
{
    $toDelete = $_POST["folder"] . $_POST["item"];
 
    if (validateExtension($toDelete, $allowed) && validateFolder($_POST["folder"], $folders) && doesntHaveFolders($_POST["item"]))
    {
        unlink($toDelete);
    }
}
else if ($opType == "rename")
{
    $oldFile = $_POST["folder"] . $_POST["oldName"];
    $newFile = $_POST["folder"] . $_POST["newName"];
    
    if (validateExtension($oldFile, $allowed) && validateExtension($newFile, $allowed) && validateFolder($_POST["folder"], $folders) && doesntHaveFolders($_POST["oldName"]) && doesntHaveFolders($_POST["newName"]))
    {
        rename($oldFile, $newFile);
    }
}
else if ($opType == "upload")
{
    $fullPath = $_POST["folder"] . $_POST["fileName"];
    
    if (validateExtension($fullPath, $allowed) && validateFolder($_POST["folder"], $folders) && doesntHaveFolders($_POST["fileName"]))
    {
        move_uploaded_file($_FILES['Filedata']['tmp_name'], $fullPath);
        chmod($fullPath, 0777);
        echo "SUCCESS";
    }
    else
    {
        unlink($_FILES['Filedata']['tmp_name']);
        echo "UPLOAD FAILURE";
    }
}
else if ($opType == "savefile")
{
    $fullPath = $_POST["folder"] . $_POST["fileName"];
    $writeData = stripslashes($_POST["fileData"]);  
 
    if (validateExtension($fullPath, $allowed) && validateFolder($_POST["folder"], $folders) && doesntHaveFolders($_POST["fileName"]))
    {
        $fp = fopen($fullPath, "wb");
        fwrite($fp, $writeData);
        fclose($fp);
        chmod($fullPath, 0777);
        echo file_get_contents($fullPath, "rb");        
    }
}
else if ($opType == "readtypes")
{
    echo implode("|", $allowed);
}
 
?>
 
I hard coded all allowed file types that can be edited, and also all folders that can be edited within, and I'm validatating that the file names don't have folder paths injected into them etc. And if the session var is not correct, i kill the script using die, AND change the opType to nothing so it won't run any operations even if for some odd reason die doesn't stop it.

I've definitely covered all security risks that I know of. It's the ones I don't know of that scare me :) I'm an expert front end coder, but an intermediate php programmer, so I wanted to post here and gain some insight from the experts :D
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Rip up My Code

Post by kaisellgren »

use $_SERVER instead of $HTTP_SERVER_VARS and enclose HTTP_HOST and REQUESET_URI within quotes.

So, if I log in, and send ?action=index.php, it will echo the contents of the index.php file? I'm not following the idea of this. Besides, file_get_contents() do not take those arguments.

You can figure out your file system structure especially if you have error reporting enabled. For instance, sending a ?action=trash&folder=asd&item=asd will have no dot character, making your script to output an error, because of undefined array index, which nicely shows some file paths.

I have no idea of the project meaning. I wonder if it's good to let anyone to delete files. For instance, what if I created a file (uploaded), can someone else remove it? Your script only checks it is in a certain folder with certain extensions. It does not take ownership into account. Perhaps something you want to think about?

Why do you strip the slashes in file data? This could eliminate line endings, for instance.

The worst thing is, that I can gain access to your server. If I send folder=preview_examples/ and fileName=asd.php%00.jpg to your script, it will pass the checks. However, then I can execute the PHP file as easily as going into preview_examples/asd.php
Last edited by kaisellgren on Tue Mar 24, 2009 3:30 pm, edited 1 time in total.
jonny2009
Forum Newbie
Posts: 5
Joined: Mon Mar 23, 2009 2:28 pm

Re: Rip up My Code

Post by jonny2009 »

kaisellgren wrote:use $_SERVER instead of $HTTP_SERVER_VARS and enclose HTTP_HOST and REQUESET_URI within quotes.
Good to know, thanks :)
kaisellgren wrote:So, if I log in, and send ?action=index.php, it will echo the contents of the index.php file? I'm not following the idea of this.
No, the action will do nothing unless it is "readdir", "trash", or something else it checks for. If it is those things then it performs that action based on the other POST vars sent (as long as the session vars check out of course) Or are you telling me that's what it'll do, if so, where and how do I stop it?
kaisellgren wrote:Besides, file_get_contents() do not take those arguments.
Well, it does work, the cms is fully functional, I'm really just making sure of security now. But please explain what arguments it puts in while we're on the subject, cuz I guess I have it wrong :(
kaisellgren wrote:It is rather easy to figure out your file system structure especially if you have error reporting enabled. For instance, sending a ?action=trash&folder=asd&item=asd will have no dot character, making your script to output an error, because of undefined array index, which nicely shows some file paths.
Hmm, just the kind of thing I wanted to know, thanks! But unfortunately I'll need a little deeper explanation, why will this happen and how would I modify to stop it?
kaisellgren wrote:I have no idea of the project meaning. I wonder if it's good to let anyone to delete files. For instance, what if I created a file (uploaded), can someone else remove it? Your script only checks it is in a certain folder with certain extensions. It does not take ownership into account. Perhaps something you want to think about?
It is a simple CMS for editing a small website, ownership is not an issue, and deleting is fine as long as it's secure enough to only delete files with the allowed extensions and only from the allowed folders, so those are the security holes I'm hoping to find.
kaisellgren wrote:Why do you strip the slashes in file data? This could eliminate line endings, for instance.
If I do not then it ends up with extra slashes, the data is coming through in a format that works with this, this part has been thoroughly tested. The script itself is fully functional in that aspect, both for \n and \r\n style line breaks. I'm really just wanting to make it as secure as possible ;)
kaisellgren wrote:The worst thing is, that I can gain access to your server. If I send folder=preview_examples/ and fileName=asd.php%00.jpg to your script, it will pass the checks. However, then I can execute the PHP file as easily as going into preview_examples/asd.php
Also exactly the kind of loophole I was hoping to find here, could you explain how I could add a check for that? Also just out of curiousity, I've never heard of this %00.jpg idea, could you explain it and why it works so I can better understand how to stop it?

Thanks!
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Rip up My Code

Post by kaisellgren »

jonny2009 wrote:No, the action will do nothing unless it is "readdir", "trash", or something else it checks for. If it is those things then it performs that action based on the other POST vars sent (as long as the session vars check out of course) Or are you telling me that's what it'll do, if so, where and how do I stop it?
I meant ?action=breakcache&file=index.php
jonny2009 wrote:Well, it does work, the cms is fully functional, I'm really just making sure of security now. But please explain what arguments it puts in while we're on the subject, cuz I guess I have it wrong :(
Yea, it "works". The second argument "rb" equals to NULL in terms of functionality. You probably want file_get_contents(...,FILE_BINARY) or if not, then forget the second argument.
jonny2009 wrote:I'll need a little deeper explanation, why will this happen and how would I modify to stop it?
Oh, sorry, I overlooked into the code.
jonny2009 wrote:If I do not then it ends up with extra slashes,
I personally hate Magic Quotes runtime.
jonny2009 wrote:Also exactly the kind of loophole I was hoping to find here, could you explain how I could add a check for that? Also just out of curiousity, I've never heard of this %00.jpg idea, could you explain it and why it works so I can better understand how to stop it?
the folder name passes the check, obviously. Then the filename will pass the extension check since it ends with .jpg. But what happens is that %00 is a NULL byte and will truncate the string, cut everything after it. So, the file ends up in having .php extension because it was .php%00.jpg.

In terms of security, you should not upload files in the document root. Moreover, you should do something like str_replace("\0",...) to remove those NULL bytes.
jonny2009
Forum Newbie
Posts: 5
Joined: Mon Mar 23, 2009 2:28 pm

Re: Rip up My Code

Post by jonny2009 »

kaisellgren wrote:
jonny2009 wrote:I meant ?action=breakcache&file=index.php
I see now. I guess I assumed that would just read back the echoed content of the file, not the actual code. There is really only one file that needs a cache broken in this manner, I'll make that non-dynamic, thank you a lot :)

jonny2009 wrote:Yea, it "works". The second argument "rb" equals to NULL in terms of functionality. You probably want file_get_contents(...,FILE_BINARY) or if not, then forget the second argument.
Again great advice, this should be very useful. I wonder where I got that rb thing from? Oh well, I'll throw it out now!

jonny2009 wrote:Oh, sorry, I overlooked into the code.
No problemo.

jonny2009 wrote:I personally hate Magic Quotes runtime.
Is that what's doing it? I had that turned on for consistency's sake, so that I'd at least know whether it was on or off regardless of the default. But I guess I should "default" it the other way then :)
jonny2009 wrote:the folder name passes the check, obviously. Then the filename will pass the extension check since it ends with .jpg. But what happens is that %00 is a NULL byte and will truncate the string, cut everything after it. So, the file ends up in having .php extension because it was .php%00.jpg.

In terms of security, you should not upload files in the document root. Moreover, you should do something like str_replace("\0",...) to remove those NULL bytes.
Wow, that is a huge one that I knew absolutely nothing about, again, very helpful, thanks a ton and best of luck!
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Rip up My Code

Post by kaisellgren »

You have messed up the quotes... :P

the "rb thing" is used in fopen(). The r specifies read mode and b specifies binary safe reading/writing. With file_get_contents(), you can specify this binary safeness with FILE_BINARY.
Post Reply