Big security issue with PHP user script

[galahad2]

Hi,

I have a small login system whereby a user logs in and then is able to access the "customer" pages in a particular folder. It works fine in itself, but I've discovered a pretty big security problem- but can't see a way around it.

The script that handles login and redirection is as follows:

[PHP]
<?php
require_once ( 'settings.php' );

if ( array_key_exists ( '_submit_check', $_POST ) )
{
if ( $_POST['username'] != '' && $_POST['password'] != '' )
{
$query = 'SELECT ID, Username, Active, Password FROM ' . DBPREFIX . 'users WHERE Username = ' . $db->qstr ( $_POST['username'] ) . ' AND Password = ' . $db->qstr ( md5 ( $_POST['password'] ) );

if ( $db->RecordCount ( $query ) == 1 )
{
$row = $db->getRow ( $query );
if ( $row->Active == 1 )
{
set_login_sessions ( $row->ID, $row->Password, ( $_POST['remember'] ) ? TRUE : FALSE );
$userid = $row->Username;
header ( "Location: " . REDIRECT_AFTER_LOGIN . "?Username=" . $userid );
//header ( "Location: " . REDIRECT_AFTER_LOGIN );
}
elseif ( $row->Active == 0 ) {
$error = 'Your membership was not activated. Please open the email that we sent and click on the activation link.';
}
elseif ( $row->Active == 2 ) {
$error = 'You are suspended!';
}
}
else {
$error = 'Login failed!';
}
}
else {
$error = 'Please use both your username and password to access your account';
}
}
?>
[/PHP]

This is the line from the above which grabs the user's ID and does the forwarding to the landing page:

[PHP]
$userid = $row->Username;
header ( "Location: " . REDIRECT_AFTER_LOGIN . "?Username=" . $userid );
[/PHP]

Works great, but if the user then goes to an "Upload item" form (which they will need to do) this is where the problem can crop up.

Firstly, in the header of the landing page I pull in the username from the URL which was generated by the header above:

[PHP]
$user = $_GET['Username'];
[/PHP]

Then I echo this in the body of the landing page- i.e "Welcome to...., $user" so they have a personalised welcome.

Now, I have a number of links for various actions that logged-in users can make, and for the "Add an item" linking I've used this code so that the $user variable is carried through to the "Add an item" form page (I need to ensure that the user ID is added into the db when a new item is added):

[PHP]
<a href="addlisting.php?_User=<?php echo $user ?>"><img src="../addlisting.gif" width="100" height="45" alt="Add New Listing" /></a>
[/PHP]

The problem is, when the user reaches the addlisting.php page, he can in theory modify the URL, which is addlisting.php?_User=testuser. He can change the name of the user to whatever, and then upload a listing item.

This is a major issue as he can effectively pretend to be another user. Granted, most users won't think of doing this, but I need a way of preventing it.

At the same time I need a way of getting the $user variable "sent" to all the pages for logged-in users, so that whenever the user fills in a form etc. anywhere in that area, his username will be included in the form.

[Matthew1980]

Hi there galahad2,

Well on first read of your script, passing anything via the url can present problems, you could assign things to $_SESSION which would then be available throughout the scope of your script as it is a super global, unless I have miss-understood you, this would be the practical way to go, as this would be effectively 'hidden' from public consumption.

There is probably a way to go through .htaccess but I don't know very much of that ;)

Also:

<a href="addlisting.php?_User=<?php echo $user; ?>"><img src="../addlisting.gif" width="100" height="45" alt="Add New Listing" /></a>

I am hoping as the missing ; was a typo, just thought I would mention it ;)

Sorry I can't be more constructive,

Cheers,
MRb

[jatar_k]

how do you know from page to page whether this person has been authenticated?

and then how do you know exactly who they authenticated as?

if the above 2 questions are answered then I think you could just test that that username entered in the url is being entered by the person who uses the username.

[galahad2]

Thanks- yep, the missing ; was a typo...

So what would I assign to $_SESSION? Do I need to use it at the beginning when the user logs in, i.e:

$userid = $row->Username;
header ( "Location: " . REDIRECT_AFTER_LOGIN . "?Username=" . $userid );

Does the above code need to be changed? I'm guessing that $userid needs to be assigned to a session but not sure how...

Thanks

[Matthew1980]

Hi there galahad2,

Top of the main parsing file:-

<?php
error_reporting(E_ALL);//turn off when going live ;)
session_start();
//
.
.Rest of your code

$_SESSION['username'] = strip_tags($_POST['username']);//Clean and assign to aptly named var
?>
Then from that $_SESSION['username']; is available throughout your script.

When you have finished with the session, use session_destroy($_SESSION); and unset($_SESSION); to completely finish it.

Again, hope I have understood you correctly, Jatar_k has two very vaild points there, are you making sure that page-to-page your user is a valid user? Honestly though, having user info in the URL is opening your system up to insecurities - I use $_SESSION wherever I can, and then for added measure, I mod rewrite the URL, but this is down to preference I guess, just my opinion :)

Cheers,
MRb

[galahad2]

Thanks MRb... I'll give this a go and see if that works, I guess the above all needs to go in the login script at the beginning so that the session is set.

It does sound more secure that passing through the url... :)

[Matthew1980]

Hi there galahad2,

Well be sure to make a backup of anything as you don't want to change something and in case it goes wrong have nothing to go back to that's a known stable issue.

For me I always start the session in the main index file, then spider everything from there, gets rid of any ambiguity then IMO. Have fun anyway, and yes from what I understand using sessions is a more secure way of doing this; though I do suggest as any sensitive data, ie, passwords are md5() hashed so that NOTHING is visible.

Cheers,
MRb

[galahad2]

Hmm... tried changing the login.php, which is where the userid needs to be collected and set, but it's not passing anything through to the landing page.

This is the code I'm now using in the login file:

[PHP]
<?php
require_once ( 'settings.php' );

error_reporting(E_ALL);//turn off when going live
session_start(); // as per your suggestion

if ( array_key_exists ( '_submit_check', $_POST ) )
{
if ( $_POST['username'] != '' && $_POST['password'] != '' )
{
$query = 'SELECT ID, Username, Active, Password FROM ' . DBPREFIX . 'users WHERE Username = ' . $db->qstr ( $_POST['username'] ) . ' AND Password = ' . $db->qstr ( md5 ( $_POST['password'] ) );

if ( $db->RecordCount ( $query ) == 1 )
{
$row = $db->getRow ( $query );
if ( $row->Active == 1 )
{
set_login_sessions ( $row->ID, $row->Password, ( $_POST['remember'] ) ? TRUE : FALSE );
$userid = $row->Username;
header ( "Location: " . REDIRECT_AFTER_LOGIN );
}
elseif ( $row->Active == 0 ) {
$error = 'Your membership was not activated. Please open the email that we sent and click on the activation link.';
}
elseif ( $row->Active == 2 ) {
$error = 'You are suspended!';
}
}
else {
$error = 'Login failed!';
}
}
else {
$error = 'Please use both your username and password to access your account';
}
}
$_SESSION['userid'] = strip_tags($_POST['userid']);//Clean and assign to aptly named var
//I'm guessing that should be the last line... I'm using $userid as that's the name of the variable that pulls out the user id. It's not passing anything through now though...

[/PHP]

Luckily I did make a backup, just wondering why it's not working though...
?>

[galahad2]

Actually, looks as if adding exit(); after the header call has made it work.

Thanks again!

[Matthew1980]

Hi there galahad2,

Sorry for not getting back to this thread in time to answer you, but it looks like you figured it out in the end. Cool.

Yes, adding an exit directly after the header(); call is good practise, because this will stop anything executing downstream from the call. So something like this is a good thing to have:-

if(Your Logged in){
header("location: secure.php");
exit;
}
//carry on!

Pseudo code, but you see the logic.

Cheers,
MRb