Покритикуйте код авторизации

[skeletor]

Скажу сразу, код работает, просто интересно, может стоит где-то оптимизировать или поправить, как нужно.

Собственно имеем такую структуру:
connect.php - подключение к БД
functions.php - описание функций для обработки вводимых данных
index.php - страница с формой авторизации
login.php - авторизация
logout.php - выход
main.php - главная страница

Интересует правильно ли провожу авторизацию и выход.

login.php

<?php

include 'functions.php';
include 'connect.php';

if (isset($_COOKIE['name'])) 
{
    header("Location: main.php");
    exit(0);
} 

if (!isset($fPassword) or !isset($fUsername))
{
    header("Location: index.php");
    exit(0);   
}

$table_admin = "admin";
$table_domain_admins = "domain_admins";

$result = mysql_query ("SELECT password FROM $table_admin WHERE username='$fUsername' AND active='1'");
$row = mysql_fetch_row($result);   
     
$password = pacrypt ($fPassword,$row['0']);
       
    if ($password == $row['0'])
    {
        session_start();
        session_regenerate_id();
        $sessid = session_id();
        setcookie("name",$fUsername,time()+3600);
        setcookie("sessid",$sessid,time()+3600);       
        header("Location: main.php");
        exit(0);
    }

    else die('error set cookies');

mysql_close();

if (!isset($_COOKIE['name'])) 
{
    header("Location: index.php");
    exit(0);
} 

?>

logout.php

<?php

if (isset($_COOKIE['name']))
{
    session_start();
    setcookie('name', '', -1);
    session_destroy();  
}

header("Location: index.php"); 
 
?>

[Andkorol]

1.У вас register_globals=On что ли..??? :

if (!isset($fPassword) or !isset($fUsername))

2.Ваш SELECT лучше ограничить LIMIT 1 на всяк случай + проверять в WHERE нужно бы пару Username/Password,а не только Username (на случай если вдруг у вас случатся юзеры с одинаковым Username)

3.Данные перед запросом к БД обработайте хотя бы mysql_real_escape_string();

4.Не совсем понятно без описания функции вот это:

$password = pacrypt ($fPassword,$row['0']);

Формируем $password на основании пользовательского $fPassword и $row['0'] из БД - затем используем его в проверке на идентичность тому же $row['0'] :

if ($password == $row['0'])

- ну мож это я чего не знаю....

[Andkorol]

Также неплохо бы проверить сначала,вернул ли ваш запрос результат(mysql_num_rows() например) - а то без результата заругается ваш pacrypt()

[Виталий Желтяков]

В принципе правильно, только у Вас:
- register_globals=On.
- из примера непонятно откуда берётся $fPassword.
- logout.php на некоторых версиях FF может глючить.

[Andkorol]

из примера непонятно откуда берётся $fPassword.

Из register_globals=On и берётся,вероятно....)))))

[Johnatan]

5 минут на взлом такого скрипта. Облегчить взлом скрипта можно только написанием админского логина и пароля на главной странице.

[dekameron]

Johnatan, полностью согласен

Я раньше так делал

PHP код:

<?php 
//Куча require_once() для задания настроек, декларации функций, коннекта к БД и прочей хрени
if(isset($_POST['auth_ok'])){
    if(isset($id)){
        $_SESSION['er']='is_auth';
        header("location: /?");
        exit;
    }else{
        $login=check(strtolower($_POST['log']));
        $pass=md5($_POST['pswd']);
        $query=mysql_query("SELECT id FROM `users` WHERE login = '$login' AND pass = '$pass' LIMIT 1;")or die(mysql_error());
        if(mysql_num_rows($query)==0){
            $_SESSION['er']='err_auth';
            header("location: /?");
            exit;
        }else{
            $id=mysql_result($query,0,0);
            $_SESSION['id']=$id;
            $_SESSION['pass']=$pass;
            $_SESSION['login']=$login;
            setcookie('id',$id,$time+30672000,'/');
            setcookie('pass',$pass,$time+30672000,'/');
            setcookie('login',$login,$time+30672000,'/');
            $_SESSION['er']='auth_ok';
            header("location: /?");
            exit;
        };
    };
};
endpage(); //Дисконнект, выход
 ?>

Функции

PHP код:

<?php
function check($message){ 
    $message=str_replace("|","I",$message); 
    $message=str_replace("||","I",$message);
    $message=htmlspecialchars($message);                  
    $message=str_replace("'","'",$message);            
    $message=str_replace("\"",""",$message);  
    $message=str_replace("\$","$",$message);    
    $message=str_replace("$","$",$message);          
    $message=str_replace("\\","\", $message);                            
    $message=str_replace("`","", $message);  
    $message=str_replace("^","^", $message);   
    $message=str_replace("%","%", $message);  
    $message=str_replace(":",":",$message);  
    $message=preg_replace("|:|",":",$message,3); 
    $message=stripslashes(trim($message));               
    $message=mysql_real_escape_string($message);               
    return $message;
    //Не знаю, как подшаманить, чтобы вывело (&_#39; (без подчеркивания)), а не сразу символы, но суть ясна :)
}

function endpage(){
    mysql_close();
    exit;
}

?>

[Виталий Желтяков]

Я раньше так делал

А где здесь проверка кук? Где очищение сессии и убийство кук?

function check($message){  
    $message=str_replace("|","I",$message);  
    $message=str_replace("||","I",$message); 
    $message=htmlspecialchars($message);                   
    $message=str_replace("'","'",$message);             
    $message=str_replace("\"",""",$message);   
    $message=str_replace("$","$",$message);     
    $message=str_replace("$","$",$message);           
    $message=str_replace("","", $message);                             
    $message=str_replace("`","", $message);   
    $message=str_replace("^","^", $message);    
    $message=str_replace("%","%", $message);   
    $message=str_replace(":",":",$message);   
    $message=preg_replace("|:|",":",$message,3);  
    $message=stripslashes(trim($message));                
    $message=mysql_real_escape_string($message);                
    return $message; 
    //Не знаю, как подшаманить, чтобы вывело (&_#39; (без подчеркивания)), а не сразу символы, но суть ясна :) 
}

Подход неправильный. Тут проверка по белому списку, а надо использовать черный, т.к. взломщик может использовать различные спецсимволы.

[mv28jam]

dekameron
ааааааааааааааааааааа!!!!!!!!
Это вообще что такое?

PHP код:

$message=str_replace("$","$",$message);
$message=str_replace("^","^", $message);    
$message=str_replace("%","%", $message);
$message=str_replace(":",":",$message); 


Зачем вы меняете символ на такой же? Почему вы хотя-бы не делает это в цикле, хотя это вообще делать не надо.

PHP код:

$message=str_replace("","", $message); 


ХИТ всех времён и народов!

[Johnatan]

Сообщение от dekameron

Я раньше так делал

Мне Вас искренне жаль. Молюсь богам программеров, чтобы вы больше так не делали.