Does this login look secure and safe against SQL Injection Attacks.

[deathbyfire]

Login page:

<?php
session_start();
require_once('../Connections/prbc.php');

if(isset($_POST['login'])){
$username = '';
$password = '';

if (isset ($_POST['username']) && $_POST['username'] != '')

$username = $_POST['username'];

if(isset ($_POST['password']) && $_POST['password'] != '')

$password = $_POST['password'];

$username = mysql_real_escape_string( $username );
$password = mysql_real_escape_string( $password );

$db_password = md5($password);

mysql_select_db('prbcweb') or die(mysql_error());
$login = mysql_query("SELECT * FROM prbc_user WHERE `user_name` = '$username' AND `user_pass` = '$db_password'");
$row_login = mysql_fetch_array($login);
$row_login_total = mysql_num_rows($login);

if ($row_login_total == 1) {
$_SESSION['MM_Username'] = $row_login['user_name'];
$_SESSION['UID'] = $row_login['user_id'];
$_SESSION['auth_level'] = $row_login['user_access_level'];
echo "<script type=text/javascript>location.href='approver.php'</script>";

} elseif ($row_login_total <> 1) {
header("Location: login_2.php");
}
}
?>

Access Control on Pages:

<?php
session_start();
if (isset($_SESSION['MM_Username']) && ($_SESSION['auth_level'] <= '5')) {
$username = $_SESSION['MM_Username'];
}
else {
echo "<script type=text/javascript>location.href='login_2.php'</script>";
}

[alimadzi]

Only issue I have is using JavaScript to bounce unauthenticated users since JS can be disabled very easily. Your first script looks all right. I would recommend writing a function to handle that stuff though: test key in POST array, retrieve value from POST array, run mysql_real_escape_string(). Then that part would look something like this:

$username = getVar( 'username' );
$password = md5( getVar( 'password' ) );

...instead of...

if (isset ($_POST['username']) && $_POST['username'] != '')

$username = $_POST['username'];

if(isset ($_POST['password']) && $_POST['password'] != '')

$password = $_POST['password'];

$username = mysql_real_escape_string( $username );
$password = mysql_real_escape_string( $password );

I'll leave the coding of the getVar() function as an exercise for you.

One technique to look into for greater security by default would be parameterizing your queries so the quoting and escaping are handled automatically. PDO is a good solution for this. Good luck.

[deathbyfire]

I went to PHP's site and typed getvar in the search box and chose function and nothing came up. Can you point me to a good site that describes it and its use?

Thanks!!!

[alimadzi]

I went to PHP's site and typed getvar in the search box and chose function and nothing came up. Can you point me to a good site that describes it and its use?

Thanks!!!
I guess I wasn't clear. Actually getVar() is a made-up function. I was suggesting that you write it in order to handle the repetitive steps you're currently using to guard against SQL injections. Sorry about that confusion.

Let me know if you're still stuck and I'll provide a getVar() function for you.

[bradgrafelman]

Moved to Code Critique forum.

Also, as a sidenote, you might want to use [php] tag instead of the general [code] tags so that your code is properly highlighted.

[deathbyfire]

Okay, I have been reading a bit about functions. So, if I am building a site and use the same query at the top of each page to pull in the user information would I make a separate file and create a function with the query and then just call the function at the top of each page?

If so, how does this differ from includes?

Or do I have this totally wrong?

Thanks again for any help!

[alimadzi]

Yes, you're on the right track. You could create a new file called functions.php and put your new getVar() function in there. Or you could make a function called getUserInfo() that returns a $userInfo array with the username, password, and anything else you might want. Suppose you go the 2nd route. Your code would look like this...


<?php

require_once( 'functions.php' );

$userInfo = getUserInfo();

// the rest of your script

?>

[deathbyfire]

and is there an advantage to doing it that way versus just putting it in an include?

[alimadzi]

and is there an advantage to doing it that way versus just putting it in an include?
I'm not quite sure what you mean.

If you mean putting all your shared code in one file, I don't think it makes any difference really. Just a matter of personal coding style.

If you're talking about using include() instead of require(), then it does make a difference. Using require() or require_once() causes your application to fail if the file cannot be found. This is actually what you want to happen in this case because your code is trying to use functions found in that file. You app will terminate with a file not found error instead of trying to proceed without the needed functions.

And using require_once() instead of require() is necessary because functions cannot be defined more than once. That's basically what you would be doing if you used two different scripts each using require() on your functions.php file.

Please let me know if I haven't adequately answered your question.