SitePoint Sponsor

User Tag List

Results 1 to 15 of 15
  1. #1
    SitePoint Addict Normal75's Avatar
    Join Date
    Oct 2001
    Location
    Vancouver, Canada
    Posts
    245
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    $_GET and include

    hi all - the following code seems good in terms of security (to the best of my knowledge) but it seems to think up to 3-4 seconds before it starts loading, can someone please help and take a look to see if there is another way but serve the same purpose? Your help is appreciated. Thanks.

    PHP Code:

    /*** the array of allowed pages ***/
        
    $ok_p = array('pricing',
                      
    'about',
                      
    'plan',
                      
    'register',
                      
    'support',
                      
    'terms',
                      
    'privacy',
                      
    'sitemap');
        
        
    /*** check if file name is in array ***/
        
    if(!in_array($_GET['pages'], $ok_p)){
            echo 
    '<div class=warn><h2>Error!</h2><p>The page entered is unavailable.</p>';
        }
        else
        {
            
    /*** assign the file name ***/
            
    $file "./pages/".$_GET['p'].".php";
            if(!
    file_exists($file))
            {
                echo 
    '<div class=warn><h2>Error!</h2><p>The file requested is unavailable.</p>';
            }
            else
            {
                
    /*** include the file ***/
                
    include $file;
            }
        } 

  2. #2
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,026
    Mentioned
    64 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Normal75 View Post
    hi all - the following code seems good in terms of security (to the best of my knowledge) but it seems to think up to 3-4 seconds before it starts loading, can someone please help and take a look to see if there is another way but serve the same purpose? Your help is appreciated. Thanks.

    PHP Code:

    /*** the array of allowed pages ***/
        
    $ok_p = array('pricing',
                      
    'about',
                      
    'plan',
                      
    'register',
                      
    'support',
                      
    'terms',
                      
    'privacy',
                      
    'sitemap');
        
        
    /*** check if file name is in array ***/
        
    if(!in_array($_GET['pages'], $ok_p)){
            echo 
    '<div class=warn><h2>Error!</h2><p>The page entered is unavailable.</p>';
        }
        else
        {
            
    /*** assign the file name ***/
            
    $file "./pages/".$_GET['p'].".php";
            if(!
    file_exists($file))
            {
                echo 
    '<div class=warn><h2>Error!</h2><p>The file requested is unavailable.</p>';
            }
            else
            {
                
    /*** include the file ***/
                
    include $file;
            }
        } 
    Hell no that isn't safe. If they know where your php tmp directory is they can post a file against that php file and use your get param to force it to execute, then you're pwned.

    NEVER pass unfiltered user input to an include, eval, system or exec statement.

  3. #3
    SitePoint Addict Beaumont's Avatar
    Join Date
    Mar 2005
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Are $_GET[ 'pages' ] and $_GET[ 'p' ] supposed to be the same variable?

  4. #4
    SitePoint Addict Normal75's Avatar
    Join Date
    Oct 2001
    Location
    Vancouver, Canada
    Posts
    245
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Beaumont View Post
    Are $_GET[ 'pages' ] and $_GET[ 'p' ] supposed to be the same variable?
    yes, it's a typo. it should be $_GET['pages'].
    ~It will come to me one day~

  5. #5
    SitePoint Addict Normal75's Avatar
    Join Date
    Oct 2001
    Location
    Vancouver, Canada
    Posts
    245
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Michael Morris View Post
    Hell no that isn't safe. If they know where your php tmp directory is they can post a file against that php file and use your get param to force it to execute, then you're pwned.

    NEVER pass unfiltered user input to an include, eval, system or exec statement.
    Hmm, unfiltered..., I thought by using array to identify the file names as well as ensuring the file exists using file_exist, i am good. Can you or someone else expand further?

    But i guess what i was trying to find out is if someone can spot the problem and point out why it's taking so long to load.

    thanks.
    ~It will come to me one day~

  6. #6
    SitePoint Addict Beaumont's Avatar
    Join Date
    Mar 2005
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Ok. Michael is right that you don't want to use unsanitized user input to do something like an include, but you are sanitizing this input by restricting it to a pre-determined set. If it's limited to a pre-determined set of files that you know exist, then the file_exists() is unnecessary. I think your code could be reduced to something like this:

    PHP Code:
    <?php

    /*** the array of allowed pages ***/

    $ok_p = array(

      
    'pricing',

      
    'about',

      
    'plan',

      
    'register',

      
    'support',

      
    'terms',

      
    'privacy',

      
    'sitemap'

    );
    // array


    if ( in_array$_GET'page' ], $ok_p ) ) {

      include 
    "./pages/{$_GET'page' ]}.php";

    }
    // if


    else {

    ?>

    <div class="warn"><h2>Error!</h2><p>The page entered is unavailable.</p></div>

    <?php

    }
    // else
    Not sure why it's taking so long to load. What environment are you running the script in?

  7. #7
    SitePoint Zealot
    Join Date
    Jun 2010
    Location
    Arizona
    Posts
    109
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Perhaps you are running into this issue:

    http://cubicspot.blogspot.com/2010/0...ost-under.html

    Maybe?
    Thomas Hruska

    Single Sign-On Server/Client - The PHP login system that rocks.

  8. #8
    Utopia, Inc. silver trophy
    ScallioXTX's Avatar
    Join Date
    Aug 2008
    Location
    The Netherlands
    Posts
    9,067
    Mentioned
    153 Post(s)
    Tagged
    2 Thread(s)
    Not a solution to your problem, but don't forget to send a 404 response to the browser if the requested page (or URL) doesn't exist.
    Rémon - Hosting Advisor

    SitePoint forums will switch to Discourse soon! Make sure you're ready for it!

    Minimal Bookmarks Tree
    My Google Chrome extension: browsing bookmarks made easy

  9. #9
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,026
    Mentioned
    64 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Normal75 View Post
    Hmm, unfiltered..., I thought by using array to identify the file names as well as ensuring the file exists using file_exist, i am good. Can you or someone else expand further?
    If they successfully inject a file onto your site then it will exist. Suppose I send a post to your script with the file I want to execute on your machine as the upload. PHP will put the file in a temporary directory determined by its config, but the defaults are fairly well known to hackers. At that point they just need to include the file - it exists so file_exists does nothing to stop the attack.

  10. #10
    SitePoint Addict Beaumont's Avatar
    Join Date
    Mar 2005
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Michael Morris View Post
    If they successfully inject a file onto your site then it will exist. Suppose I send a post to your script with the file I want to execute on your machine as the upload. PHP will put the file in a temporary directory determined by its config, but the defaults are fairly well known to hackers. At that point they just need to include the file - it exists so file_exists does nothing to stop the attack.
    In the code the OP posted, the include is limited to specific filenames in a specific directory. Of course it's correct that you shouldn't be doing an include using unsanitized user input that could refer to a file that the user could have injected into the filesystem, and that in that situation file_exists() wouldn't do anything to protect you.

  11. #11
    SitePoint Evangelist
    Join Date
    Aug 2005
    Location
    Winnipeg
    Posts
    498
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hell no that isn't safe. If they know where your php tmp directory is they can post a file against that php file and use your get param to force it to execute, then you're pwned.
    While I agree more filtering is needed just for safety sake, in this example the OP is effectively filtering by allowing a minimal whitelist through in the first step of validation.

    Even if you did inject a PHP script unless it's name is in the array $ok_p

    In anycase, including dynamic template like this is a bad idea in general. You should be pulling static HTML files via file_get_contents() not include() unless it's absolutely required like in template composition.

    This is a common technique in implementing a templated site by new PHP developers and will likely bite you in the butt eventually. My biggest worry, is that those included files are not properly protected via file permissions, and someone on your shared server adds some sneaky code. Or worse, someone finds a whole in your code somewhere else and over writes one of the included files whitelisted as "safe".

    Cheers,
    Alex
    The only constant in software is change itself

  12. #12
    SitePoint Addict Beaumont's Avatar
    Join Date
    Mar 2005
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    If they're static files, then by all means, use file_get_contents(). But if they're PHP scripts then obviously include() is called for.

    In anycase, including dynamic template like this is a bad idea in general.
    That's a very broad statement. I wouldn't say that in general. It depends on the use case.

    This is a common technique in implementing a templated site by new PHP developers and will likely bite you in the butt eventually.
    It will likely burn you? I wouldn't say that either.


    My biggest worry, is that those included files are not properly protected via file permissions, and someone on your shared server adds some sneaky code.
    That's a legitimate concern in general, and something the OP would be wise to consider, but what leads you to believe that those files in particular are likely to have permission problems? If the script that pulls them in [using include() or file_get_contents()] had a permission problem, then that script could be compromised itself.


    Or worse, someone finds a whole in your code somewhere else and over writes one of the included files whitelisted as "safe".
    The problem there would be the permissions on those files and the insecure script that allowed them to be overwritten, not the legitimate use of include() (assuming it is legitimate).

  13. #13
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,026
    Mentioned
    64 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by PCSpectra View Post
    In anycase, including dynamic template like this is a bad idea in general. You should be pulling static HTML files via file_get_contents() not include() unless it's absolutely required like in template composition.

    This is a common technique in implementing a templated site by new PHP developers and will likely bite you in the butt eventually.
    So to avoid this solutions like smarty get tried, and much later you realize that maybe include wasn't the enemy - it was misuse of include. After all, include is at the heart of my template engine...

    Code php:
    protected function parseTemplate($template, array $output = array() ) {
     
    	// Extract the global output first.
    	extract( $this->getArrayCopy() );
     
    	// Now the template's local output, which overwrites any global stuff.
    	if (!empty($output)) {
    		extract( $output );
    	}
     
    	ob_start();
     
    	if (!include($this->getTemplate($template))) {
    		throw new FatalException('Unable to read template file '.$template);	
    	}
     
    	return ob_get_clean();
    }

    What's going on here is more elaborate than what the OP is doing by a fair stretch - and by doing the include in a function scope I control the variables that file gets to see. Not only that but the logic by which the template is determined is more sophisticated which is to be expected - I've been doing this 7 years now.

    Include isn't what kills - it's getting scope muddled up and losing track of what is going on in each include that causes problems.

  14. #14
    SitePoint Evangelist
    Join Date
    Aug 2005
    Location
    Winnipeg
    Posts
    498
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I never intended to suggest "include" was evil - I know from past experience that what OP described is likely a mild misuse of include. Include itself is a absolute required feature of PHP no doubt.

    It's commonly used in most popular template engines, even Smarty at it's core.



    Cheers,
    Alex
    The only constant in software is change itself

  15. #15
    SitePoint Zealot
    Join Date
    Jun 2010
    Location
    Arizona
    Posts
    109
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by thruska View Post
    Perhaps you are running into this issue:

    http://cubicspot.blogspot.com/2010/0...ost-under.html

    Maybe?
    Did my suggestion work or not? I'd like to know.
    Thomas Hruska

    Single Sign-On Server/Client - The PHP login system that rocks.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •