PHP Script security help

Hi Folks,

I have these basic php scripts that I yuse on my sites. Recently somebody started spamming thru this script, due to this my accound is suspended.

Would someone please tell me what is the problem with this script.
I need to fix this asap.

//read htm
$htm = ".htm";
$pid = "$page$htm";
function show_page($pid)
{
require($pid);
}
// read htm ends

//read title txt
$txt = ".txt";
$title = "$page$txt";
function show_title($title)
{
require($title);
}
// read title ends

Code (markup):

and in a php file I use

show_title("$title");

show_page("$pid");

Code (markup):

to call them.

I think there is a security issue with my function. But I am not that advanced yet. So I really need you help.

 

These $title and $pid variables are coming from GET?

 

your code snippet is not appropriate to understand the situation and your motive which you are looking to achieve ...please explain in more details and supply more relevant code ...

 

Why do you define a function just to call a different function? That's a waste of processor cycles.

 

Have one function that as follows.

//read htm
$htm = ".htm";
$pid = "$page$htm";
function show_page($pid)
{
require($pid);
}
// read htm ends

//read title txt
$txt = ".txt";
$title = "$page$txt";
function show_title($title)
{
require($title);
}
// 

Code (markup):

And with in details.php I have

show_title("$title");
............................
...........................
show_page("$pid");

Code (markup):

I call the files as /details.php?content=aboutus
Title file is called aboutus.txt
And content is called aboutus.htm

This details.php calls these two files.

 

If $page comes right from the URL, there's your security risk right there. And you should seriously rethink your logic of creating one function just to create another. I didn't mention this only to decorate this thread.

 

Sure.. Like I have said, I am not that advanced. But still trying to learn..
What would be more secure way ?

 

One way is to parse the variables and block unwanted stuff: for example if somebody calls an URL like this: /details.php?content=/etc/passwd - to prevent this you could check that the $content var doesn't contain slashes, or dots etc.

BUT - I don't like this method to allow everything except malicious stuff and I like much better to not allow anything except allowed stuff

So how would you to this? Simple:

if ($content == 'aboutus') {
  $page = 'aboutus.txt';
} elseif ($content == 'feedback') {
  $page = 'feedback.txt';
...
} else {
  die('Do you really think you are cool?');
}
require($page);

PHP:

See what I mean? You only execute the script if you received a valid (obviously you have to decide what is valid and what is not) input, otherwise you block execution.

HTH, cheers!

 

Thanks Picouli,

It is impossible to for me to do that as I have hundreds of htm files thats is called.

So for now All I can think of is allow only alphabet a-z and numbers ?
any other way ?

 

What you have done there is to tell the browser to go and get a file from the current path. However, someone may instruct the browser to go and get another file from another directory like:

/details.php?content=/etc/passwd

Or even call an external php file like:

/details.php?content=http://www.baddomain.com/exploit.php

I am not a security expert, but this is roughly how it could be done.
I guess what you can do is to double check the parameters passed from the url using regular expressions.

 

You don't even have to use regular expressions for this. The function basename() was created exactly for this purpose. It would convert a string like "/home/www/htdocs/index.html" into simply "index.html", that way no one can access files outside of a given directory.

 

<?
function get_details( $pid )
{
  # Clean up the pid
  $pid = preg_replace( "/\.(.*)$/", "", $pid );
  # I'm being anal, no need for this ( for most machines )
  $return = array(); 
  # Might want this to log foul requests
  $return["requester"] = $_SERVER['REMOTE_ADDR'];
  # While we're here :
  $return["time"] = time();
  # User agent into array ...
  $return["ua"] = $_SERVER['HTTP_USER_AGENT'];  
  # Need to check that we're making sense still
  if( !file_exists( dirname(__FILE__) . "/" . $pid . ".htm" ) )
  {
    $return["content"] = "Unrecognized content";
  }
  else
  {
    $return["content"] = @file_get_contents( dirname(__FILE__) . "/" . $pid . ".htm" );
  }
  
  if( !file_exists( dirname(__FILE__) . "/" . $pid . ".txt" ) )
  {
    $return["title"] = "Unrecognized title";
  }
  else
  {
    $return["title"] = @file_get_contents( dirname(__FILE__) . "/" . $pid . ".txt" );
  }
  
  # If you had meta desc for pages, you could do the following also :
  # $return["meta"] = @file_get_contents( dirname(__FILE__) . "/" . $pid . ".meta" ) ; 
  # but you don't so I'll leave it commented, but you see how it's working now
  # right?  
  return $return; # Return data, an associative array of all sorts
}
$page = get_details($_GET['content']);
?>
<html>
<head>
<!-- <meta name="description" content="<?=$page['meta'] ?>" /> -->

<title><?=$page['title'] ?></title>
</head>
<body>
<?=$page['content'] ?>

<!-- 
     Other stuff you might wanna include or use to point users
     at other pages

<br />Timestamp : <?=date('l dS \of F Y h:i:s A', $page["time"] ); ?>

<br />Useragent : <?=$page["ua"] ?>

<br />Request From : <?=$page["requester"] ?>

-->
</body>
</html>

PHP:

NJoy.....

 

Cheers.. looks and sounds perfect.. I will definatelly try this code.. Ta mate..

 

always a pleasure ....

 

You would have realized that most of that stuff is unnecessary had you read my post.

 

You have your way and I have mine ....

 

I know that everybody have their own understanding of things but, could you explain what part is not needed and why ?

 

<?
function show_title( $page )
{
  require( basename($page) . ".txt");
}

function show_page( $page )
{
  require( basename($page) . ".htm");
}

show_title($_GET['content']);
?>
<br />
<?
show_page($_GET['content']);

PHP:

He's pointing out that you could use something as simple as that, without using regexp or anything else, however I don't see the point in making empty suggestions and I only saw one person posting a solution, so a solution is what I gave you.

IMO my way is better ....

 

Empty suggestion? I said what would be better. How is that an "empty suggestion"? I am not going to hold hands with someone here.

As for your revised code, you didn't even understand basename() correctly since you don't have to append ".txt" and ".html" to the file names. And you have to explain to me how using cpu intensive regular expressions is better over simply using basename().

So either you have something to say or you just zip it.

 

if .txt and .html werent there the files would never be loaded as the url is
?content=item
not item.ext .....

I understand perfectly what the user wanted and thats what he got ....