Security Review, Please :)

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

Post Reply
alecks
Forum Newbie
Posts: 5
Joined: Fri Nov 03, 2006 4:49 pm

Security Review, Please :)

Post by alecks »

Security Review, Please! :)

I am new to PHP (started learning 1 and a half weeks ago) and for experience I made a site (on localhost, not online yet). Users do not register, but it does have an admin that edits content and simple site configuration. If you could review the login code and security of, that'd be great :). A few things:

- It isn't done yet. I haven't coded the admin ops yet, so function adminops() is empty.
- Title and other site descriptions are called from a mysql db in mainfile.php
- I am trying to get everything (login, logout, admin functions) all in the admin.php. It is hard because of the setcookie() and header(). Any suggestions?
- The username and pass are base64_encoded. Is this necessary?
- In terms of cleaning everything up... Any suggestions?

Any and all suggestions, criticisms, and comments are welcome!

Thanks!

admin.php

Code: Select all

<?php
require_once("mainfile.php");

//////////////////////////////////
//////////////////////////////////

$username = base64_encode("user");
$password = base64_encode("pass");

//////////////////////////////////
//////////////////////////////////

function admin(){
adminhead();
adminindex();
adminops();
}

function adminhead(){
global $title, $custommsg, $metakeywords, $metadescription;
echo "<html><head>"
."<title>".$title.": Administration</title>"
."<meta http-equiv=\"Content-Type\" content=\"text/html; charset=iso-8859-1\" />"
."<meta name=\"Description\" content=\"".$metadescription."\" />"
."<meta name=\"Keywords\" content=\"".$metakeywords."\" />"
."<link rel=\"stylesheet\" href=\"style.css\" type=\"text/css\" media=\"screen,projection\" />"
."</head>"
."<body>"
."<div id=\"wrap\">"
."<a href=\"index.php\" class=\"plain\"><div id=\"header\"><p>".$custommsg."</p></div></a>"
."<div id=\"navbar\"></div>";
}

function adminindex(){
echo "<div id=\"nav\">"
."<table>"
."<tr>"
."<td><a href=\"admin.php?op=news\"><img src=\"images/news.png\"></a></td>"
."<td><a href=\"admin.php?op=legal\"><img src=\"images/legal.png\"></a></td>"
."<td><a href=\"admin.php?op=about\"><img src=\"images/about.png\"></a></td>"
."<td><a href=\"admin.php?op=contact\"><img src=\"images/contact.png\"></a></td>"
."<td><a href=\"admin.php?op=about\"><img src=\"images/content.png\"></a></td>"
."</tr>"
."<tr>"
."<td><a href=\"admin.php?op=news\">News<br /><br /></a></td>"
."<td><a href=\"admin.php?op=legal\">Legal<br /><br /></a></td>"
."<td><a href=\"admin.php?op=about\">About<br /><br /></a></td>"
."<td><a href=\"admin.php?op=contact\">Contact<br /><br /></a></td>"
."<td><a href=\"admin.php?op=about\">Content<br /><br /></a></td>"
."</tr>"
."<tr>"
."<td><a href=\"admin.php?op=news\"><img src=\"images/options.png\"></a></td>"
."<td><a href=\"admin.php?op=ads\"><img src=\"images/adedit.png\"></a></td>"
."<td><a href=\"logout.php\"><img src=\"images/logout.png\"></a></td>"
."<td>&nbsp;</td>"
."<td>&nbsp;</td>"
."</tr>"
."<tr>"
."<td><a href=\"admin.php?op=news\">Options</a></td>"
."<td><a href=\"admin.php?op=ads\">Adverts</a></td>"
."<td><a href=\"logout.php\">Logout</a></td>"
."<td>&nbsp;</td>"
."<td>&nbsp;</td>"
."</tr>"
."</table>"
."</div>"
."<div id=\"content\"><p><br />Test Content</p></div>";
}

function adminops(){
global $op;

// not finished  

}

function loginprmpt(){
echo "<div id=\"content\"><p><br /><font class=\"largep\">Administrator Login:</font>"
."<form class=\"center\" action=\"admin.php\" method=\"post\"><br>Username:&nbsp;"
."<input class=\"regform\" type=\"text\" name=\"username\" size=\"21\"><br /><br />Password:&nbsp;&nbsp;"
."<input class=\"regform\" type=\"password\" name=\"password\" size=\"21\"><br /><br />"
."<input class=\"regbutton\" type=\"submit\" value=\"Submit\" size=\"10\"></form><br /><br /></p>"
."</div>";
}

//////////////////////////////////
//////////////////////////////////

if (!isset($_COOKIE['user']) && !isset($_COOKIE['pass'])){
if ($_POST['username'] == base64_decode($username) || $_POST['password'] == base64_decode($password)){

setcookie("user", $username, time()+60*60*24*100, "/");
setcookie("pass", $password, time()+60*60*24*100, "/");
admin();

}else{
include("header.php");
loginprmpt();
}}

if (isset($_COOKIE['user']) && isset($_COOKIE['pass']) and $_COOKIE['user'] != $username && $_COOKIE['pass'] != $password){ 
if ($_POST['username'] == base64_decode($username) || $_POST['password'] == base64_decode($password)){

setcookie("user", $username, time()+60*60*24*100, "/");
setcookie("pass", $password, time()+60*60*24*100, "/");
admin();
        
}else{
include("header.php");
loginprmpt();
}}

if (isset($_COOKIE['user']) and isset($_COOKIE['pass']) and $_COOKIE['user'] == $username && $_COOKIE['pass'] == $password) {
admin();
}

//////////////////////////////////
//////////////////////////////////

include("footer.php")

?>
logout.php

Code: Select all

<?php
if(isset($_COOKIE['user']) && isset($_COOKIE['pass'])){
setcookie("user", "", time()-60*60*24*100, "/");
setcookie("pass", "", time()-60*60*24*100, "/");
}
Header ("Location: admin.php");
?>
Thanks :)
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

base64 isn't a good encryption, nor would I advise storing the username and password in cookies. If you must store them, place them in a session.
wyrmmage
Forum Commoner
Posts: 56
Joined: Sat Oct 28, 2006 12:43 pm
Location: boise, ID

Post by wyrmmage »

ya...storing in cookies is bad because they can so easily be brute-forced; I always just hash my passwords. Hashing makes it so that your passwords are (virtually) unbreakable, because the vavlue that was the password is non-retrievable. Also, why do you want all of your code in one page? I tend to put mine in more than one just for simpicities sake. :)
-wyrmmage
alecks
Forum Newbie
Posts: 5
Joined: Fri Nov 03, 2006 4:49 pm

Post by alecks »

what encryption would you recommend? Is it necessary if I store the passes in sessions?
wyrmmage
Forum Commoner
Posts: 56
Joined: Sat Oct 28, 2006 12:43 pm
Location: boise, ID

Post by wyrmmage »

I recommend storing passwords in sessions because you really need to have some way that the user is not posing as someone else when you authenticate something that the user is doing.
-wyrmmage

Oh, and also: I don't know much about encryption, but if you want to hash something (which is entirely different than ecrypting something; don't confuse the two!), then I would recommend using md5(), like so:

Code: Select all

<?

$string = "Whatever you want to put here";

md5($string);

?>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

I would recommend SHA256 before MD5, myself.

SHA256 > SHA1 > MD5, more specifically.
wyrmmage
Forum Commoner
Posts: 56
Joined: Sat Oct 28, 2006 12:43 pm
Location: boise, ID

Post by wyrmmage »

hmmm.....I've read about SHA256, but never tried it, myself; you're pick, but I would go with Feyd's choice, he knows more about it than I ;)

-wyrmmage
alecks
Forum Newbie
Posts: 5
Joined: Fri Nov 03, 2006 4:49 pm

Post by alecks »

Ok... I rewrote it using sessions. The authentication and cookie part of the original script are all gone, replaced with this:

Code: Select all

// -------------------------------- //
// LOGIN SESSION AND AUTHENTICATION //
// -------------------------------- //

session_start();

if (($_SESSION['logged'] == true) and ($_SESSION['username'] == $username) and ($_SESSION['username'] == $username)){
admin();
}

if ($_SESSION['logged'] == false){

if (sha1($_POST['username']) == $username || sha1($_POST['password']) == $password){
$_SESSION['logged'] = true;
$_SESSION['password'] = $password;
$_SESSION['username'] = $username;
Header ("Location: admin.php");
}

if (sha1($_POST['username']) != $username || sha1($_POST['password']) != $password){
include("header.php");
loginprmpt();

}else{
include("header.php");
loginprmpt();

}
}
It seems to work pretty well. I have also changed the variables $username and $password to be sha1() encrypted. Is this new script any good? Thanks for the advice.

ps. logout.php has been changed to:

Code: Select all

<?php
session_start();
session_destroy();
Header ("Location: admin.php");
?>
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Code: Select all

and ($_SESSION['username'] == $username) 
and ($_SESSION['username'] == $username)
Checking twice just to be sure? :P
Use && instead of "and"

How do you fill $username and $password? I hope it's not register_globals.

Also the if is quite baffling:

if (condition) A else A
alecks
Forum Newbie
Posts: 5
Joined: Fri Nov 03, 2006 4:49 pm

Post by alecks »

Mordred wrote:

Code: Select all

and ($_SESSION['username'] == $username) 
and ($_SESSION['username'] == $username)
Checking twice just to be sure? :P
Use && instead of "and"

How do you fill $username and $password? I hope it's not register_globals.

Also the if is quite baffling:

if (condition) A else A
Ah whoops :oops: That's meant to be $password.
I don't know what you mean by register_globals. They are set at the beginning as variables and are sha1 hashed.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

What about the if?
And I'm sure loginprmpt is not in the Scrabble dictionary.
Post Reply