2 questions: if and session's

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

boss01
Forum Newbie
Posts: 19
Joined: Sun May 27, 2007 3:31 pm

2 questions: if and session's

Post by boss01 »

Is:

Code: Select all

<?php if($_GET['id']=='5'){Page here if ?id=5}?>
that kind of code secure?
What data i should store in sessions. LoggedIN or user name and password?
Some people say 1 one is stupid and when you use second one someone will come and say what a idiote storing user data in session's
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Re: 2 questions: if and session's

Post by superdezign »

boss01 wrote:Is:

Code: Select all

<?php if($_GET['id']=='5'){Page here if ?id=5}?>
that kind of code secure?
What data i should store in sessions. LoggedIN or user name and password?
Some people say 1 one is stupid and when you use second one someone will come and say what a idiote storing user data in session's
You're code is secure, as long as you actually define all of the outcomes you accept, and give an error if there happens to be an outcome given that you don't accept.

And for logging in, I typically save entire objects that hold their login information and how it's validation has gone, but when it comes down to it, all you really need is one session variable claiming that they are logged on. However, maybe you could make that variable their user name and, occasionally, address the user by it.

But session hacking isn't easy to do, so you shouldn't be worried about it early on.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

If you are using something like that as a basic form of password protection than I am afraid that is not secure. However the conditional check itself isn't ever likely to be comprised.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

It is kinda hard to fracture something that defined. It is flexible? Not on your life. Is it fairly difficult to spoof '5' to get what you want? Sure.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

I think what Ole and Everah are trying to say is that yes, what you have is secure, but we're sure that somewhere along the line, you might make it insecure. That "type" of code could mean almost anything.

Write some more and post it up so we can critique it. 8)
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

Never store the password in a session. Never ever store the password in plain text anywhere! You should at least md5() the password.

Store logged_in and username in a session, that is fine and common.

Your code you posted is secure, however, verifying that they are ID #5 would require seeing more of your code.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

The snippet posted would actually be helpful to an attacker, potentially. The error generated by the index not existing could yield a lot of information that is useful in coordinating an attack.
boss01
Forum Newbie
Posts: 19
Joined: Sun May 27, 2007 3:31 pm

Post by boss01 »

Here is my full page: There is nothing special realy and i removed the template:
Index.php

Code: Select all

<?
session_start();
if($_GET['id']=='6'){
  $password2 = md5($_POST['password']);  //make password md5
ob_start();

include("config.php");


//Let's log into mysql server
$link = mysql_connect($server, $db_user, $db_pass)
or die ("Can't connect mysql ".mysql_error());

//select database
mysql_select_db($database)
or die ("Can't select database ".mysql_error());

//Let's check user data
$kontroll = "select id from $table where username = '".$_POST['username']."'
and password = '".$password2."';";

$query = mysql_query($kontroll)
or die ("mysql_error()");
$num_rows = mysql_num_rows($query);

$result = mysql_query("SELECT * FROM login
 WHERE username='$_POST[username]'") or die(mysql_error());
$row = mysql_fetch_array( $result );

// get the first (and hopefully only) entry from the result
$row2 = mysql_fetch_array( $result2 );

if ($num_rows <= 0) {
header("Location:?id=8");
exit;
} else {    //kui kõik läks hästi hakkame cookiega mässama
setcookie("loggedin", "TRUE", time()+(9000 * 1)); //panem cookie säilivus aja paika
$_SESSION['id'] = $row[id];
$_SESSION['username'] = $_POST[username];
header("Location:indeks.php"); //ajame kasutaja indeks.php nimelisele lehel 
}
ob_end_flush();

}
?>
<a href=index.php>Home</a>
<a href=?id=1>Log in</a>
<a href=?id=2>Register</a>
<?php
//avaleht
if (empty($_GET['id'])){?>
Welcome to my useless and templateless page
<?  //register
 } elseif ($_GET['id']==2) { ?>

                 <form action="reg.php" method="post">
Username: <input type="text" name="username" size="20" class='defaultinput2'><br>
Password: <input type="password" name="password" size="26" class='defaultinput2'><br>
E-mail: <input type="text" name="email" size="27" class='defaultinput2'><br>
<input type="submit" value="Registreeri" name="regabutton" class='defaultinput2'>

<?php }  //Log in
if($_GET['id']=='1'){ ?><form action="?id=6" method="post">
                    Username: <input type="text" name="username" size="15" class='defaultinput2'>
                   Password: <input type="password" name="password" size="15" class='defaultinput2'> <input type="submit"
                    value="Logi Sisse" name="loginbutton" class='defaultinput2'>
                    <?php } ?>
Indeks.php

Code: Select all

<?
session_start();
//Log out page
if($_GET['id']=='4'){
        setcookie("loggedin", "FALSE", time()+(0 * 1));
        header("Location: index.php"); }
if (!isset($_COOKIE['loggedin'])) {
       die(header("Location: index.php"));
} else {}
?>
<a href=indeks.php>Home</a>
<a href=?id=1>Second page</a>
<a href=?id=2>Page 3</a>
<a href=?id=3>Page 4</a>
<a href=?id=4>Log out</a>
<?php
//Deafult page
if (empty($_GET['id'])){?>First/default page is here.<?
//second page
} elseif ($_GET['id']==1) { ?>Second one here<?php }
 //Another page
if($_GET['id']=='2'){ ?>Page 3 here<?php }
//Next page
if($_GET['id']=='3'){?>Here the 4 page<?php }
?>
reg.php

Code: Select all

<?

$password2 = md5($_POST['password']); //Make password md5

include("config.php");

//log onto mysql server
$link = mysql_connect($server, $db_user, $db_pass)
or die ("Error in connecting mysql: ".mysql_error());

// Select database
mysql_select_db($database)
or die ("error in selecting database".mysql_error());

//name lenght check
if (strlen($_POST[username])<4){
header("Location:index.php?id=9");
} else {

//password lenght check
if (strlen($_POST[password])<4){
header("Location:index.php?id=9");
} else {

//E-mail check
if (!strstr($_POST['email'],"@") || !strstr($_POST['email'],".")) {
header("Location:index.php?id=9");
} else {

//htmlspecialchars 
$username2 = htmlspecialchars($_POST['username']);
$email2 = htmlspecialchars($_POST['email']);

//username control
$kontroll = "select id from $table where username = '".$username2."';";
$query = mysql_query($kontroll)
or die ('mysql_error()');
$num_rows = mysql_num_rows($query);
if ($num_rows != 0) {
header("Location:index.php?id=9");
exit;
} else {
//addslashes
$username3 = addslashes($username2);
$password3 = addslashes($password2);
$email3 = addslashes($email2);

// Let's add data to mysql database
$insert = mysql_query("insert into $table values ('NULL', '$username3','$password3', '$email3')")
or die("Ei saanud andmeid lisada ,sest ".mysql_error());
$insert2 = mysql_query("insert into $table2 values ('NULL', '2500000','2000', '1', '0', '0', '0', '0')")
or die("Ei saanud andmeid lisada ,sest ".mysql_error());
//Let's send user to index.php?id=10
header("Location:index.php?id=10");
} } } }
?>
Config.php

Code: Select all

<?
//my little and useless config 
$server = "localhost"; //server
$database = "testing"; //database name
$db_user = "root"; //username
$db_pass = "root"; //password
$table = "login"; //tabel name
$?>
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

This is vulnerable in several ways:

1. Use mysql_real_escape_string() (NOT addslashes, and NOT htmlspecialchars) on the variables before constructing a dynamic query. $_POST['username'] in index.php and reg.php allows SQL injection.

2. On a successful login, save the "loggedin" flag in the session, not the cookie. Right now, an attacker can create a cookie and hit indeks.php and he'll be let in.

3.

Code: Select all

setcookie("loggedin", "FALSE", time()+(0 * 1));
will in fact NOT log out the user ($_COOKIES['loggedin'] will still be set ... to "FALSE")

4. Location headers should use absolute URLs

Edit:
5.

Code: Select all

die("Ei saanud andmeid lisada ,sest ".mysql_error());
This is a bad habit, even if in your case it wouldn't display anything to the attacker due to ob_start().
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

I noticed a few other things:
  1. Use of $_GET/$_POST data blindly. i.e. no existence check done.
  2. Several references to $_GET or $_POST data with unquoted indices
  3. Use of short open tags. They should always be the long open tags.
  4. Unquoted HTML attribute values
  5. Missing indentation in code, making readability go down.
  6. No unset() on sensitive variable information. e.g. $db_pass.
boss01
Forum Newbie
Posts: 19
Joined: Sun May 27, 2007 3:31 pm

Post by boss01 »

Ok i will start patching the securty holes. But how i can fix 5 point in morder post.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

boss01 wrote:Ok i will start patching the securty holes. But how i can fix 5 point in morder post.
Don't die() with mysql_error(). Log the error somewhere. error_log() may be of interest.
boss01
Forum Newbie
Posts: 19
Joined: Sun May 27, 2007 3:31 pm

Post by boss01 »

Mordred 1-4 things fixed.
And i have one question to:
Is my code correct? Will that fix: 1. Use of $_GET/$_POST data blindly. i.e. no existence check done.

Code: Select all

<?php if (empty($_POST['username'])) {Here that stuff when $username is not set} else {Here code if $username is set} ?>
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

Yes.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Better use is_set(), empty() is correct in your concrete sutiation, but it has different semantics. What you want is is_set().
Let's see your updated code, no offense, but as a general rule security fixes may introduce new holes ;)
Post Reply