Page 1 of 2

We say NO to hackers.

Posted: Mon Mar 02, 2009 4:09 am
by Yanayaya
Hello PHP peeps,

I have been looking to ensure the PHP that I am coding is as secure as it can be (if that is possible). To ensure that this takes place I was wondering if you guys and gals would be so kind as to view my code and highlight any areas you feel need changes made. I cannot afford to be building PHP scripts that can be attacked by Hackers. It is a real concern of mine. I have made a simple CMS that talks directly to a mySQL server database.

The first page I want to confirm is good is the login page for access to the cms. I have stripped out the HTML to leave the php for easier reading.

Code: Select all

 
<?php 
//Connect to database.
include 'library/configuration.php';
include 'library/opendatabase.php';
 
$user_email = mysql_real_escape_string($_POST['email']);
//MD5 Encryption.
if ($_POST['Submit']=='Login'){
$md5pass = md5($_POST['pwd']);
//Pick up the user data to athenticate. Activated is conditional.
$sql = "SELECT id,user_email FROM cms_users WHERE 
        user_email = '$user_email' AND
        user_pwd = '$md5pass' AND user_activated='1'";  
            
$result = mysql_query($sql) or die (mysql_error()); 
$num = mysql_num_rows($result);
 
if ( $num != 0 ) { 
 
//A matching row was found - the user is authenticated. 
session_start(); 
list($user_id,$user_email) = mysql_fetch_row($result);
// this sets variables in the session 
$_SESSION['user']= $user_email;  
//Logged in
if (isset($_GET['ret']) && !empty($_GET['ret'])){
header("Location: $_GET[ret]");} else{header("Location: myaccount.php");}
exit();}
 
//If error, advise user.
header("Location: login.php?msg=Your Password and/or Username are Invalid");
 
//End
exit();} ?>
 
<!--Login Form-->
           <form name="form1" method="post" action="login.php">
           <table width="368" border="0" align="center" cellpadding="0" cellspacing="4">
           <tr>
           <td height="76" colspan="3" class="formfont">&nbsp;</td>
           </tr>
           <tr>
           <td width="73" class="formfont">Your Email:</td>
           <td colspan="2"><input name="email" type="text" class="box" id="email" size="50" /></td>
           </tr>
           <tr>
           <td class="formfont">Password:</td>
           <td colspan="2"><input name="pwd" type="password" class="box" id="pwd" size="50" /></td>
           </tr>
           <tr>
           <td colspan="3"><div align="center"><img src="images/bar_login.gif" width="328" height="2" alt="" /></div></td>
           </tr>
           <tr>
           <td>&nbsp;</td>
           <td width="49"><input class="red" type="submit" name="Submit" value="Login" /></td>
           <td width="230"><input class="red" type="reset" name="Reset" value="Reset" /></td>
           </tr>
           <tr>
           <td>&nbsp;</td>
           <td colspan="2"><h6><?php if (isset($_GET['msg'])) { echo "<div class=\"msg\"> $_GET[msg] </div>"; } ?></h6></td>
           </tr>
           </table>
           </form>
<!--Login Form End-->
The next page I want to have verified is the page that posts data to the MySQL database. Starting with a session check (with redirection to login if not valid) then moving onto data post.

Code: Select all

 
<?php
 
session_start();
 
if (!isset($_SESSION['user']))
{
header("Location: login.php");
}
?>
 
 
<?php
if(isset($_POST['save']))
{
// Data to post
$title   = $_POST['title'];
$content = $_POST['content'];
    
if(!get_magic_quotes_gpc())
{
$title   = addslashes($title);
$content = addslashes($content);
}
//Database Connection
include 'library/configuration.php';
include 'library/opendatabase.php';
 
//Where to put the data when submitted.
$query = "INSERT INTO data (title, content) VALUES ('$title', '$content')";
mysql_query($query) or die('Error ,query failed');
include 'library/closedatabase.php';
    
//Message to confirm the update has worked.
$msg = "<b>Thank you</b> : Your data has been added.";
}
?>
Thank you for your time and help.

Yan

Re: We say NO to hackers.

Posted: Mon Mar 02, 2009 5:12 am
by Chris Corbyn
This in itself is a concern and lends itself to attack. Somebody can send a person to your site with their own domain in $_GET['ret'], make the site on their domain look just like yours and then do as they please once the user is redirected (by you) to their domain without them knowing it.

Code: Select all

if (isset($_GET['ret']) && !empty($_GET['ret'])){
header("Location: $_GET[ret]");}
"or die(mysql_error())" may also be risky. Log the error somewhere "private", don't display it to the public.

This is open to XSS attacks:

Code: Select all

echo "<div class=\"msg\"> $_GET[msg] </div>";
This is (possibly, depending on character sets used) open to SQL injection...

Code: Select all

$title   = addslashes($title);
 
// ... snip ...
 
$query = "INSERT INTO data (title, content) VALUES ('$title', '$content')";
It's a good job you're asking for advice ;) Some relatively minor fixes can help clean most of this up ;)

Re: We say NO to hackers.

Posted: Mon Mar 02, 2009 5:39 am
by Yanayaya
Chris Corbyn wrote:This in itself is a concern and lends itself to attack. Somebody can send a person to your site with their own domain in $_GET['ret'], make the site on their domain look just like yours and then do as they please once the user is redirected (by you) to their domain without them knowing it.

Code: Select all

if (isset($_GET['ret']) && !empty($_GET['ret'])){
header("Location: $_GET[ret]");}
"or die(mysql_error())" may also be risky. Log the error somewhere "private", don't display it to the public.

This is open to XSS attacks:

Code: Select all

echo "<div class=\"msg\"> $_GET[msg] </div>";
This is (possibly, depending on character sets used) open to SQL injection...

Code: Select all

$title   = addslashes($title);
 
// ... snip ...
 
$query = "INSERT INTO data (title, content) VALUES ('$title', '$content')";
It's a good job you're asking for advice ;) Some relatively minor fixes can help clean most of this up ;)

Thank you for the highlights, it really helps. I dont want to be subject to sql injection I have been looking at how they are doing these sorts of things and it is a concern of mine. What fixes would you personally suggest to make this secure? Can you possible give an example?

Cheers.


Yan

Re: We say NO to hackers.

Posted: Mon Mar 02, 2009 7:13 am
by kaisellgren
Outputting mysql_error() is not risky, but dangerous. When I'm trying to crack phpBB applications, it is often needed to know the table names. These tables often have prefixes, e.g. "phpbb_", but if you change it, I cannot know it unless you are exposing MySQL errors.
Chris Corbyn wrote:This is (possibly, depending on character sets used) open to SQL injection...

Code: Select all

$title   = addslashes($title);
 // ... snip ...
 $query = "INSERT INTO data (title, content) VALUES ('$title', '$content')";
In addition to that, addslashes() will not do a thing to prevent \x1a, \r or \n being placed into the query. Using addslashes() is insufficient no matter what encoding you are using. Using mysql_real_escape_string() is the proper way to do it. Also, forgetting to pass the both two parameters into it, might cause troubles.

I would say that you need to work on the session system a bit more... no protections against Fixation or Hijacking attacks, for instance.

Re: We say NO to hackers.

Posted: Mon Mar 02, 2009 2:26 pm
by Mordred
I think Yanayaya stole^H^H^H^H^H used somebody's else code (and I think it was not one, but two somebodies) and is now trying to glue these as best as he can, while freeriding us for security advise. Which I have nothing against per se, mind you (we DO have some fun finding holes, and moreover this forum is exactly the cool place to be freeriding on!) just thought it's worth mentioning ;)

I'll throw in the pot the fun fact that the authentication system is not really working: the check, when failed, doesn't stop hte rest of the script from executing.

Re: We say NO to hackers.

Posted: Wed Mar 04, 2009 4:37 am
by Yanayaya
Mordred wrote:I think Yanayaya stole^H^H^H^H^H used somebody's else code (and I think it was not one, but two somebodies) and is now trying to glue these as best as he can, while freeriding us for security advise. Which I have nothing against per se, mind you (we DO have some fun finding holes, and moreover this forum is exactly the cool place to be freeriding on!) just thought it's worth mentioning ;)

I'll throw in the pot the fun fact that the authentication system is not really working: the check, when failed, doesn't stop hte rest of the script from executing.

Hello Mordred, Not exactly my friend. I have a PHP book which I use for reference, however it is not always the best and providing security advise because it is out of date when it goes to print, with all these hacking techniques floating around I am trying to secure things as best I can. The book also is not a provider of full source code, it is sectional, so the overall effect must be a mish mash. my apologies for that.

However that is why I am here, to work out the kinks and develop better php :)

Re: We say NO to hackers.

Posted: Fri Mar 06, 2009 1:35 pm
by Mordred
Heh, as I said I have nothing against borrowing code and skills :)
There's no need for you to get defensive and offended (esp. as what I guessed at turns out to be true). Pay attention to the advices given, be careful when you implement the fixes and then show us the code to see if you haven't forgotten something.

Re: We say NO to hackers.

Posted: Thu Mar 19, 2009 8:12 am
by batfastad
When I was learning there are 3 things that I found out the hard way pretty quickly :(
I am by no means an expert, so if my advice is complete cr*p then someone please let me know ASAP 8O

Code: Select all

//DETECT+CLEAN MAGIC QUOTE MESS
function input_clean(&$var) {
    $var = stripslashes($var);
}
if ( ini_get('magic_quotes_gpc')) {
    array_walk_recursive($_GET, 'input_clean');
    array_walk_recursive($_POST, 'input_clean');
    array_walk_recursive($_COOKIE, 'input_clean');
}
Runs on every page of a site in a global config.php file or something. You don't need to run this manually, the array_walk_recursive does this job for you
Means you should never have to use addslashes or stripslashes in your code again, unless you know you need to and you know what you're doing.

Code: Select all

// MYSQL CLEAN, prevents MySQL injection
function mysql_clean($var) {
    return mysql_real_escape_string($var);
}
Run this function on EVERY variable before it gets put into your SQL query
No need to run it on vars that aren't going to be used by SQL

Code: Select all

// OUTPUT CLEAN, prevents HTML injection
function html_clean($var) {
    return htmlspecialchars($var, ENT_QUOTES, 'UTF-8');
}
Run this function on EVERY variable before it gets output to your HTML page
No need to run it on vars that won't be output to HTM, or vars that will be processed further like timestamps with date() etc.

Don't refer to variables by their $_GET/$_POST/$_COOKIE etc global reference in your SQL queries or HTML pages. Set them to proper variables manually:

Code: Select all

$variable = mysql_clean($_POST['variable']);
$sql_query = "SELECT `address` FROM `contacts` WHERE `field2`='$variable'";
And make sure register_globals if off in your php.ini file.

Hope these help, and please correct me if I'm wrong 8O
Thx, B

Re: We say NO to hackers.

Posted: Thu Mar 19, 2009 8:38 am
by kaisellgren
batfastad wrote:

Code: Select all

// MYSQL CLEAN, prevents MySQL injection
function mysql_clean($var) {
    return mysql_real_escape_string($var);
}
"clean"? It does not even sanitize the input so why the word "clean"?
batfastad wrote:

Code: Select all

// OUTPUT CLEAN, prevents HTML injection
function html_clean($var) {
    return htmlspecialchars($var, ENT_QUOTES, 'UTF-8');
}
Run this function on EVERY variable before it gets output to your HTML page
Well. That is encoding really. Prevents XSS in case you output it within HTML and your site is rendered as UTF-8.
batfastad wrote:Don't refer to variables by their $_GET/$_POST/$_COOKIE etc global reference in your SQL queries or HTML pages.
Same applies to non-global variables and data in general.

Re: We say NO to hackers.

Posted: Thu Mar 19, 2009 8:56 am
by batfastad
kaisellgren wrote:"clean"? It does not even sanitize the input so why the word "clean"?
Yes sorry, bad choice of words. The developer still needs to validate the data to make sure it is exactly what they expect/want to be put in the DB.

Re: We say NO to hackers.

Posted: Tue Mar 31, 2009 6:49 pm
by dsick
how big is your site, im wondering if you should of used MD5 to encrypt

Re: We say NO to hackers.

Posted: Wed Apr 01, 2009 4:19 am
by kaisellgren
dsick wrote:how big is your site, im wondering if you should of used MD5 to encrypt
MD5 does not encrypt and it should be left alone.

Re: We say NO to hackers.

Posted: Wed Apr 22, 2009 10:06 am
by temidayo
kaisellgren wrote:When I'm trying to crack phpBB applications, it is often needed to know the table names. These tables often have prefixes, e.g. "phpbb_", but if you change it, I cannot know it unless you are exposing MySQL errors.
If you know the table name, is it surefire that you can crack the application?

Re: We say NO to hackers.

Posted: Wed Apr 22, 2009 10:13 am
by kaisellgren
temidayo wrote:If you know the table name, is it surefire that you can crack the application?
Define cracking.

Re: We say NO to hackers.

Posted: Wed Apr 22, 2009 10:46 am
by temidayo
:D I didn't invent the word. I only quoted you.
And contextually, I was thinking you mean you could break into the application
maliciously or circumvent security barrier and gain access to sections which are supposed
to be off-limits.