We say NO to hackers.

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Yanayaya
Forum Commoner
Posts: 42
Joined: Tue Dec 02, 2008 7:49 am

We say NO to hackers.

Post 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
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: We say NO to hackers.

Post 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 ;)
Yanayaya
Forum Commoner
Posts: 42
Joined: Tue Dec 02, 2008 7:49 am

Re: We say NO to hackers.

Post 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
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: We say NO to hackers.

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: We say NO to hackers.

Post 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.
Yanayaya
Forum Commoner
Posts: 42
Joined: Tue Dec 02, 2008 7:49 am

Re: We say NO to hackers.

Post 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 :)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: We say NO to hackers.

Post 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.
User avatar
batfastad
Forum Contributor
Posts: 433
Joined: Tue Mar 30, 2004 4:24 am
Location: London, UK

Re: We say NO to hackers.

Post 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
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: We say NO to hackers.

Post 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.
User avatar
batfastad
Forum Contributor
Posts: 433
Joined: Tue Mar 30, 2004 4:24 am
Location: London, UK

Re: We say NO to hackers.

Post 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.
dsick
Forum Commoner
Posts: 57
Joined: Fri Mar 27, 2009 3:34 pm

Re: We say NO to hackers.

Post by dsick »

how big is your site, im wondering if you should of used MD5 to encrypt
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: We say NO to hackers.

Post 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.
temidayo
Forum Contributor
Posts: 109
Joined: Fri May 23, 2008 6:17 am
Location: Nigeria

Re: We say NO to hackers.

Post 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?
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: We say NO to hackers.

Post by kaisellgren »

temidayo wrote:If you know the table name, is it surefire that you can crack the application?
Define cracking.
temidayo
Forum Contributor
Posts: 109
Joined: Fri May 23, 2008 6:17 am
Location: Nigeria

Re: We say NO to hackers.

Post 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.
Post Reply