[PHP][MySQL] Bezpieczeństwo i poprawność logowania Opcje

[Darekxp]

Witam!

Czy takie logowanie jest w miare bezpieczne? Jeśli nie co jest nie tak? Za pomoc z góry dziękuję

[PHP] pobierz, plaintext 
<?php
 
  if($_SESSION['userId'] > 1)  
  { 
    header('Location: /index.php');
  }        
  else {
 
  $user      = filter($_POST['login']);
  $password  = md5(filter($_POST['password']));  
  $query     = mysql_query(' SELECT * FROM `users` WHERE userLogin="'.query($user).'" AND userPassword="'.query($password).'" AND userActive="yes" ');
 
  if($query && mysql_num_rows($query) == 1)
  {
    while($row = mysql_fetch_array($query))      
    {
        session_start();          
        if (!isset($_SESSION['initiate']))          
        {                   
          session_regenerate_id();              
          $_SESSION['initiate']           = true;
          $_SESSION['initiate']           = $new;
          $_SESSION['address_ip']         = $_SERVER['REMOTE_ADDR'];
          $_SESSION['userId']             = $row['userId'];
          $_SESSION['userLogin']          = $row['userLogin'];
          $_SESSION['userPassword']       = $row['userPassword'];                    
          $_SESSION['userActive']         = $row['userActive']; 
          $_SESSION['userGroup']          = $row['userGroup']; 
          $_SESSION['userName']           = $row['userName']; 
          $_SESSION['userSurname']        = $row['userSurname'];    
          $_SESSION['userCity']           = $row['userCity'];  
          $_SESSION['userPhoneNumber']    = $row['userPhoneNumber']; 
          $_SESSION['userEmail']          = $row['userEmail']; 
          $_SESSION['userGGNumber']       = $row['userGGNumber']; 
          $_SESSION['userAboutMe']        = $row['userAboutMe'];  
          $_SESSION['userDateBirth']      = $row['userDateBirth']; 
 
          $_SESSION['userHideDateBirth']  = $row['userHideDateBirth'];   
          $_SESSION['userHideSurname']    = $row['userHideSurname'];    
          $_SESSION['userHidePhoneNumber']= $row['userHidePhoneNumber']; 
          $_SESSION['userHideEmail']      = $row['userHideEmail']; 
          $_SESSION['userHideGGNumber']   = $row['userHideGGNumber']; 
          $_SESSION['userHideAboutMe']    = $row['userHideAboutMe'];  
 
          $_SESSION['userLastLogin']      = $row['userLastLogin'];  
          $_SESSION['userLastLoginIP']    = $row['userLastLoginIP'];
 
          mysql_query(' UPDATE `users` SET userLastLogin="'.query(date('Y-m-d H:i:s')).'"           WHERE userId="'.query($_SESSION['userId']).'" LIMIT 1');
          mysql_query(' UPDATE `users` SET userLastLoginIP="'.query($_SERVER['REMOTE_ADDR']).'"     WHERE userId="'.query($_SESSION['userId']).'" LIMIT 1');
 
          header('Location: /myAccount');                                 
        }
    }
  }
  else
    { 
      systemMessage('Logowanie', 'Prawdopodobnie podałeś zły login, lub hasło.', '', '', 'login', 'Spróbuj ponownie się zalogować', '', '', '', 'COMMUNIQUE_ERROR');
    }
}
 
?>
[PHP] pobierz, plaintext 

[vieri_pl]

Na pewno możesz posolić hasło, samo md5 to dość mało.

PS: Dlaczego masz dwa razy mysql_query, linia 48,49? Przecież to jedno zapytanie załatwi

[pyro]

Cały ten kod jest kompletnie bez sensu. Nie jest ani bezpieczny, ani wydajny, ani zgodny z zasadą DRY. Aż mi się nie chce wymieniać. Spróbuj to jeszcze raz napisać.

Ten post edytował pyro 20.04.2012, 04:24:56

[vieri_pl]

Pyro, ale kod nie jest bez sensu bo spełnia swoje zadanie, usera zaloguje, a to w jakim stylu... Tak na dobrą sprawę kazałeś człowiekowi napisać to jeszcze raz nie podając mu żadnych wskazówek jak to powinno wyglądać.

Od siebie dodam:
- jeśli do wartości do sesji mają taką samą nazwę jak rekord w bazie to wystarczy zrobić foreach($row as $key => $val) i dopisać wszystko do sesji.
- na pewno nie musisz pobierać wszystkiego (*) z bazy by zobaczyć czy user istnieje, do tego nie musisz pobierać nawet więcej niż dwóch rekordów
- $_SESSION['initiate'] najpierw true, a potem nie zdefiniowana zmienna $new? To notice.
- Zamiast funkcji date w PHP w zapytaniu możesz użyć NOW()

[Niktoś]

Przechowywanie wszystkiego w sesji,nie jest ani bezpieczne, ani wydajne.

[PanGuzol]

Korzystasz z $_SESSION w 3 linii ale session_start() masz dopiero w 17.

[Van Pytel]

Po co to:

[PHP] pobierz, plaintext 
AND userPassword="'.query($password)
[PHP] pobierz, plaintext 

Lepiej pobrać login i hasło usera z bazy (WHERE login = login), a potem dopiero sprawdzić w php czy hasło równa się haśle (if...)

[PanGuzol]

Po co to:

[PHP] pobierz, plaintext 
AND userPassword="'.query($password)
[PHP] pobierz, plaintext 

Lepiej pobrać login i hasło usera z bazy (WHERE login = login), a potem dopiero sprawdzić w php czy hasło równa się haśle (if...)

Lepiej przenieść jak najwięcej logiki do bazy danych. Czyli od razu w zapytaniu dać porównania do loginu i hasła, warto jeszcze do tego zrobić index unique dla loginu.