Pomoc - Szukaj - Użytkownicy - Kalendarz
Pełna wersja: $_SESSION i Uwierzytelnianie - pytanie
Forum PHP.pl > Forum > PHP
MGreg
Witam.
Męczę się nad bezpiecznym systemem uwierzytelniania i proszę ocenić, czy takie coś będzie bezpieczne:
  1. <?php
  2.  
  3. if(isset($_POST['login']) and isset($_POST['haslo']) and $_GET['action']=='zaloguj')
  4. {
  5. $query = mysql_query("select * from `users` where `login`='".mysql_real_escape_string($_POST['login'])."' and `haslo`='".mysql_real_escape_string(md5($_POST['haslo'])).'"');
  6. if(mysql_num_rows($query)>0)
  7.  {
  8.  $_SESSION['logged'] = 1;
  9.  $_SESSION['username'] = $_POST['login'];
  10.  }
  11. else
  12. {
  13. $_SESSION['logged']=0;
  14. {
  15. }
  16.  
  17.  if($_SESSION['logged']==1)
  18. {
  19.  /* JAKIŚ TAM PANEL ADMINISTRACYJNY */
  20. }
  21.  
  22. ?>


Głównie chodzi mi o to, czy ktoś może zmienić wartość tablicy sesji $_SESSION['logged'] na 1 i tym samym zdobyć uprawnienia admina.
marcio
Ja bym zmienil jak juz cos jedna rzecz zamiast dawac mysql_num_rows() wez sobie za pomoca mysql_fetch_assoc() sprawdzaj jeszcze raz dane to raz a dwa dodal bym else z header'em na google gdy ktos poda zle dane wtedy nie bedzie nic widzial etc ogolnie jak trzymasz identifikator w url to generuj za kazdym razem nowe id dla sesji i potem na kazdej podstawie sprawdzaj czy jest dany login z sesji w bazie
MGreg
Umieściłem tylko przykład, dlatego nie dodawałem else z informacją typu "błędny login lub hasło" a identyfikator sesji przecież nie jest przekazywany w url-u, no chyba, że coś przeoczyłem smile.gif
pyro
jeszcze zabezpieczenie przed XSS by można było ^^
Shili
Nie jest, ale czy dyrektywa session.use.cookies.only jest włączona? Jeśli nie identyfikator pójdzie przez adres w przypadku gdy cookie nie zostanie przyjęte. Także regeneracja nie jest aż tak zbędna, jakby się wydawało, chociaż dzisiaj praktycznie większość serwerów wymusza sesje przez ciacha ^^

@up
A gdzie tu masz wypisywanie jakichkolwiek danych wprowadzonych przez użytkownika?
MGreg
Cytat(Shili @ 4.06.2008, 19:10:55 ) *
@up
A gdzie tu masz wypisywanie jakichkolwiek danych wprowadzonych przez użytkownika?


Nie umieszczałem formularza, bo chodzi mi głównie o kod php smile.gif Ale jak to ma się na coś zdać to dodałbym tam np.
  1. <form action="login.php?action=login" method=post>
  2. login <input type=text name=login><br>
  3. hasło <input type=password name=haslo<br>
  4. <input type=submit value=zaloguj>
  5. </form>

smile.gif
Shili
Nie nie, to było do @pyro. Powinnam była wyraźniej zaadresować winksmiley.jpg
pyro
Cytat(Shili @ 4.06.2008, 19:10:55 ) *
Nie jest, ale czy dyrektywa session.use.cookies.only jest włączona? Jeśli nie identyfikator pójdzie przez adres w przypadku gdy cookie nie zostanie przyjęte. Także regeneracja nie jest aż tak zbędna, jakby się wydawało, chociaż dzisiaj praktycznie większość serwerów wymusza sesje przez ciacha ^^

@up
A gdzie tu masz wypisywanie jakichkolwiek danych wprowadzonych przez użytkownika?


No tak... to co pokazał autor to jest pewnie cały skrypt, prawda Shili?
Shili
Nieprawda, ale przekazywane jest tylko login i hasło (bo i po co więcej do logowania), co można podejrzeć na dodanym przez autora kodzie. Zresztą nie do końca poprawnym, ale to inna bajka. Zresztą...

Bez zobaczenia reszty kodu można gdybać, bo może być tam brak zabezpieczenia do każdego możliwego błędu, autor może nawet wypisywać wszystkie swoje hasła. Ale po co gdybać, jeśli wyraźnie dostaliśmy taki fragment kodu do analizy, jaki MGreg chce by zanalizowano?

Co najwyżej można poprosić o resztę i wtedy się odnosić winksmiley.jpg
MGreg
Co do XSS stosuję strip_tags(); i chyba to mi wystarczy smile.gif Jutro wstawię poprawioną wersję i będziemy dyskutować dalej smile.gif
LonelyKnight
Cytat(pyro @ 4.06.2008, 21:15:32 ) *
No tak... to co pokazał autor to jest pewnie cały skrypt, prawda Shili?



To jeszcze powiedz autorowi jak powinien zabezpieczyć serwer bo przecież gdzieś to musi uruchomić sciana.gif Autor pisze o systemie autoryzacji a Ty opowiadasz o XSS jakby to miało cokolwiek wspólnego.

Wracając do tematu to zależy co rozumiesz pod słowem "bezpieczny"... Ogólnie przy systemach autoryzacji powinieneś wziąć pod uwagę jeszcze takie rzeczy jak Brute-force i zastosować np.

1. Po 3 nieudanych próbach - kolejne próby dostępne co minutę.
2. Max. 10 nieudanych prób logowania, potem blokada konta, aktywacja poprzez mail do właściciela.

Poza tym, już niezwiązane z brute-force:

1. Zastosować SSL.
2. Uważać na tzw. replay attack poprzez nieużywanie danych, które umożliwiają trwały dostęp do zasobów. Szczególnie tzw. trwały login. Jeśli już używasz takich zabawek to nie trzymać w ciastkach hasła i loginu, nawet zhashowanych, tylko zastosować dodatkowy identyfikator zamiast loginu + token zamiast hasła.
3. Przy logowaniu zastosować hasła maskowane, klawiaturę ekranową...

No i oczywiście SQL Injection.

Wszystko zależy od tego jak bardzo "bezpieczny" chcesz mieć system autoryzacji.

Jeśli chodzi bezpośrednio o kod, który podałeś to:

1. Nie używaj Select *...
2. Dodaj LIMIT do zapytania
3. Nie sprawdzaj mysql_num_rows($query)>0 tylko mysql_num_rows($query)===1 -> zalogowany
4. Dodaj ograniczenie długości dla wprowadzanego hasła i loginu.
5. Nie trzymaj w sesji nazwy użytkownika tylko np. wspomniany już losowy identyfikator...

..i to tyle z rzeczy, które teraz przyszły mi do głowy.
MGreg
Zobaczcie czy teraz jest lepiej. Dodałem sprawdzanie długości loginu i hasła, wstawiłem session_regenerate_id();, dodałem limit do zapytania, wywaliłem zapisywanie loginu do sesji i dodałem wylogowywanie.

  1. <?php
  2. include('config.php');
  3. pol_mysql();
  4.  
  5. if($_GET['action']=='logout')
  6. {
  7. if($_SESSION['logged']==1)
  8. {
  9. $_SESSION['logged']=0;
  10. echo "<b>zostałeś wylogowany<b>, jeśli nie zostaniesz przeniesiony na stronę logowania kliknij <a href=login.php>tutaj</a>";
  11. header("refresh: 5,login.php");
  12. }
  13. else
  14. {
  15. echo "nie byłeś zalogowany!";
  16. }
  17. }
  18.  
  19. if(!isset($_SESSION['logged'])){$_SESSION['logged']=0;}
  20.  
  21. if(isset($_POST['login']) and isset($_POST['haslo']))
  22.  {  
  23.  if(strlen($_POST['login'])<25 and strlen($_POST['haslo'])<25)
  24.  {  
  25.  $query = mysql_query("select `username`,`user_password` from `phpbb_users` where `username`='".mysql_real_escape_string($_POST['login'])."' and `user_password`='".mysql_real_escape_string(md5($_POST['haslo']))."' limit 1");
  26.  
  27.  if(mysql_num_rows($query)===1)
  28.  {  
  29.  $_SESSION['logged'] = 1;  
  30.  }
  31.  else
  32. {
  33. echo "Błędny login lub hasło";
  34. $_SESSION['logged']= 0;
  35. }
  36.  }  
  37.  else
  38. {
  39. echo "Zbyt długi login lub hasło";
  40. }  
  41.  }
  42.  
  43.  if($_SESSION['logged']==1)
  44.  {  
  45.  echo "Jakiś tam panel admina <a href=login.php?action=logout>Wyloguj</a>";  
  46.  }  
  47.  
  48.  
  49.  
  50. if($_SESSION['logged']==0 and $_GET['action']!=='logout')
  51. {
  52. ?>
  53. <form action="login.php" method=post>
  54. login <input type=text name=login><br>
  55. hasło <input type=password name=haslo><br>
  56. <input type=submit value=zaloguj>
  57. </form>
  58. <?
  59. }
  60.  
  61. ?>


Czy teraz jest lepiej?
Cotter
Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
5. Nie trzymaj w sesji nazwy użytkownika tylko np. wspomniany już losowy identyfikator...


Dlaczego trzymanie nazwy użytkownika w sesji może być niebezpieczne?
LonelyKnight
Cytat(Cotter @ 5.06.2008, 11:22:53 ) *
Dlaczego trzymanie nazwy użytkownika w sesji może być niebezpieczne?


Dlatego, że w niektórych przypadkach można ujawnić dane sesji użytkowników. Jeśli atakujący to zrobi i zobaczy jak sesja wygląda, co zawiera, którego użytkownika dotyczy... to będzie miał spore pole do popisu. Szczególnie trzeba uważać na współdzielonych hostingach.
MGreg
Wracając do tematu... czy może ktoś obiektywnie ocenić, czy ten mój poprawiony system uwierzytelniania jest lepszy niż poprzedni i czy trzeba w nim coś zmienić?
LonelyKnight
Może być.


session_regenerate_id(true) zamist session_regenerate_id().
pyro
Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
To jeszcze powiedz autorowi jak powinien zabezpieczyć serwer bo przecież gdzieś to musi uruchomić sciana.gif Autor pisze o systemie autoryzacji a Ty opowiadasz o XSS jakby to miało cokolwiek wspólnego.


Autor pyta o ten kawałek kodu co podał, a tam NIE MA zabezpieczenia przed XSS, dlatego o tym powiedziałem.

Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
1. Nie używaj Select *...

Niby dlaczego nie?
Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
2. Dodaj LIMIT do zapytania

Co to ma dać? Dłuższe zapytanie?
Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
3. Nie sprawdzaj mysql_num_rows($query)>0 tylko mysql_num_rows($query)===1 -> zalogowany

W przypadku tego kawałka kodu co to za różnica? Lecz powinno się też sprawdzać czy użytkownik już istnieje.
Cytat(LonelyKnight @ 4.06.2008, 22:02:40 ) *
5. Nie trzymaj w sesji nazwy użytkownika tylko np. wspomniany już losowy identyfikator...

A co? Uważasz, że to niebezpieczne?


Cytat
Dlatego, że w niektórych przypadkach można ujawnić dane sesji użytkowników. Jeśli atakujący to zrobi i zobaczy jak sesja wygląda, co zawiera, którego użytkownika dotyczy... to będzie miał spore pole do popisu. Szczególnie trzeba uważać na współdzielonych hostingach.


To może nie używajmy wogóle baz danych, plików txtowych ani wogóle niczego, bo w niektórych przypadkach można je ujawnić?

w
wlamywacz
Nie Select * tylko wybrane pola czyli Select pole1, pole2. Chodzi o to aby w przypadku ataku luki itp. można było wyciągnąć dane z tych pól a nie np. z pola password itp. Limit to samo w razie ataku pobierze jednego usera a nie całą tablice userów. A co do sesji to lepiej dmuchać na zimne
pyro
Cytat(wlamywacz @ 5.06.2008, 17:55:39 ) *
Nie Select * tylko wybrane pola czyli Select pole1, pole2. Chodzi o to aby w przypadku ataku luki itp. można było wyciągnąć dane z tych pól a nie np. z pola password itp.


Czy chociaż sam rozumiesz co tu napisałeś ^^?
Cytat(wlamywacz @ 5.06.2008, 17:55:39 ) *
Limit to samo w razie ataku pobierze jednego usera a nie całą tablice userów.

Co to za różnica, tak czy inaczej mozna pobrac całą tablicę userów.
Cytat(wlamywacz @ 5.06.2008, 17:55:39 ) *
A co do sesji to lepiej dmuchać na zimne


aha... czyli potwierdzają się moje słowa panie "włamywacz"?

Cytat
To może nie używajmy wogóle baz danych, plików txtowych ani wogóle niczego, bo w niektórych przypadkach można je ujawnić?
Shili
Dalej nie ma tam żadnej wzmianki o tym, że autor chce wyświetlać tekst pochodzący z formularza. Już nawet napisałam, że może tam spokojnie umieścić swoje wszystkie hasła, ale póki tego nie zrobi nie ma po co męczyć go stwierdzeniami: pamiętaj, nie wypisuj swoich haseł na stronie tongue.gif

Jedna uwaga - jeśli action jest logout, to mimo wszystko wykona się dalszy kawałek kodu. Co prawda pewnie nic się nie stanie, ale po co maszyna ma mielić coś, czego użytkownik nie ma na myśli.
Spokojnie użyj tam else.
empathon
http://www.acros.si/papers/session_fixation.pdf

Prosto z manuala, naprawdę warto tam zaglądać snitch.gif
LonelyKnight
Cytat(pyro @ 5.06.2008, 16:49:34 ) *
Autor pyta o ten kawałek kodu co podał, a tam NIE MA zabezpieczenia przed XSS, dlatego o tym powiedziałem.


W takim razie, powiedz mi proszę, jak chciałbyś przeprowadzić atak XSS na ten kawałek kodu, który tam jest podany.

Cytat(pyro @ 5.06.2008, 16:49:34 ) *
To może nie używajmy wogóle baz danych, plików txtowych ani wogóle niczego, bo w niektórych przypadkach można je ujawnić?


Ehh... nie mam zamiaru Cię przekonywać, chyba nie wiesz za bardzo o czym piszesz, a ja nie mam ani chęci ani czasu na takie zabawy.

Btw. pisze się "w ogóle"
To jest wersja lo-fi głównej zawartości. Aby zobaczyć pełną wersję z większą zawartością, obrazkami i formatowaniem proszę kliknij tutaj.
Invision Power Board © 2001-2024 Invision Power Services, Inc.