Page 1 of 2

Website security

Posted: Mon Jan 15, 2007 5:55 pm
by djroadstar
I,m building a website and want you to ask to review my site and look if there are security bugs.

Other feedback is welcome! The URL is http://www.koopeenpaard.nl/index2.php

Tanks!

Posted: Mon Jan 15, 2007 5:56 pm
by feyd
We need you to post the code. We cannot, for many legal reasons, perform security checks against a URL.

Posted: Mon Jan 15, 2007 5:59 pm
by Luke
are there any specific areas you're worried about? If so, post the code from those areas.

Posted: Mon Jan 15, 2007 6:23 pm
by djroadstar
I have writen a function that will look at mij $_GET and $_POST array and clean it.

Mail me to info@koopeenpaard.nl and i mail you back. If you don,t beleve me, write down some words and i will post it on my site.

The security i,m worry about is that sql_injection and that somebody try to send spam by my site. I already had someone who sended me 1400 mails.....

Posted: Mon Jan 15, 2007 6:25 pm
by Nodda4me
djroadstar: The correct punctuation is ' not ,.

Posted: Mon Jan 15, 2007 6:30 pm
by djroadstar
Nodda4me wrote:djroadstar: The correct punctuation is ' not ,.
?

Posted: Mon Jan 15, 2007 6:32 pm
by John Cartwright
Post the code your worried about. One thing I noticed is you arn't using htmlspecialchars() on your output.. tsk tsk :wink:

Google up XSS injection.

Posted: Mon Jan 15, 2007 6:36 pm
by djroadstar
feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


The code against mysql injection etc

Code: Select all

function CheckArrayInput($array)
{
        //----- check if $array is not empty
        if (!empty($array))
        {
            //----- loop through the array and check if the value's been set
            foreach ($array as $value)
            {
                $array = escapeInput($value);
            }
        }        
}

function escapeInput($input)
{
	$input = strip_tags($input);
	$input = mysql_real_escape_string($input);	
	$input = trim($input);
	return $input;
}

Code against mail headers hacking

Code: Select all

function protectMailHeaders($string)
{
    $string = str_replace("\n", "", $string); // Verwijder \n
    $string = str_replace("\r", "", $string); // Verwijder \r
    $string = str_replace("\"", "\\\"", str_replace("\\", "\\\\", $string)); // Slashes van quotes

    return $string;
}

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]

Posted: Mon Jan 15, 2007 6:37 pm
by John Cartwright
djroadstar wrote:I have writen a function that will look at mij $_GET and $_POST array and clean it.

Mail me to info@koopeenpaard.nl and i mail you back. If you don,t beleve me, write down some words and i will post it on my site.

The security i,m worry about is that sql_injection and that somebody try to send spam by my site. I already had someone who sended me 1400 mails.....
Nodda4me wrote:djroadstar: The correct punctuation is ' not ,.
djroadstar wrote:
Nodda4me wrote:djroadstar: The correct punctuation is ' not ,.
?
He was referring to the word's I've highlighted in red from your original post.

Posted: Mon Jan 15, 2007 6:40 pm
by feyd
CheckArrayInput() will perform no actual end result actions.

Posted: Mon Jan 15, 2007 6:46 pm
by John Cartwright
feyd has also posted a smarter strip tags regex before

Posted: Mon Jan 15, 2007 6:55 pm
by djroadstar
feyd wrote:CheckArrayInput() will perform no actual end result actions.
How can I fix this ?

Posted: Mon Jan 15, 2007 7:00 pm
by feyd
It must return the altered array, or the array must be passed by reference to alter it directly. However there is another issue:

Code: Select all

$array = escapeInput($value);
Wipes out the array, leaving only the last value.

Posted: Tue Jan 16, 2007 12:29 am
by jmut
djroadstar wrote:...
function escapeInput($input)
{
$input = strip_tags($input);
$input = mysql_real_escape_string($input);
$input = trim($input);
return $input;
}


...
mysql_real_escape_string is more than enough to escape input for database context. Function also makes attempt to validate data so name is not very appropriate.

Posted: Tue Jan 16, 2007 1:31 am
by matthijs
- Make a list of all variables used in your script/code.
- See were these variables enter your code.
- Decide what they should be like. Number, alpanumeric, a valid email, etc etc? Write code to validate/filter the input data to your rules.
- Asign the validated/filterd data to a "clean" array, to be used in the process of the code.

- See in which places data is outputted. Either to HTML, to a database, or somewhere else.
- In each of those places use a specific escaping funtion to make sure the data can be safely output. So htmlentities for output to HTML, mysql_real_escape_string() to a mysql db, etc.

Code: Select all

// Filter input
clean = array();

switch($_POST['color'])
{
    case 'red':
    case 'green':
    case 'blue':
        $clean['color'] = $_POST['color'];
        break;
}

if (ctype_alnum($_POST['username']))
{
    $clean['username'] = $_POST['username'];
}

// escape output
$html = array();

$html['username'] = htmlentities($clean['username'],
                    ENT_QUOTES, 'UTF-8');

echo "<p>Welcome back, {$html['username']}.</p>";

$mysql = array();

$mysql['username'] = mysql_real_escape_string($clean['username']);

$sql = "SELECT *
        FROM   profile
        WHERE  username = '{$mysql['username']}'";

$result = mysql_query($sql);
Examples straight from http://phpsecurity.org/code
I found the book php security invaluable as a start to better understand php security. Before that I also just threw every striptags/trim/escape function I could find at any variable. But the thing is, a filter/escape function used in the wrong context doesn't do much good.