1
\$\begingroup\$

I do programing with core php & i don't want to like use any cms or mvc framework. if any handy ideas then welcome it.

index.php file

<?php 
/** INCLUDE BSIC CONFIGURATION FILE */
strip_php_extension();

/** GET REQUEST NAME */
$request_uri = trim(filter_input(INPUT_GET, 'uri_req'),'/');

/** CALL REQUESTED PAGE */
_PageCall($request_uri);

?>

function.php file

<?php 
/**
 * strip_php_extension()
 *
 * check website url in .php file name is exist
 *
 * @param null
 * @return redirect the page with removing .php extension from the url bar
 */
function strip_php_extension()  
{  
  $uri = filter_input(INPUT_SERVER,'REQUEST_URI');
  $ext = substr(strrchr($uri, '.'), 1);  
  if ($ext == 'php')  
  {  
    $url = substr($uri, 0, strrpos($uri, '.'));  
    redirect($url);  
  }  
} 

/**
 * _PageCall()
 *
 * check in web current web request
 *
 * @param null,filename
 * @return return file if or return 404 file
 */
function _PageCall($file_name=null){ 
    $pagedir = ROOT.DIRECTORY_SEPARATOR.PAGEDIR;

    /* Every File Request Must Be in HTML */
    if(".html"===substr($file_name, -5)):
        $file_name = trim($file_name,'.html');
        if(file_exists($pagedir.$file_name.'.php')):
            require_once $pagedir.$file_name.'.php';
        else:
            redirect('404.html');
        endif;
    else:
        if(NULL==$file_name||$file_name=='index.html'):
            require_once $pagedir.'index.php';
        elseif($file_name=='a'):
            require_once $pagedir.'ajax.php';
        else:
            redirect(SITEURL.'404.html');
        endif;
    endif;
} ?>
\$\endgroup\$
1
\$\begingroup\$

Guard against ../ in the uri_req parameter

I'm seeing a possible security hole. A big one. Consider the following URL:

http://yoursite.com/index.php?uri_req=../../hacking_scripts/dump_database.php

Now let's pretend for a moment that I'm criminal who has previously breached the web server that hosts your web application. Your app is stored at:

/www/himanshu/index.php

Now when I breached the web server, I created the following file:

/hacking_scripts/dump_database.php

Which contains PHP code to connect to your database as the same username and password as your web site, because after all, I can view the raw PHP code and found a database connection string with a username and password.

The dump_database.php script basically queries for all the tables your web site's user has available to it, does a SELECT * on the table and dumps out the raw data in the resulting HTML file. Upon hitting the URL above, your router gleefully loads that file and executes the PHP inside because I placed ../../ in the uri_req parameter.

Some other improvements:

  • Don't redirect to a 404 Not Found page, because the HTTP status will first be a 301 redirect to a GET request that results in a 200 OK response. Your intended response is 404 Not Found, so set the HTTP status to that value and simply include the 404.html file.

    Web crawlers and search engines, when encountering a 404 Not Found, will not attempt (or should not attempt) to hit that URL again. In your setup, they will be back to the same "Not Found" URL over and over again.

  • The _PageCall function name doesn't really tell you what it does. It routes the request. Why not call it something like routeRequest?

  • Add a try-catch around the calls to other PHP files, then return a 500 Server Error response. This way you don't get ugly stack traces showing up in production, and you can create your own custom, prettier looking error page for unhandled catastrophic errors.

    function _PageCall($file_name=null) {
        try {
            // route the request
        } catch (Exception $ex) {
            http_response_code(500);
            require_once($pagedir . "500.html");
            // Log the error message and stack trace
        }
    }
    
  • Remove duplicated 404 Not Found logic by separating the detecting of a file to load versus the actual executing of that file

    function _PageCall($file_name=null) {
        try {
            $pagedir = ROOT.DIRECTORY_SEPARATOR.PAGEDIR;
            $page = getFullFilePath($pagedir, $file_name);
    
            if (!isset($page)) {
                $page = $pagedir . '404.html';
                http_response_code(404);
            }
    
            require_once($page);
        } catch (Exception $ex) {
            http_response_code(500);
            require_once($pagedir . "500.html");
            // Log the error message and stack trace
        }
    }
    
    function getFullFilePath($pagedir, $file_name=null) {
    
        /* Every File Request Must Be in HTML */
        if(".html"===substr($file_name, -5)):
            $file_name = trim($file_name,'.html');
            if(file_exists($pagedir.$file_name.'.php')):
                return $pagedir.$file_name.'.php';
            else:
                return null;
            endif;
        else:
            if(NULL==$file_name||$file_name=='index.html'):
                return $pagedir.'index.php';
            elseif($file_name=='a'):
                return $pagedir.'ajax.php';
            else:
                return null;
            endif;
        endif;
    }
    
  • Whitespace is your friend. For readability put spaces around all operators and the conditions for if statements:

    if(".html"===substr($file_name, -5)):
    

    Becomes:

    if (".html" === substr($file_name, -5)):
    

    And put some empty lines between IF statements:

    $file_name = trim($file_name,'.html');
    
    if (file_exists($pagedir . $file_name . '.php')):
    
  • Add additional functions for detecting certain kinds of files so that:

    if (".html" === substr($file_name, -5)):
    

    Becomes:

    if (isHtmlFile($file_name)):
    
  • Use a consistent variable naming convention. I see $pagedir and $file_name. In the very least avoid using all lower case letters. Separate words by an underscore or use camelCase.

This isn't a bad script to do simple routing. Really the most important change is to fix the security flaw allowing you to specify any file path in the URL parameter.

\$\endgroup\$
  • \$\begingroup\$ Thank you for reviewing my code.. my method for uri routing is okay or i do another logic for best routing. \$\endgroup\$ – Himanshu Apr 30 '16 at 8:57
  • \$\begingroup\$ I Just post a question of url security hole , you say in this way ? \$\endgroup\$ – Himanshu May 2 '16 at 11:57
  • \$\begingroup\$ The question you posted does not replicate the issue. You appear to be loading and executing a PHP script based on a URL parameter called uri_req. This is, plus not stripping out ../ in the value of the parameter is the source of the vulnerability. \$\endgroup\$ – Greg Burghardt May 2 '16 at 12:25
  • \$\begingroup\$ okay i got that thing, so every request should directly called into index.php and then call uri routing function, never use in parameter for url. \$\endgroup\$ – Himanshu May 2 '16 at 12:44
  • \$\begingroup\$ can u please explain without my uri_req parameter, how to do with direct index.php my routing. \$\endgroup\$ – Himanshu May 2 '16 at 12:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.