Basic Authentication

[augathra]

I wrote a login system and i'm hoping someone will check it for flaws.

require_once 'includes/common.inc.php';
require_once 'includes/connect.inc.php';
session_start();
$action = addslashes($_GET['action']);
if($action == 'logout') {
        session_destroy();
}
$status = '';
$signin = $_POST['signin'];
if(eregi('[~`!@#$%^&*()-+=|\'";:.,?/\\]', $_POST['login'])) {
    $invalid = '<font color="#FF000"><b>Invalid characters.</b></font>';
}
if(!$invalid) {
$login = stripslashes($_POST['login']);
$password = addslashes($_POST['password']);
$password = md5($password);

if($signin) {
    if(empty($login)) {
        $status .= '<font color="#FF0000"><b>Empty login, please fill in the text box.</b></font><br>';
        $continue = false;
    }
    if(empty($_POST['password'])) {
        $status .= '<font color="#FF0000"><b>Empty password, please fill in the text box.</b></font><br>';
        $continue = false;
    }
    if($continue !== false) {
        $SELECT = "SELECT * FROM users WHERE login = '$login' AND password = '$password' LIMIT 1";
        $QUERY = mysql_query($SELECT) or die($error);
        if($a = mysql_fetch_array($QUERY)) {
            $user_id = $a['id'];
            if(($a['login'] !== $login) && ($a['password'] !== $password)) {
                $session_create = false;
                $status .= '<font color="#FF0000"><b>Invalid login/password combination. Please try again.</b></font><br>'; 
            } else {
                $_SESSION['user_id'] = $user_id;
                Redirect('http://mysite.com/');
            }
        } else {
            $status = '<font color="#FF0000"><b>Invalid login/password combination. Please try again.</b></font><br>';
        }
    }
}
} 

[augathra]

Also, if anyone else has a basic login sys they wanna show, please do so, and I realize there are many out there.

[dbalsdon]

have you tried testing it yet? if so, were there any problems with it, did any errors appear??? could you maybe post them, so that we know what to look for?

[augathra]

No, everthing seemed fine. It's a login system, so I am really looking for hacking flaws.

[jbr]

Hi

I don't know what this script is part of, but I get the sense that your service contains multi pages, so I think some logic needs to be adjusted!

One of the most important things you should always do, is to make your code easy to modify or update without the need to update every script in the service!

Session control is the heart of any protected service, by centralizing your sessions control you will make your total script better, because keeping each major control separate will help keep your code truly organized!


Lets look at your script!

Start with this line...

session_start(); 

This should be replaced and placed in a include file! Where your session control will be maintained!

session control should contain the following....

1. is there already a active session?
2. if there is should they redirected to a service page?
3. is the session cookie based or is it passed via a URL query!
4. if it's URL based, you must define the session strings that will need to be added to all URL strings to maintain the session!
5. if there is no session create a new one
6. then begin the session!

By doing this you will cover all the logic that your service needs and also allow for updates, like changing over to database controlled sessions!


Now lets look at other things in your script!

$action = addslashes($_GET['action']); 
if($action == 'logout') { 
        session_destroy(); 
} 

assigning a $var to a already declared $var is wasteful and not needed! Also always assign the session to an empty array() before destroying it!

if ( isset ( $_GET['action'] ) && $_GET['action'] == 'logout' )
{
    $_SESSION = array ();
    session_destroy (); 
} 

Again, assigning a $var to a already declared $var is wasteful and not needed!

every time you do this, you are using another memory address

$signin = $_POST['signin']; 

Try to use Perl type regex(s), they are more system friendly

if(eregi('[~`!@#$%^&*()-+=|\'";:.,?/\\]', $_POST['login'])) { 
    $invalid = '<font color="#FF000"><b>Invalid characters.</b></font>'; 
} 

also you did not define 'invalid', which allows for anyone to pass it a value to your script! Which also would allow anyone to by-pass part of your script!

$invalid = '';

if ( !preg_match ( "/^[a-z0-9_]+$/is", $_POST['login'] ) )
{
    $invalid = '<font color="#FF000"><b>Invalid characters.</b></font>';
} 

This next if() could be by-passed because 'invalid' was not defined!

if(!$invalid) { 

There are other problems, including your database query, and more un-needed assign of var(s)!


J!

[augathra]

Thank you for the reply! I have a few questions i hope you will answer.

also you did not define 'invalid', which allows for anyone to pass it a value to your script! Which also would allow anyone to by-pass part of your script!

If the !eregi returned true, $invalid would not be empty anymore. The next piece is if(!$invalid) {}, which was supposed to be if(!empty($invalid)) { }.
Wouldn't this prevent the user from putting in those characters?

I am fairly new to $_SESSION stuff, so newbie session question.

is there already a active session?

The only thing i'm looking for is $_SESSION['user_id'], wouldn't this check for active sessions?

If you meant not making them login if one already exists, I still had to add that. I should have stated that.

[ssx-gun]

If the !eregi returned true, $invalid would not be empty anymore. The next piece is if(!$invalid) {}, which was supposed to be if(!empty($invalid)) { }.
Wouldn't this prevent the user from putting in those characters?

Its out of scope right now. Define it before the if statement
eg

$invalid = "";
  if (... 

yea it was already posted srry didnt see but yea its out of scope it had to be defined before it goes into the if.