Page 1 of 2

2 questions: if and session's

Posted: Wed May 30, 2007 4:40 pm
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

Re: 2 questions: if and session's

Posted: Wed May 30, 2007 4:47 pm
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.

Posted: Wed May 30, 2007 5:55 pm
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.

Posted: Wed May 30, 2007 7:33 pm
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.

Posted: Wed May 30, 2007 7:42 pm
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)

Posted: Wed May 30, 2007 10:42 pm
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.

Posted: Thu May 31, 2007 12:04 am
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.

Posted: Thu May 31, 2007 5:58 am
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
$?>

Posted: Thu May 31, 2007 7:07 am
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().

Posted: Thu May 31, 2007 7:24 am
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.

Posted: Thu May 31, 2007 7:32 am
by boss01
Ok i will start patching the securty holes. But how i can fix 5 point in morder post.

Posted: Thu May 31, 2007 7:41 am
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.

Posted: Thu May 31, 2007 10:58 am
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} ?>

Posted: Thu May 31, 2007 11:16 am
by superdezign
Yes.

Posted: Thu May 31, 2007 4:18 pm
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 ;)