login code with sessions

kclark

I have been using the following code for a login to view a webcast. It uses sessions. It is now not working, or at least I am getting an error:Warning: Wrong parameter count for session_destroy() in /usr/local/4admin/apache/vhosts/mysite.com/httpdocs/php/livelogin.php on line 7"
The only thing that I have changed is I have gone from php4 to php5. I am baffled. Anyway, here is my code.

<?

// Use session variable on this page. This function must put on the top of page.
session_start();

////// Logout Section. Delete all session variable.
      session_destroy("username");

$message="";

////// Login Section.
$Login=$_POST['Login'];
if($Login){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password']; // Encrypt password with md5() function. 


// Check matching of username and password.
$result=mysql_query("select * from videologin where username='$username' and password='$password' and live='1'");
if(mysql_num_rows($result)!='0'){ // If match.
session_register("username"); // Create session username.
header("location:live"); // Re-direct to main.php
exit;
}else{ // If not match.
$message="--- Incorrect Username, Password, or You Are Trying To View At An Incorrect Time  ---<br>
Please check our <u><videohelp.html target=blank>FAQ's and Help List</a></u> first for help.<br>
If you feel this is an error, and you need immediate access<br>
to the live video feed, please contact us immediately at<br>
, or try again.<br><br>
But PLEASE know that you MUST get the username<br>
and password directly from .<br><br>";
}

} // End Login authorize check.
?>
<br>  

<p>
<p align="center"><? echo $message; ?></p>
<form id="form1" name="form1" method="post" action="<? echo $_SERVER['PHP_SELF']; ?>">
<table align="center">
<p align="center">
Please Login below to view our live video, using the name and password given to you.<tr>
</p>
<td>User : </td>
<td><input name="username" type="text" id="username" ></td>
</tr>
<tr>
<td>Password : </td>
<td><input name="password" type="password" id="password" ></td>
</tr>
</table>
<p align="center">
<input name="Login" type="submit" id="Login" value="Login" >
</p>
</form>
<p align="center"><b>Please Note</b> that usernames and passwords will not work<br>until approximately 10 minutes prior to service time.</p>

dagon

as the manual says, session_destroy does not take any parameters

kclark

I have gotten rid of the error it looks like, but my page is not sending me to my live video page. Every time I put in the user name and password, and click submit I get the form again without an error message of any kind.

bradgrafelman

kclark wrote:
I have also tried it as session_destroy;

You mean literally "session_destroy;" ? If so, you're missing the parentheses on the function call.

kclark

no I mean session_destroy(); but read my updated post above, my form is not redirecting for some reason.

bradgrafelman

Here are some issues/suggestions:

Since use of the short tags ("<?" and "<?=(expression)?>") is discouraged, you should always use the full "<?php" opening tag in all of your PHP scripts.
Code like this:
```
$Login=$_POST['Login']; 
if($Login){ // If clicked on Login button. 
```
generates E_NOTICE level error messages when the external data you're attempting to access doesn't exist. Instead, it's better to use [man]isset/man or [man]empty/man to check before you ever access external data.
This code:
```
$password=$_POST['password']; // Encrypt password with md5() function. 
```
talks about using the MD5 function (by the way, the MD5 algorithm is not an "encryption" algorithm 😉) yet you never do. If the passwords in your database are indeed stored as MD5 hashes, then this is likely to be the main source of your current problem.
User-supplied data should never be placed directly into a SQL query string, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. Instead, you must first sanitize it with a function such as [man]mysql_real_escape_string/man (for string data).

EDIT: More comments below, since I accidentally hit submit too early...
You should indent your code properly so that it's easier for others (and yourself!) to read and analyze.
Try to avoid using "SELECT " queries and instead only SELECT the columns from which you actually need data. In this case, since you don't need any data, you could just select the number 1 or some simple arbitrary value like that.
You may or may not benefit from this (depending upon your indexes and/or how MySQL uses them), but if you're only expecting at most one row to be returned, it's not a bad idea to add a "LIMIT 1" clause to your query.
While PHP is very flexible/loose when it comes to variable types, note that this code:
```
if(mysql_num_rows($result)!='0'){
```
suggests that [man]mysql_num_rows/man returns a string (which it does not).
[man]session_register/man has been deprecated; instead, you should be using the $_SESSION superglobal array (manual page: [man]variables.session[/man]).
The "Location" HTTP header is supposed to contain a full URI, e.g. "Location: http://mysite.com/path/to/live" .

kclark

Since use of the short tags ("<?" and "<?=(expression)?>") is discouraged, you should always use the full "<?php" opening tag in all of your PHP scripts.

I fixed that, oversite on my part. Still doesn't work though.

Code like this:
$Login=$_POST['Login']; 
if($Login){ // If clicked on Login button. 
generates E_NOTICE level error messages when the external data you're attempting to access doesn't exist. Instead, it's better to use [man]isset/man or [man]empty/man to check before you ever access external data.

I am checking to see if the Submit button on the form at the bottom of the page has been pressed. I don't understand your suggestion. The way that I did it is the only way that I knew how. This code is word for word the same as on another site and it works flawlessly for me.

This code:
$password=$_POST['password']; // Encrypt password with md5() function. 
talks about using the MD5 function (by the way, the MD5 algorithm is not an "encryption" algorithm 😉) yet you never do. If the passwords in your database are indeed stored as MD5 hashes, then this is likely to be the main source of your current problem.
[*]User-supplied data should never be placed directly into a SQL query string, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. Instead, you must first sanitize it with a function such as [man]mysql_real_escape_string/man (for string data).

The username and passwords are supplied to my viewers from me. They are not md5. They are simple plain jane passwords. Where/How do I sanitize the data and is there a way to see if my code, not just this page can be attacked?

[*]You should indent your code properly so that it's easier for others (and yourself!) to read and analyze.

I should learn to indent more. I just starting writing php learning on my own since march. I have dabbled very little a couple years ago.

Try to avoid using "SELECT " queries and instead only SELECT the columns from which you actually need data. In this case, since you don't need any data, you could just select the number 1 or some simple arbitrary value like that.
[*]You may or may not benefit from this (depending upon your indexes and/or how MySQL uses them), but if you're only expecting at most one row to be returned, it's not a bad idea to add a "LIMIT 1" clause to your query.
[*]While PHP is very flexible/loose when it comes to variable types, note that this code:
if(mysql_num_rows($result)!='0'){
suggests that [man]mysql_num_rows/man returns a string (which it does not).

In this particular db, I only have 3 fields. Username, Password, and Live. That's it.

[man]session_register/man has been deprecated; instead, you should be using the $_SESSION superglobal array (manual page: [man]variables.session[/man]).

Is this a php4 thing or php5? Like I said above, I have this working great on another site of mine, but it is running php4. This code is running on php5.

bradgrafelman

kclark wrote:
I am checking to see if the Submit button on the form at the bottom of the page has been pressed. I don't understand your suggestion. The way that I did it is the only way that I knew how.

You're checking to see if it was pressed by directly accessing it. If that variables doesn't exist, you'd be attempting to assign the value of a non-existent array item to the $Login variable. In other languages, this can cause problems varying in moderate to severe (e.g. segmentation faults or something of that nature). In PHP, you fortunately only generate E_NOTICE level error messages.

Instead, you should first see if that external data exists in a safe manner, e.g. using [man]isset/man or [man]empty/man in the if() statement.

kclark wrote:
This code is word for word the same as on another site and it works flawlessly for me.

IMHO, nothing works "flawlessly" if you can't set error_reporting to E_ALL and have no errors logged. It will usually work (despite the E_NOTICE messages), however, simply thanks to how graciously PHP can handle such code (which is IMHO somewhat poorly written).

kclark wrote:
The username and passwords are supplied to my viewers from me. They are not md5.

Well of course they are, I wasn't expecting the users to go hash their passwords with the MD5 algorithm and then type in that hashed value.

The code comment suggests that you are to be hashing the password using MD5. This led me to assume that the passwords were thus stored in your database not in plaintext but as MD5 hashes, meaning that simply checking if "password='johndoe'" would always fail since you aren't hashing the user's password before querying the DB.

kclark wrote:
Where/How do I sanitize the data and is there a way to see if my code, not just this page can be attacked?

The "where" is before you use any user-supplied data in a SQL query. The "how" is a complex answer (though many of the complexities can be mitigated by learning how to use prepared statements). SQL data sanitization and security isn't a topic that you can learn in a sentence or two.

The PHP manual does have a brief introduction to the topic here: [man]security.database.sql-injection[/man].

kclark wrote:
In this particular db, I only have 3 fields. Username, Password, and Live. That's it.

Um... okay? Not sure what the relevance of any of that is? 😕

kclark wrote:
Is this a php4 thing or php5?

Both. While it was only "officially" deprecated since PHP 5.3, there have been warnings against using it since PHP 4.2. I say "officially" because that's when the PHP manual listed it; the PHP community as a whole probably decided it was a bad idea before that.

kclark

So do I just need to start from scratch or can I do some very simple mods to my script?

kclark

I changed parts of my code as reflected below. Still not working. I left my form alone, so I did not paste it here. I am lost can't figure out why it is not working anymore. I hightlighted the parts that I changed.

<?php

// Use session variable on this page. This function must put on the top of page.
session_start();

////// Logout Section. Delete all session variable.
[COLOR="Blue"]unset($_SESSION['username']);[/COLOR]

$message="";

////// Login Section.
$Login=$_POST['Login'];
[COLOR="blue"]if(isset($Login)){ // If clicked on Login button.[/COLOR]
$username=$_POST['username'];
$password=$_POST['password']; 
// Get a database object

// Check matching of username and password.
$result=mysql_query("select * from videologin where username='$username' and password='$password' and live='1'");
if(mysql_num_rows($result)!='0'){ // If match.
[COLOR="blue"]$_Session("username"); // Create session username.[/COLOR]
[COLOR="blue"]header("location:http://www.mysite.com/index.php/live"); [/COLOR]// Re-direct to main.php
exit;
}else{ // If not match.
$message="--- Incorrect Username, Password, or You Are Trying To View At An Incorrect Time  ---";
}

} // End Login authorize check.
?>

bradgrafelman

Er, this code is a bit misleading now:
```
////// Logout Section. Delete all session variable.
unset($_SESSION['username']);
```
Specifically, what the comment says doesn't match what the code does. If you want to clear all session data, a simple way to do that is to set $_SESSION equal to an empty array.
```
$Login=$_POST['Login'];
if(isset($Login)){ // If clicked on Login button.
```
$Login is guaranteed to be set, since you defined it on the line right before you call isset(). I was suggesting that you use [man]isset/man directly on the incoming data, e.g.:
```
if(isset($_POST['login']))
```
```
$_Session("username"); // Create session username.
```
Few things wrong with this code:
1. There is no variable called $_Session. I think you mean $SESSION instead.
2. Using parenthesis tells PHP you're trying to call a function, which are you aren't. $_SESSION is an array, and you access array indexes using square brackets, e.g. $array['index_name'].
3. You don't actually define the 'username' index by assigning it a value.

kclark

I believe that I have made the proper changes that you suggest, but when I enter a valid username & pwd, my page acts like it just refreshes and I get an empty login form again.

<?php

// Use session variable on this page. This function must put on the top of page.
session_start();

////// Logout Section. Delete all session variable.
unset($_SESSION['']);

$message="";

////// Login Section.
//$Login=$POST['Login'];
if(isset($POST['Login'])){ // If clicked on Login button.
$username=$POST['username'];
$password=$POST['password']; // Encrypt password with md5() function.

// Check matching of username and password.
$result=mysql_query("select * from videologin where username='$username' and password='$password' and live='1'");
if(mysql_num_rows($result)!='0'){ // If match.
$_SESSION['username']; // Create session username.

kclark

Just checking in to see if there is anyone with any ideas.

bradgrafelman

Here:
```
////// Logout Section. Delete all session variable.
unset($_SESSION['']);
```
you're trying to unset the array item identified by an empty string as the key. I highly doubt you use such a key, so that line is fairly meaningless. As I said above, if you want to clear out the session, simply set the $_SESSION superglobal array equal to an empty array.
```
$_SESSION['username']; // Create session username.
```
This line doesn't do anything (other than possibly cause an E_NOTICE error). It's the same as writing something like:
```
1+2;
```
If you're trying to create that array index (as the code comment suggests), then you have to assign it some value.
Same comments as above in regards to both the contradicting MD5 code comment as well as properly sanitizing user-supplied data to prevent SQL injection attacks and/or errors.

kclark

////// Logout Section. Delete all session variable.
unset($SESSION['']);

I thought would I used was an empty session to delete all sessions; so would this be

unset($_SESSION);

$SESSION['username']; // Create session username.

Would I use

$_SESSION['username'] = $username;

I will worry about the sql injections once I get my form to work properly.

bradgrafelman

kclark;10957191 wrote:
I thought would I used was an empty session to delete all sessions; so would this be
unset($_SESSION);

That would unset the entire $_SESSION superglobal array, making sessions unusable for the rest of the script.

Read my post(s) above again slowly:

bradgrafelman wrote:
simply set the $SESSION superglobal array

$_SESSION

bradgrafelman wrote:
equal to

bradgrafelman wrote:
an empty array

array()

bradgrafelman wrote:
.

kclark;10957191 wrote:
Would I use
$_SESSION['username'] = $username;

Assuming that $username is defined, then yes.

kclark

This is still not working. I am just getting my form refreshed it seems. I am also posting my code for the page that is to show my video if login is correct. It does check for session.

<?php

// Use session variable on this page. This function must put on the top of page.
session_start();

////// Logout Section. Delete all session variable.
unset($_SESSION[array]);

$message="";

////// Login Section.
//$Login=$_POST['Login'];
if($Login == "Login"){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password']; 



// Check matching of username and password.
$result=mysql_query("select * from videologin where username='$username' and password='$password' and live='1' LIMIT 1");
if(mysql_num_rows($result)!='0'){ // If match.
$_SESSION['username'] = $username; // Create session username.

header("location:http://www.site.com/index.php/live"); // Re-direct to main.php
exit;
}else{ // If not match.
$message="--- Incorrect Username, Password, or You Are Trying To View At An Incorrect Time  ---";
}

} // End Login authorize check.
?>
<br>  

<p>
<p align="center"><? echo $message; ?></p>
<form id="form1" name="form1" method="post" action="<?php echo $_SERVER['PHP_SELF']; ?>">
<table align="center">
<p align="center">
Please Login below to view our live video, using the name and password given to you.<tr>
</p>
<td>User : </td>
<td><input name="username" type="text" id="username" ></td>
</tr>
<tr>
<td>Password : </td>
<td><input name="password" type="password" id="password" ></td>
</tr>
</table>
<p align="center">
<input name="Login" type="submit" id="Login" value="Login" >
</p>
</form>
<p align="center"><b>Please Note</b> that usernames and passwords will not work<br>until approximately 10 minutes prior to service time.</p>

Main video page.

<?php
session_start(); // Use session variable on this page. This function must put on the top of page.
if(!session_is_registered("username")){ // if session variable "username" does not exist.
header("location:http://www.site.com/index.php/livelogin"); // Re-direct to index.php

?>
<center><?php include "livecode.php"; ?></center>

If for any reason you loose your connection, please refresh your browser to reconnect.

bradgrafelman

[man]session_is_registered/man is a deprecated function - don't use it. As the PHP manual states:

PHP Manual wrote:
Note: If $SESSION (or $HTTP_SESSION_VARS for PHP 4.0.6 or less) is used, use isset() to check a variable is registered in $SESSION.

kclark

I changed session_is_registered to this:

<?php

if(isset($_SESSION['username']))// if session variable "username" does not exist.

{
print "<center>";

include "livecode.php";
print "If for any reason you loose your connection, please refresh your browser to reconnect.</center>";
}
else
{
header("location:http://www.site.com/index.php/livelogin");
}

?>

bradgrafelman

If that's the entire code of the second page, you're now missing a call to [man]session_start/man.