Czy takie zabezpieczenia są wystarczające ? Opcje

[sheriff]

Jako iż to mój pierwszy post, witam wszystkich serdecznie (IMG:style_emoticons/default/smile.gif)

Napisałem prosty skrypt do rejestracji użytkownika. Chcę się dowiedzieć jakie błędy popełniłem i czy takie zabezpieczenia są wystarczające.
Z góry dziękuję (IMG:style_emoticons/default/winksmiley.jpg)

[PHP] pobierz, plaintext 
<?php
$error = ' ';
$fail = 0;
 
if (!isset($_POST['login'])){
 echo '
 <form method="post" action="register.php">
   <input type="text" name="login" /><br><br>
   <input type="password" name="password"/><br>
   <input type="password" name="rpassword"/><br><br>
   <input type="submit" value="Gotowe"/>
  </form>
 ';
$fail = 1; 
}
else{
$login = $_POST['login'];
$pass = $_POST['password'];
$rpass = $_POST['rpassword'];
$passmd5 = md5($pass);
mysql_connect('localhost', 'root', 'krasnal') or die('MySql fatal error.');
mysql_select_db('sheriffengine') or die('MySql fatal error.');
 
  if (strlen($login) < 4)$error .= '<br>- login musi składać się z conajmniej 5 znaków';
 
  if (strlen($pass) < 5)$error .= '<br>- hasło musi składać się z conajmniej 6 znaków';     
 
  if ($pass !== $rpass)$error .= '<br>- podane hasła muszą być takie same';
 
 
  if (preg_match("/[^A-z0-9_-]/", $login)==1)$error .= '<br>- login  może składać się wyłącznie z liter i cyfr';
 
 
  if (preg_match("/[^A-z0-9_-]/", $pass)==1 || preg_match("/[^A-z0-9_-]/", $rpass)==1 )$error .= '<br>- hasło  może składać się wyłącznie z liter i cyfr';
 
  if(mysql_num_rows(mysql_query("select * from users where user_login='".htmlspecialchars($login."'")))) $error .= '- użytkownik o podanym loginie istnieje, wybierz inny login';
 
 if ($error !== ' '){
   echo '<br>Rejestracja się nie powiodła. Zastosuj się do poniższych wskazówek:';
   $fail = 1;
    }
 
echo $error;
 
}
 
if ($fail == 0){
	mysql_query("insert into users values(NULL, '".htmlspecialchars($login)."', '".$passmd5."')"); 
  echo 'Witaj '.$login.'! Rejestracja przebiegła pomyślnie.';
}
 
?>
 
[PHP] pobierz, plaintext 

[l0ud]

W pierwszym zapytaniu ' znajduje się w htmlspecialchars(). To błąd, który pokazuje przy okazji, że to zabezpieczenie w obecnej formie jest do... niczego (IMG:style_emoticons/default/winksmiley.jpg)

Bo skoro zapytanie działa mimo tego, oznacza to, że pojedyncze cudzysłowy ' nie są zamieniane przez htmlspecialchars(). A skoro nie są no to mamy dziurę (IMG:style_emoticons/default/winksmiley.jpg)

Rady:
- nie polecam używania htmlspecialchars() przed wrzucaniem czegokolwiek do bazy. Funkcję tą stosuj najlepiej tuż przed wyświetleniem czegoś wprowadzonego przez użytkownika (najlepiej bezpośrednio w echo '') - aby zapobiec XSS.
- zamiast htmlspecialchars() do zapytań w bazie użyj mysql_real_escape_string aby zabezpieczyć się przed SQL Injection
- skoro ładnie łączysz ciągi za pomocą . (zamiast wstawiać zmienne bezpośrednio w nich - pomiędzy " ") zacznij używać pojedynczych cudzysłowów ' ' zamiast podwójnych " ". W zapytaniach SQL natomiast ciągi opakuj " " zamiast w ' '. Słowem - zamień miejscami ' i " w zapytaniach.


Powinno wyglądać to np. tak:

[PHP] pobierz, plaintext 
#
if(mysql_num_rows(mysql_query('select * from users where user_login="'.mysql_real_escape_string($login).'"'))) $error .= '- użytkownik o podanym loginie istnieje, wybierz inny login';
[PHP] pobierz, plaintext 

A... i md5() jest już do niczego (IMG:style_emoticons/default/winksmiley.jpg) . Zamiast tego użyj przynajmniej sha1. Można jeszcze dodać tzw. sól do hasła - poszukaj.

Ten post edytował l0ud 24.06.2010, 21:51:15

[haahh]

1. Ładniej będzie sprawdzać błędy tak: (wtedy niepotrzebna jest zmienna $fail)

[PHP] pobierz, plaintext 
if(empty($error)){
 
//dodajemy do bazy danych
 
}
else{
echo '<br>Rejestracja się nie powiodła. Zastosuj się do poniższych wskazówek:';
echo $error;
}
[PHP] pobierz, plaintext 

2. W preg_match możesz już sprawdzać, czy ma dobrą długość np. tak (ale to jak chcesz):

[PHP] pobierz, plaintext 
preg_match('/^[A-Z \'.-]{2,20}$/i', $login) // od 2 do 20 znaków
[PHP] pobierz, plaintext 

3. Wystarczy:

[PHP] pobierz, plaintext 
if(preg_match("/[^A-z0-9_-]/", $login))
 
zamiast 
 
if(preg_match("/[^A-z0-9_-]/", $login)==1)
[PHP] pobierz, plaintext 

4. Osobiście wszystkie dane od użytkownika najpierw trimuje, później mysql_real_escape_string i ewentualnie strip_tags (ale tutaj masz wyrażenia regularne).

[PHP] pobierz, plaintext 
$trimmed = array_map('trim', $_POST); // jest łatwiej bo zamiast trim($_POST['first_name']) piszesz:
$trimmed['first_name'];
[PHP] pobierz, plaintext 

5. Hasło najlepiej zabezpieczyć jakąś solą/ziarnem/czy jak to inaczej nazywają indywidualnym dla każdego usera (sporo o tym na forum)
6. Ja bym dał formularz na dół oddzielony od kodu php, tak by się wyświetlał jeżeli będą jakieś błędy. Kiedy wszystko jest ok używać exit();, ale to znowu zależy od gustu.

[Fifi209]

Odsyłam do tematu: Temat: Bezpieczenstwo skryptow PHP

btw. wyrażenia masz źle napisane, możesz tutaj użyć odpowiedniego ctyle_ zamiast ich.

[tehaha]

- przede wszystkim wszelkie dane otrzymane od użytkownika i użyte w zapytaniu wkładaj do mysql_real_escape_string()
- nie wiem po co używasz htmlspecialchars() skoro chcesz zezwalać tylko na litery i cyfry..może sprawdź najpierw w manualu co robi ta funkcja
- czy to preg_match() na pewno działa? bo coś mi nie gra w tych wzorcach