Forum PHP.pl

Napisałem własne skrypty, działają i wgl, ale muszą zostać zabezpieczone. W związku z tym piszcie, czy napisałem w miarę ok, czy są rażące widoczne błędy, itp.
Plik logowanie.php

[PHP] pobierz, plaintext 
<?php 
sesion_start(); //rozpoczęcie sesji
 
$login = mysql_real_escape_string($_POST['login']);  //określenie zmiennej login, dodanie backshalsy
$haslo = mysql_real_escape_string(md5($_POST['haslo'])); //określenie zmiennej haslo, dodanie backshalsy,kodowanie md5
 
if ($login != "" && $haslo != ""){ //sprawdzanie czy zmienne nie są puste
 
$zapytanie = "SELECT * FROM `uzytkownicy` WHERE login="'. $login.'" and haslo="'.$haslo.'" "; //zapytanie
$uzytkownik = mysql_fetch_array(mysql_query($zapytanie) or die("Wystąpił nieoczekiwany błąd.")); //tworzymy tablicę
 
$_SESSION['login'] = $uzytkownik['login']; //określamy zmienną sesyjną login
$_SESSION['haslo'] = $uzytkownik['haslo']; //określamy zmienną sesyjną haslo
$_SESSION['id'] = $uzytkownik['id']; //określamy zmienną sesyjną id
echo "Zostałeś zalogowany jako: ".$_SESSION['login'].""; //jeśli wszystko ok, wyświetlamy login
}
else {
echo  'Podałeś błędne dane, spróbuj ponownie.';  //jeśli nie baj baj
}
 
?>
[PHP] pobierz, plaintext 

Czy wszystko robię ok ? Czy jest to odporne na SQJ injection ? Macie może jakieś sugestie ?
Czy w pliku tylko dla zalogowanych w zupełności wystarczy:

[PHP] pobierz, plaintext 
<?php
session_start();
  if (!isset($_SESSION['login']) || !isset($_SESSION['haslo'])) {
  header("location:index.php"); // Przekierowanie do index.php
}
?>
[PHP] pobierz, plaintext 

Ten post edytował marian2299 17.08.2009, 00:34:12

A czy `przelecenie mysql_real_escape_string` po pustych zmiennych może dać niepożądane skutki ?, if ($login != "" && $haslo != ""){ to po `przeleceniu` nie wystarczy ?

Co do tych wyrażeń, poczytam, ale jutro (ciężki dzień (IMG:style_emoticons/default/haha.gif) ).

A ogólnie ten kod może być ?

Czyli zostawić id i login? Będzie działać wszystko?

Podpinając się pod ten temat (gdyż trochę się nim sugerowałem), chciałem zapytać o wskazówki odnośnie takiego prostego skryptu, który wysmażyłem. Plik login.php pobiera dane od formularza w pliku index.php i wygląda następująco:

[PHP] pobierz, plaintext 
<?php
	ob_start();
	session_start();
	include ("db_connect.php");
 
	// sprawdzamy dane w tablicy $_POST
	if ((isset($_POST['username'])) && (isset($_POST['password']))) {
		if ((($_POST['username']) == "") || (($_POST['password']) == "")) {
			// dla pustych zmiennych w $_POST (uzytkownik nic nie wpisal) automatycznie wracamy do logowania
			header('Location: index.php');
		}
		else {
			$username = $_POST['username'];
			$password = sha1($_POST['password']);
			$zapytanie_do_bazy = sprintf("SELECT * FROM logowanie WHERE uzytkownicy_username='%s'", mysql_real_escape_string($username));
			$wynik_zapytania = mysql_query($zapytanie_do_bazy) OR die(mysql_error());
			$tablica_pomocnicza = mysql_fetch_assoc ($wynik_zapytania);
			if ($tablica_pomocnicza['uzytkownicy_username'] == $passwordd) {
				// haslo sie zgadza - możemy przejść do pliku plik_chroniony.php
				$_SESSION['zalogowany'] = true;
				header('Location: plik_chroniony.php');
			}
			else {
				// haslo sie nie zgadza - wracamy do logowania
				// czyli kierujemy teraz użytownika z powrotem do logowania, wyświetlając odpowiedni komunikat						
			}
		}
	}
	else {
		// jesli wystapilo bezposrednie odwolanie do pliku login.php, zamiast użycie formularza w pliku index.php, to kierujemy z powrotem do index.php
		header('Location: index.php');
	}
	ob_end_flush();
?>
[PHP] pobierz, plaintext 

Jeśli mogę prosić o jakąś opinię - jeśli coś jest źle, to jak poprawić?

pozdrawiam
Radek

Co źle? A sam sprawdzić nie możesz? Audyt? Giełda ofert.

Sprawdzam i działa. Chodzi mi tylko o podpowiedź odnośnie "dobrych praktyk programistycznych", czy nie popełniłem jakiegoś "faux-pas" w kodzie. Napisałem przecież "jeśli coś jest źle", a nie "co jest źle" (IMG:style_emoticons/default/smile.gif)

Bardziej zależało mi na poradzie typu "to-czy-tamto można zrobić lepiej, wydajniej, tu niepotrzebnie np. duplikujesz dane". Ogółem działać działa i jestem z siebie zadowolony, że opanowałem ten temat. Tylko żeby nie popadać w samozachwyt wolę się upewnić, że czegoś nie pochrzaniłem (IMG:style_emoticons/default/winksmiley.jpg)

Mam takie uwagi do obu Panów (IMG:style_emoticons/default/smile.gif)
@radabus:
Te if'y :

[PHP] pobierz, plaintext 
if ((isset($_POST['username'])) && (isset($_POST['password']))) {
   if ((($_POST['username']) == "") || (($_POST['password']) == "")) {
[PHP] pobierz, plaintext 

można zastąpić po prostu tym :

[PHP] pobierz, plaintext 
if(!empty($_POST['username']) && !empty($_POST['password'])) {
[PHP] pobierz, plaintext 

efekt będzie ten sam, a przynajmniej jeden warunek mniej do sprawdzania - tym bardziej ze w przypadku niepoprawnych danych oba if'y kierowaly do pliku index.php.

Ponadto - i to się tyczy do Was obu - walidacja danych (IMG:style_emoticons/default/exclamation.gif) WAŻNE jest założenie, że KAŻDE dane pochodzące z zewnątrz, a szczególnie GET, POST - są niebezpieczne ! Dlatego też (tak jak wcześniej proponował fifi209) - bez wyrażeń regularnych się nie obejdzie.

@marian2299 - przeanalizuj sobie kod radabus'a. Bezpieczne logowanie powinno wyglądać właśnie tak jak on to zrobił - na podstawie loginu wyciągamy hasło z bazy i porównujemy z tym, które podano w formularzu.
Niestety ale Twój kod nie jest bezpieczny pod względem SQL Injection. Wystarczy w polu gdzie podajesz login użytkownika wpisać: " ' OR 1=1 " lub coś podobnego i masz dostęp do systemu.

Ten post edytował Suh 19.08.2009, 21:00:19

@Suh: czyli samo

[PHP] pobierz, plaintext 
mysql_real_escape_string($username)
[PHP] pobierz, plaintext 

nie wystarczy? Najpierw trzeba wyrażeniami regularnymi sprawdzić, czy string przypadkiem nie zawiera niedozwolonych znaków i dopiero wtedy puścić go do MySQLa? Dobrze rozumiem? Bo jeśli mysql_real_escape_string nie wystarcza, to albo ja coś źle rozumiem, albo manual wprowadza w błąd (IMG:style_emoticons/default/winksmiley.jpg) Jest tam przecież napisane, że...


Samo napisanie sprawdzenia wyrażenia regularnego dla prostego ciągu znaków jak nazwa użytkownika nie powinno być trudne (chociaż jeszcze w pełni tego nie opanowałem), ale w takim razie po co używać mysql_real_escape_string? Takie dublowanie roboty wtedy jest (IMG:style_emoticons/default/winksmiley.jpg)

Ten post edytował radabus 19.08.2009, 22:42:23

Witam, nie będę zaczynał nowego tematu bo pytanie trochę ma do tego tematu , otóż przy skrypcie logowania , jak sprawdzam potem czy są isset zmienne $_POST , potem czy nie są puste i pasują do wyrażenia regularnego(preg_match) , potem w skrypcie ta metoda mysql_real_escape_string, to czy to mi da dosyć bezpieczny skrypt logowania ?

Ten post edytował kamillo121 19.08.2009, 22:59:02

Przeczytaj przyklejony wątek o bezpieczeństwie, dopiero potem pytaj... :X

Przeczytałem, i pytam (pytanie poprzednie ) Bo nie znalazłem odpowiedzi na moje pytanie (IMG:style_emoticons/default/sadsmiley02.gif) ...

Lepiej zrobić o jedno zabezpieczenie więcej, niż mniej.. Na pewno to nie zaszkodzi, a może co najwyżej poprawić sytuację.
Poza tym mysql_real_escape_string() może Ci co najwyżej sprawdzić czy nie ma w podanym ciągu takich znaków jak np. ' " czy coś podobnego. A wyrażenia regularne mogą ponadto np. sprawdzać czy w loginie nie ma cyfr lub podkreśleń czy innych takich znaków, których sobie po prostu nie życzysz.