Page 1 of 1

Security Review, Please :)

Posted: Fri Nov 03, 2006 5:24 pm
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 :)

Posted: Fri Nov 03, 2006 5:29 pm
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.

Posted: Sat Nov 04, 2006 11:38 am
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

Posted: Sun Nov 05, 2006 10:08 am
by alecks
what encryption would you recommend? Is it necessary if I store the passes in sessions?

Posted: Sun Nov 05, 2006 10:55 am
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);

?>

Posted: Sun Nov 05, 2006 11:16 am
by feyd
I would recommend SHA256 before MD5, myself.

SHA256 > SHA1 > MD5, more specifically.

Posted: Sun Nov 05, 2006 11:26 am
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

Posted: Sun Nov 05, 2006 1:32 pm
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");
?>

Posted: Mon Nov 06, 2006 4:45 am
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

Posted: Mon Nov 06, 2006 9:40 am
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.

Posted: Mon Nov 06, 2006 10:00 am
by Mordred
What about the if?
And I'm sure loginprmpt is not in the Scrabble dictionary.