unsubscribe/subscribe/forgotpassword security

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

murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

unsubscribe/subscribe/forgotpassword security

Post by murlopaz »

Hi everybody. I developed a members area on one of my websites.
I have a couple of security concerns though..

I have a unsubscribe option for a user that is logged in.

When the user click the unsubscribe button, he is sent a email containing a url that will be passed to the delete.php containing a hash of time+username+salt that is saved in a field of the user (in the db). This field is called $confirmation, and it is generated when the user signs up.
IF the user click on the url from the email the delete.php script finds $confirmation field for that particular user and deletes all the information about him as well as kills any sessions that are associated with that particular user.

The same idea is used when a user subscribes as well as when a user forget his password.

Does anybody see any security concerns that I should be aware of?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

On your end, there's nothing to really worry about unless your scripts have holes in them.
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

well one thing i noticed is that... if the user wants to change his password, then he could just go directly to the link provided in the email over and over again... and I don't want that...
Should I change $confirmation everytime a user changes his password?


Also, as far as script security goes:

1. I validate input
2. escape output
That's pretty much it... I am also keeping the id number of the user in the session variable. I suppose this is a bad idea.

My website is hosted on a shared server, should I be really worried about this?(i use dreamhost)
I tried to access somebody else's session on the server, and I was unsuccessful, so that's good news.

Also I don't have anything that would counter session fixation.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

murlopaz wrote:well one thing i noticed is that... if the user wants to change his password, then he could just go directly to the link provided in the email over and over again... and I don't want that...
Should I change $confirmation everytime a user changes his password?
That isn't a security problem, so that's your choice.
murlopaz wrote:My website is hosted on a shared server, should I be really worried about this?(i use dreamhost)
Shared servers have their own problems, but those are mostly for the host to worry about.
murlopaz wrote:I tried to access somebody else's session on the server, and I was unsuccessful, so that's good news.
That doesn't mean it's not possible.. but I'd bet that Dreamhost would void your account if you keep trying.
murlopaz wrote:Also I don't have anything that would counter session fixation.
Well, that may be something.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

If you want more than an educated guess at your security, post your code!

It is a long shot, but I don't like this:
This field is called $confirmation, and it is generated when the user signs up.
...
Should I change $confirmation everytime a user changes his password?
The password change challenge should be generated each time a user requests a password change and invalidated if he changes his password or after a short (1 day?) timeout period.

Your setup does not mitigate the risk of intrusion enough. Imagine an attack when someone can read your database, and assume the login credentials themselves are well protected ([plug="shameless"]Do you use double-salted hashes?[/plug]). The attacker still has a strong entry point - he can use the $confirmation tokens to change the passwords of ALL users or delete their accounts.
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


This is the script that emails the user a link that would delete him from the db.

unsubscribe.php

Code: Select all

<?
/************************GET unique Confirmation ID from DB*********************/
$connection=mysql_connect($hostname,$username,$password) OR DIE('DB Error:1 ' . mysql_error());
mysql_select_db("$dbName", $connection) or die('DB Error:2 ' . mysql_error());
$sql="SELECT confirmation 
          FROM users 
	  WHERE userID=".quote_smart($_SESSION["user"]);

//execute query
if (!($result = @ mysql_query ($sql, $connection)))
    die('DB Error:3 ' . mysql_error());
$row=@mysql_fetch_array($result);
$confirmation=$row["confirmation"];

//get sms_email value
$sql="SELECT sms_email from sms_emails WHERE userID=".quote_smart($_SESSION["user"]);

//execute query
if (!($result = @ mysql_query ($sql, $connection)))
    die('DB Error:3 ' . mysql_error());

$row=@mysql_fetch_array($result);
$sms_email=$row["sms_email"];
/************************GET unique Confirmation ID from DB*********************/

/************************UNSUBSCRIBE EMAIL BEGINS HERE**********************/
$from="abc";
$header = "";
$footer = "";
$subject = "ACCOUNT CONFIRMATION";
$body = "Hello. Please click on  the following link to unsubscribe your account 
		 from the system: blahblahblah/delete.php?id=".$confirmation;	   
//***change link later on to reflect the correct path***
$to=$sms_email;
$from="abc";
mail("$to", "$subject","$header\n  $body\n $footer\n", "from:$from");
/************************UNSUBSCRIBE EMAIL ENDS HERE************************/

echo "An email has been sent to you with further instructions. 


$_SESSION=array();
session_destroy();
?>
delete.php

Code: Select all

<?
$id=$_GET["id"];
//validate this !!!
/************************GET unique Confirmation ID from DB*********************/
$connection=mysql_connect($hostname,$username,$password) OR DIE('DB Error:1 ' . mysql_error());
mysql_select_db("$dbName", $connection) or die('DB Error:2 ' . mysql_error());
$sql="SELECT userID from users WHERE confirmation=".quote_smart($id);

//execute query
if (!($result = @ mysql_query ($sql, $connection)))
    die('DB Error:3 ' . mysql_error());
$row=@mysql_fetch_array($result);
$count=@mysql_num_rows($result);
if ($count==1)
{
	$id=$row["userID"];
	//deletes user's entry from users table
	$sql="DELETE from users WHERE userID=".quote_smart($id);
	if (!($result = @ mysql_query ($sql, $connection)))
		die('DB Error:3 ' . mysql_error());
	//deletes user's entry from user_role table
	$sql="DELETE from user_role WHERE userID=".quote_smart($id);
	if (!($result = @ mysql_query ($sql, $connection)))
		die('DB Error:3 ' . mysql_error());
	//deletes user's entry from sms_phones table
	$sql="DELETE from sms_phones WHERE userID=".quote_smart($id);
	if (!($result = @ mysql_query ($sql, $connection)))
		die('DB Error:3 ' . mysql_error());
	//deletes user's entry from sms_emails table
	$sql="DELETE from sms_emails WHERE userID=".quote_smart($id);
	if (!($result = @ mysql_query ($sql, $connection)))
		die('DB Error:3 ' . mysql_error());
/************************GET unique Confirmation ID from DB*********************/
	
	echo "Your have been successfully unsubscribed.";
}else if ($count==0)
{
    echo "Invalid ID.";
}else
{
    echo "Unexpected error, please contact the administrator";
}
?>

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Show us quote_smart() as well.
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

Code: Select all

function quote_smart($value) {
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }

   if (!is_numeric($value) || $value[0] == '0') {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

This part seems secure, there's just the question of sms_email, which should be cleaned of \r and \n in order to avoid email header injection. Are you doing this?

I'd put the quotes in the SQL statements and mysql_real_escape_string() in all ocasions - there's no problem to assign '123' to a INT field, but this is your coding style, not mine ;)

There is a possible problem (not security related) - if you quote_smart a value that hasn't come from $_GET, $_POST etc. and magic_quotes are enabled, you would blindly strip the slashes off it, which is not correct.
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

Thanks Mordred for your help.

sms_email is validated + escaped when the user subscribes...

There is one more concern I have. I use the confirmation for unsubscribe, subscription confirmation as well as when somebody forgets their password.

This approach seems clumsy to me...

Also can anybody illustrate how a hacker can use session fixation to impersonate a user?

I am planning on using https. This should exclude the possibility of the hacker capturing anything related to the session...

Are there other ways that a hacker can take advantage session fixation?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

As I said before, the confirmation token has to be generated ONLY when the user requests deletion or password change, and have a validity period. Since it is temporary, there's no problem to use it for both purposes.

About session hijacking/fixation and https, I suggest you find some reference papers and read the basics.
Session fixation != session hijacking, and https will (generally) NOT help against either.
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

what would be a good confirmation token?
hash of (username+time+salt).

Is that good enough?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

A random string of 8+ alphanumeric characters. Do not md5 a random int and cut 8 chars off it, it's not the same ;)
murlopaz
Forum Commoner
Posts: 60
Joined: Wed Oct 11, 2006 5:02 pm
Location: Baltimore, MD, USA

Post by murlopaz »

well there is a slight possibility that two users would get the same confirmation number...
Z3RO21
Forum Contributor
Posts: 130
Joined: Thu Aug 17, 2006 8:59 am

Post by Z3RO21 »

Also remember not to output mysql_error() to the end user. This can reveal relevant information to the user that could be used against you.

Also in your code for unsubscribe.php you have a code error.

Code: Select all

echo "An email has been sent to you with further instructions.


$_SESSION=array();
session_destroy();
?>
should be

Code: Select all

echo "An email has been sent to you with further instructions.";


$_SESSION=array();
session_destroy();
?>
Noting the added "; to the line:

Code: Select all

echo "An email has been sent to you with further instructions.";
Also look into session_regenerate_id() this function might be useful to session fixation.
Post Reply