9

I have the following code designed to begin a session and store username/password data, and if nothing is submitted, or no session data stored, redirect to a fail page.

session_start();
if(isset($_POST['username']) || isset($_POST['password'])) {
    $username = $_POST['username'];
    $password = $_POST['password'];
    $_SESSION['username'] = $username;
    $_SESSION['password'] = $password;
}

if(isset($_SESSION['username']) || isset($_SESSION['password'])){
    $navbar = "1";
    $logindisplay = "0";
    $username = $_SESSION['username'];
    $password = $_SESSION['password'];
} else {
    header('Location:http://website.com/fail.php');
}

$authed = auth($username, $password);
if( $authed == "0" ){
    header('Location:http://website.com/fail.php');
}

Its not working the way it should and is redirecting me to fail even though i submitted my info and stored it in the session. Am i doing something wrong?

NOTE the authed function worked fine before i added the session code.

Pascal MARTIN
  • 395,085
  • 80
  • 655
  • 663
mrpatg
  • 10,001
  • 42
  • 110
  • 169

7 Answers7

25

what about using this to setup session 

session_start();
if( isset($_POST['username']) && isset($_POST['password']) )
{
    if( auth($_POST['username'], $_POST['password']) )
    {
        // auth okay, setup session
        $_SESSION['user'] = $_POST['username'];
        // redirect to required page
        header( "Location: index.php" );
     } else {
        // didn't auth go back to loginform
        header( "Location: loginform.html" );
     }
 } else {
     // username and password not given so go back to login
     header( "Location: loginform.html" );
 }

and at the top of each "secure" page use this code:

session_start();
session_regenerate_id();
if(!isset($_SESSION['user']))      // if there is no valid session
{
    header("Location: loginform.html");
}

this keeps a very small amount of code at the top of each page instead of running the full auth at the top of every page. To logout of the session:

session_start();
unset($_SESSION['user']);
session_destroy();
header("Location: loginform.html");
tf.rz
  • 1,347
  • 6
  • 18
  • 47
Tim
  • 964
  • 2
  • 15
  • 30
  • What does `session_regenerate_id` do here? – richardaum Aug 25 '15 at 14:55
  • @Richard probably an attempt at evading session fixation – Marki Mar 26 '16 at 12:27
  • 1
    If the session_regerate_id isn't used the session id is constant until the browser is closed or session is expired even if the page is refreshed. An attacker can get their own session id by visiting the site, sends it as ?SID=SESSIONID to the victim, victim uses the link without knowing and login, meaning this session id is now authorized to access to the site, since it's the attacker's session id, attacker can access to the site as the victim until the session id expired (which take some time) – Don Dilanga Oct 05 '17 at 11:18
  • 1
    when session_regerate_id is used,it generates a new session id, destroys the old session id, passes its value to the new session id whenever the page is refreshed.So now victim accesses to the site with the attacker's link as in above scenario, the victim grants access to attacker's session id but as soon as it's granted,as the page loads a new session is created,old session is destroyed, values in old session is passed to the new session. so now if the attacker tried to login with the session id sent to the victim,they can't access it anymore because it's old session is long gone. – Don Dilanga Oct 05 '17 at 11:27
9

First, don't store the password in the session. It's a bad thing. Second, don't store the username in the session until after you have authenticated.

Try the following:

<?php

session_start();

if (isset($_POST['username']) && isset($_POST['password'])) {
    $username = $_POST['username'];
    $password = $_POST['password'];
    $authed = auth($username, $password);

    if (! $authed) {
        header('Location: http://website.com/fail.php');
    } else {
        $_SESSION['username'] = $username;
    }
}

if (isset($_SESSION['username'])) {
    $navbar = 1;
    $logindisplay = 0;
} else {
    header ('Location: http://website.com/fail.php');
}
hobodave
  • 28,925
  • 4
  • 72
  • 77
  • Well if you have to pass the username/password to a 3rd party API, then I guess you don't want to escape it. I'd leave that to them. – hobodave Aug 07 '09 at 06:27
3

Just some random points, even though they may not actually pertain to the problem:

  • Don't store the password in plaintext in the session. Only evaluate if the password is okay, then store loggedIn = true or something like that in the session.

  • Check if the password and the username are $_POSTed, not || (or).

  • Don't pass password and username back and forth between $password and $_SESSION['password']. Decide on one place to keep the data and leave it there.

  • Did you check if you can store anything at all in the session? Cookies okay etc...?

To greatly simplify your code, isn't this all you need to do?

if (isset($_POST['username'] && isset($_POST['password'])) {
    if (auth($_POST['username'], $_POST['password'])) {
        $_SESSION['user'] = /* userid or name or token or something */;
        header(/* to next page */);
    } else {
        // display "User credentials incorrect", stay on login form
    }
} else {
    // optionally: display "please fill out all fields"
}
deceze
  • 510,633
  • 85
  • 743
  • 889
  • The password is only used for accessing a 3rd party API through the site. The auth basically uses the 3rd parties site for authentication, therefore i have to pass the password off to the api (in plain text). Any alternative suggestions will be greatly appreciated though, im obviously new at this. – mrpatg Aug 07 '09 at 06:26
  • Okay, that depends on your specifics, unless you clarify those it's hard to suggest anything. Still, do you have to store it in the session? I'm guessing `authed()` does the 3rd party check? Do you have to do that more than once? – deceze Aug 07 '09 at 06:33
  • i do need to authorize more than once, just a couple of times, enough to upset the flow of the site if user has to keep re-entering info – mrpatg Aug 07 '09 at 06:42
1

Here are a few other things, which may or may not help you, by the way :

  • Do you have error_reporting on ? (see also)
  • Do you have display_errors on ?
  • Is session_start the first thing you are doing in your page ? There must be nothing output before
  • Are the cookies created on the client-side ?
  • header Location indicates the browser it has to go to another page ; it doesn't stop the execution of the PHP script. You might want to (almost always anyway) add "exit" after it.
Pascal MARTIN
  • 395,085
  • 80
  • 655
  • 663
  • good point on the sessions coming first, didnt know that, thanks – mrpatg Aug 07 '09 at 06:41
  • You're welcome :-) Actually, it's a bit more complicated than that : session_start sends cookies ; cookies are sent as HTTP headers ; HTTP headers can only be sent when no output has been sent ; so, session_start should be used before any output is generated. (there's an exception, if you are using output_buffering ; but you can't rely on it) – Pascal MARTIN Aug 07 '09 at 06:50
  • thats really good information to know actually, never would have thought. So sessions are basically server managed cookies? – mrpatg Aug 07 '09 at 07:01
  • Not really : "you" (well, PHP) must keep a link between a user and the session file on the disk ; as every request in HTTP is independant of the others, it's generally done with a cookie that store the session's identifier. And when PHP receives that cookie, it knows which session correspond to the user. – Pascal MARTIN Aug 07 '09 at 07:08
1

Headers are not function calls. They put a directive into the HTTP headers, and the last one to execute is the one which will be processed. So let say if you have something like this

if ($bAuthed)
{
     header("location: login.php");
}

// error case
header("location: error-login.php");

You will always be redirected to error-login.php no matter what happens. Headers are not function calls!

Extrakun
  • 19,057
  • 21
  • 82
  • 129
0

The solution to my specific problem above

session_start();
if(isset($_POST['username']) || isset($_POST['password'])){
$username = $_POST['username'];
$password = $_POST['password'];
$_SESSION['username'] = $username;
$_SESSION['password'] = $password;
}

if(isset($_SESSION['username']) || isset($_SESSION['password'])){
$navbar = "1";
$logindisplay = "0";
$username = $_SESSION['username'];
$password = $_SESSION['password'];
$authed = auth($username, $password);
if( $authed == "0" ){
header('Location:http://website.com/fail.php');
}
} else {
header('Location:http://website.com/fail.php');
}
mrpatg
  • 10,001
  • 42
  • 110
  • 169
0

Don't use else section in second if statement.

session_start();

if(isset($_POST['username']) || isset($_POST['password'])) {

    $username = $_POST['username'];

    $password = $_POST['password'];

    $_SESSION['username'] = $username;

    $_SESSION['password'] = $password;

}

if(isset($_SESSION['username']) || isset($_SESSION['password'])){

    $navbar = "1";

    $logindisplay = "0";

    $username = $_SESSION['username'];

    $password = $_SESSION['password'];

}

$authed = auth($username, $password);

if( $authed == "0" ){

    header('Location:http://website.com/fail.php');

}
sjaustirni
  • 3,056
  • 7
  • 32
  • 50
amit kumar
  • 1