[skrypt] Logowanie Opcje

[sebekzosw]

Witam!

Mam taki skrypt logowania opartego o sesje - moje pytanie brzmi: Czy taki skrypt jest bezpieczny?:

[PHP] pobierz, plaintext 
<?php
    function DodawanieDoBazy($ciag) {
        $wynik = stripslashes(strip_tags(addslashes(trim($ciag))));
        $wynik = str_replace(array("\"", "'", "\\", '\"', "\'", "<", ">", "&nbsp;"), array("&quot;", "'", "\", "&quot;", "'", "&lt;", "&gt;", " "), $wynik);
        $wynik = preg_replace(array("/\=/","/\#/","/\sOR\s/"), "", $wynik);
        return $wynik;
    }
 
 
    if(isset($_POST["logowanie"]) && isset($_POST['login']) && isset($_POST['login'])) {
        $logon_failure = '';
        if(empty($_POST["login"])) { $logon_failure .= "Wpisz login<br />"; }
        if(empty($_POST["password"])) { $logon_failure .= "Wpisz hasło<br />"; }
        $login = DodawanieDoBazy($_POST["login"]);
        $password = md5($_POST["password"]);
 
        if(empty($logon_failure)) {
            if(!mysql_fetch_row(mysql_query("SELECT * FROM users WHERE login='".$login."' AND pass='".$password."'"))) {
                $logon_failure .= "Nieprawidłowy login lub hasło";
            } elseif(!mysql_fetch_row(mysql_query("SELECT * FROM users WHERE login='".$login."' AND activity='1'"))) {
                $logon_failure .= "Konto jest nieaktywne";
            }
 
            if(empty($logon_failure)) {
                $_SESSION["login"] = $login;
                 $_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
                 $_SESSION['browser'] = $_SERVER['HTTP_USER_AGENT'];
            } else {
                echo $logon_failure;
            }
        } else {
            echo $logon_failure;
        }
    }
    echo "<div id=\"menu\">";
    if(UZYTKOWNIK) {
        echo "Zalogowany jako ".$_SESSION["login"];
    } else {
 
 
?>
    <form action="" method="post" id="logowanie">
        <fieldset>
            <dl>
                <dt>Login</dt>
                 <dd><input type="text" name="login" id="login" value="Login" onblur="if(this.value=='') this.value='Login';" onfocus="if(this.value=='Login') this.value='';" /></dd>
 
                <dt>Hasło:</dt>
                 <dd><input type="password" name="password" id="password" value="Hasło" onblur="if(this.value=='') this.value='Hasło';" onfocus="if(this.value=='Hasło') this.value='';" /></dd>
            </dl>
        </fieldset>
        <div style="float:right;"><input type="submit" name="logowanie" value="Zaloguj się" onclick="WyslijFormularz('logowanie'); return false;" /></div>
    </form>
    <img src="http://test.php.pl/www/gfx/login_loading.gif" id="ladowanie" alt="" style="display:none;float:left;" />
    <div id="wynik"></div>
 
    <p><a href="/rejestracja/">Rejestracja</a>
        <br />
        <a href="/przypomnij/">Przypomnij hasło</a>
 
 
 <?
 }
 ?>
 </div>
[PHP] pobierz, plaintext 

Wszystkie sugestie na temat polepszenia mile widziane

Ten post edytował erix 19.08.2009, 23:10:18

Powód edycji: [erix] przeniosłem

[Quantum]

[PHP] pobierz, plaintext 
ie"]) && isset($_POST['login']) && isset($_POST['login'])
[PHP] pobierz, plaintext 

w jakim celu sprawdzasz 2 razy ? dodatkowo dziwna nazwa funkcji, DodawanieDoBazy, lepiej byłoby FiltrujZapytanie, choć i tak jestem zwolennikiem nazewnictwa ang. używaj http://www.php.net/mysql_real_escape_string.
Dlaczego raz używasz ' a raz " ? zastąp cudzysłów apostrofem, zwiększa to szybkość działania skryptu.
Co do bezpieczeństwa.. MD5 ? ta funkcja skrótu teoretycznie w nowych aplikacjach pojawiać się nie powinna, zastąp ją SHA-1. Od siebie dodam, że lepiej napisałbyś to obiektowo, proceduralne - gorzej się czyta, po 2 tygodniach przerwy nie będziesz wiedział jak on nawet działał, OOP daje większe możliwości, na ten przykład PDO - nie musiałbyś martwić się o bezpieczeństwo, zapytania są tam filtrowane. Dodatkowo rozbudowa takich skryptów jest o wiele szybsza i prostsza.

[PHP] pobierz, plaintext 
if(!mysql_fetch_row(mysql_query("SELECT * FROM users WHERE login='".$login."' AND pass='".$password."'"))) {
[PHP] pobierz, plaintext 

zastąp tym, będzie wydajniej

[SQL] pobierz, plaintext 
SELECT count(*) FROM users WHERE login='".$login."' AND pass='".$password."'
[SQL] pobierz, plaintext 

Ten post edytował sniffer32 20.08.2009, 06:14:23

[sebekzosw]

Poprawiłem już to co mówiłem, tylko nie wiem gdzie zastosować mysql_real_escape_string I zamiast czego to użyć

I dobrze zrobiłem z tym??:

[PHP] pobierz, plaintext 
<?php
if(!mysql_result(mysql_query("SELECT count(*) FROM users WHERE login='".$login."' AND pass='".$password."'"), 0)) {
        		$logon_failure .= "Nieprawidłowy login lub hasło";
        	}
?>
[PHP] pobierz, plaintext 

Ten post edytował sebekzosw 20.08.2009, 08:32:31

[Quantum]

1. wystarczy zajrzeć do manuala, przykład 3, widać czarno na białym jak używać i do czego mysql_real_escape_string służy
2. sprawdź czy działa

[drake88]

I dobrze zrobiłem z tym??:

[PHP] pobierz, plaintext

1. <?php
2. if(!mysql_result(mysql_query("SELECT count(*) FROM users WHERE login='".$login."' AND pass='".$password."'"), 0)) {
3. $logon_failure .= "Nieprawidłowy login lub hasło";
4. }
5. ?>

[PHP] pobierz, plaintext

Jak najbardziej dobrze.

[sebekzosw]

Trochę poprawiłem całe logowanie dodając nową opcję, co sądzicie teraz o takim skrypcie:

[PHP] pobierz, plaintext 
<?php
   	session_start();
 
   	function DodawanieDoBazy($ciag) {
    	$wynik = stripslashes(strip_tags(addslashes(trim($ciag))));
		$wynik = str_replace(array("\"", "'", "\\", '\"', "\'", "<", ">", " "), array(""", "'", "\", """, "'", "<", ">", " "), $wynik);
        $wynik = preg_replace(array("/\=/","/\#/","/\sOR\s/"), "", $wynik);
        return $wynik;
    }
 
	if(isset($_POST["logowanie"]) && isset($_POST["login"])) {
		$logon_failure = "";
		if(empty($_POST["login"]) OR $_POST["login"] == "Login") { $logon_failure .= "Wpisz login<br />"; }
		if(empty($_POST["password"]) OR $_POST["password"] == "Hasło") { $logon_failure .= "Wpisz hasło<br />"; }
        $login = DodawanieDoBazy($_POST["login"]);
        $password = sha1($_POST["password"]);
 
        if(empty($logon_failure)) {
        	if(!mysql_result(mysql_query("SELECT count(*) FROM users WHERE login='".$login."' AND pass='".$password."'"), 0)) {
        		mysql_query("UPDATE users SET login_failed=login_failed+1, date_last_failed_login='".time()."' WHERE login='".$login."' LIMIT 1"); //Dodawanie +1 do obecnej liczby błędnych logowań
        		$zapytanie = mysql_fetch_assoc(mysql_query("SELECT * FROM users WHERE login='".$login."'"));
        		if($zapytanie["login_failed"] > 3) {
        			mysql_query("UPDATE users SET activity='0' WHERE login='".$login."' LIMIT 1"); //Deaktywacja konta po 3 błędnych próbach logowania
        			$logon_failure = "Konto zostało zdeaktywowane, ponieważ liczba prób logowania była większa niż 3. Aby aktywować konto, kliknij <a href=\"\">tu</a>";
        		} else {
        			$logon_failure .= "Nieprawidłowy login lub hasło";
        		}
        	} elseif(!mysql_result(mysql_query("SELECT count(*) FROM users WHERE login='".$login."' AND activity='1'"), 0)) { //Sprawdzamy czy konto jest aktywne
        		$logon_failure .= "Konto jest nieaktywne";
        	}
 
        	if(empty($logon_failure)) {
        		$zapytanie = mysql_fetch_assoc(mysql_query("SELECT * FROM users WHERE login='".$login."'"));
            	mysql_query("UPDATE users SET last_login='".$zapytanie["penultimate_login"]."', penultimate_login='".time()."' WHERE id='".$zapytanie["id"]."' LIMIT 1"); //Aktualizacja danych o ostatnim logowaniu
				mysql_query("UPDATE users SET login_failed='0' WHERE login='".$login."' LIMIT 1"); //Resetowanie liczby błędnych logowań
				$_SESSION["login"] = $login;
	 			$_SESSION["ip"] = $_SERVER["REMOTE_ADDR"];
 				$_SESSION["browser"] = $_SERVER["HTTP_USER_AGENT"];
 			} else {
        		echo $logon_failure;
        	}
        } else {
			echo $logon_failure;
		}
	}
    echo "<div id=\"menu\">";
	if(UZYTKOWNIK) {
		echo "Zalogowany jako ".$_SESSION["login"];
	} else {
 
 
?>
	<form action="" method="post" id="logowanie">
		<fieldset>
	    	<dl>
	    		<dt>Login</dt>
	     		<dd><input type="text" name="login" id="login" value="Login" onblur="if(this.value=='') this.value='Login';" onfocus="if(this.value=='Login') this.value='';" /></dd>
 
	    		<dt>Hasło:</dt>
	     		<dd><input type="password" name="password" id="password" value="Hasło" onblur="if(this.value=='') this.value='Hasło';" onfocus="if(this.value=='Hasło') this.value='';" /></dd>
	    	</dl>
		</fieldset>
		<div style="float:right;"><input type="submit" name="logowanie" value="Zaloguj się" onclick="sWyslijFormularz('logowanie'); return false;" /></div>
	</form>
	<img src="http://test.php.pl/www/gfx/login_loading.gif" id="ladowanie" alt="" style="display:none;float:left;" />
	<div id="wynik"></div>
 
	<p><a href="/rejestracja/">Rejestracja</a>
		<br />
        <a href="/przypomnij/">Przypomnij hasło</a>
 
 
 <?
 }
 ?>
 </div>
[PHP] pobierz, plaintext 

Ten post edytował sebekzosw 24.08.2009, 14:52:14

[drake88]

Może najpierw powiedz, jaką opcję dodałeś? ^^

[sebekzosw]

Deaktywacja konta po 3 błędnych próbach logowania

[Fifi209]

[PHP] pobierz, plaintext 
$wynik = stripslashes(strip_tags(addslashes(trim($ciag))));
$wynik = str_replace(array("\"", "'", "\\", '\"', "\'", "<", ">", " "), array(""", "'", "\", """, "'", "<", ">", " "), $wynik);
$wynik = preg_replace(array("/\=/","/\#/","/\sOR\s/"), "", $wynik);
[PHP] pobierz, plaintext 

Jaki w tym sens widzisz?

addslashes doda \
strislashes usunie \
str_replace jest nie potrzebny tak samo preg_replace

Użyj po prostu mysql_real_escape_string?

I jeszcze to:

[PHP] pobierz, plaintext 
if(UZYTKOWNIK) {
[PHP] pobierz, plaintext 

Gdzie definiujesz tą stałą?

Ten post edytował fifi209 27.08.2009, 13:41:15

[l0ud]

Cześć

Cała funkcja DodawanieDoBazy() jest zupełnie niepotrzebna, najlepszym i najprostszym zabezpieczeniem przed SQL injection oraz XSS jest przepuszczenie ciągu do dodania do bazy przez mysql_real_escape_string, a następnie - przy wyświetlaniu (a nie dodaniu do bazy) htmlspecialchars.

W tym kodzie zamiast

[PHP] pobierz, plaintext 
$login = DodawanieDoBazy($_POST["login"]);
[PHP] pobierz, plaintext 

Użyj po prostu

[PHP] pobierz, plaintext 
$login = mysql_real_escape_string($_POST["login"]);
[PHP] pobierz, plaintext 

(Albo przepuszczaj login przez mysql_real_escape_string bezpośrednio w zapytaniu)

[Fifi209]

Cześć

Cała funkcja DodawanieDoBazy() jest zupełnie niepotrzebna, najlepszym i najprostszym zabezpieczeniem przed SQL injection oraz XSS jest przepuszczenie ciągu do dodania do bazy przez mysql_real_escape_string, a następnie - przy wyświetlaniu (a nie dodaniu do bazy) htmlspecialchars.

W tym kodzie zamiast

[PHP] pobierz, plaintext 
$login = DodawanieDoBazy($_POST["login"]);
[PHP] pobierz, plaintext 

Użyj po prostu

[PHP] pobierz, plaintext 
$login = mysql_real_escape_string($_POST["login"]);
[PHP] pobierz, plaintext 

(Albo przepuszczaj login przez mysql_real_escape_string bezpośrednio w zapytaniu)

Po prostu to sobie daruj takie posty.

Napisałeś to co ja...(tylko w innych słowach)

[l0ud]

Po prostu to sobie daruj takie posty.

Napisałeś to co ja...(tylko w innych słowach)

Mhm... a nie pomyślałeś że mogłem po prostu dłużej formułować tego posta? Poza tym nie wspomniałeś o htmlspecialchars której działanie czyni również jego funkcja.

[Fifi209]

Mhm... a nie pomyślałeś że mogłem po prostu dłużej formułować tego posta? Poza tym nie wspomniałeś o htmlspecialchars której działanie czyni również jego funkcja.

O 7 minut ?

htmlspecialchars zamienia na encje - jego funkcja tego nie robi... Więc o czym tu mowa.

[l0ud]

Tak, o 7 minut. Co do htmlspecialchars, robiła to poprzednia wersja funkcji, a na nią patrzałem (mój bład). Niemniej w takiej formie jak teraz tym bardziej należałoby o tym wspomnieć, bo wyrzucenie zmiennej bez filtracji to dziura xss.
btw. właśnie wywołałeś bezsensowną dyskusję na 5 postów Dużo lepiej by było, jakbyś w takich sprawach po prostu napisał na PW.

[Fifi209]

Dyskusja jest też na temat.

Napisałeś, przy wyrzucaniu z bazy używać htmlspecialchars tylko powiedz mi jaki ma sens ciągłe obciążanie parsera jak można to zrobić raz przy zapisie do bazy?

[l0ud]

Daje możliwość bezproblemowej edycji i wyświetlenia danych w wersji niezmodyfikowanej - łatwo chociażby wygenerować listę użytkowników do pliku .txt. Poza tym takie zachowanie to przecież standard

[klocu]

A tak właściwie to czy jest sens bombardować bazę tymi wszystkimi zapytaniami?
Nie lepiej byłoby na początku wyciągać usera po loginie i potem sprawdzać jego parametry zgodnie z Twoimi założeniami?
Bo tak możesz maksymalnie zmieścić się w: 1 SELECT + 1 UPDATE (przy błędnym zalogowaniu) [dobrze myślę?]

Jednym słowem wartoby trochę zoptymalizować skrypt. Ale droga jest chyba dobra, tak sądzę.

[phpion]

Nie lepiej byłoby na początku wyciągać usera po loginie i potem sprawdzać jego parametry zgodnie z Twoimi założeniami?

Słuszna uwaga!

Deaktywacja konta po 3 błędnych próbach logowania

Bardzo niebezpieczna opcja... wystarczy jeden złośliwy użytkownik aby poblokować Ci innych użytkowników.

[sebekzosw]

Demo (Login: demo , Hasło: demo) - sprawdźcie bezpieczeństwo

a oto kod:

[PHP] pobierz, plaintext 
<?php
	if(isset($_POST["logowanie"]) && isset($_POST["login"])) {
		$logon_failure = "";
		if(empty($_POST["login"]) OR $_POST["login"] == "Login") { $logon_failure .= "Wpisz login<br />"; }
		if(empty($_POST["password"]) OR $_POST["password"] == "Hasło") { $logon_failure .= "Wpisz hasło<br />"; }
        $login = mysql_real_escape_string($_POST["login"]);
        $password = sha1($_POST["password"]);
 
        $users = mysql_fetch_assoc(mysql_query("SELECT * FROM users WHERE login='".$login."'"));
 
        if(empty($logon_failure)) {
        	if($users["pass"] != $password) {
        		if($ustawienia["off_account"] == 1) {
	        		if($users["login_failed"] >= 3) {
	        			mysql_query("UPDATE users SET activity='0' WHERE login='".$login."' LIMIT 1"); //Deaktywacja konta po 3 błędnych próbach logowania
	        			$logon_failure = "Konto zostało zdeaktywowane, ponieważ liczba prób logowania była większa niż 3. Aby aktywować konto, kliknij <a href=\"\">tu</a>";
	        		} else {
	        			mysql_query("UPDATE users SET login_failed=login_failed+1, date_last_failed_login='".time()."' WHERE login='".$login."' LIMIT 1"); //Dodawanie +1 do obecnej liczby błędnych logowań
	        			$logon_failure .= "Nieprawidłowy login lub hasło";
	        		}
	       		} else {
	       			$logon_failure .= "Nieprawidłowy login lub hasło";
	       		}
        	} elseif($users["activity"] == 0) {
        		$logon_failure .= "Konto jest nieaktywne";
        	}
 
        	if(empty($logon_failure)) {
            	mysql_query("UPDATE users SET last_login='".$users["penultimate_login"]."', penultimate_login='".time()."', login_failed='0' WHERE id='".$users["id"]."' LIMIT 1"); //Aktualizacja danych o ostatnim logowaniu
				$_SESSION["login"] = $login;
	 			$_SESSION["ip"] = $_SERVER["REMOTE_ADDR"];
 				$_SESSION["browser"] = $_SERVER["HTTP_USER_AGENT"];
 			} else {
        		echo $logon_failure;
        	}
        } else {
			echo $logon_failure;
		}
	}
    echo "<div id=\"menu\">";
	if(UZYTKOWNIK) {
		echo "Zalogowany jako ".$_SESSION["login"];
	} else {
 
 
?>
	<form action="" method="post" id="logowanie">
		<fieldset>
	    	<dl>
	    		<dt>Login</dt>
	     		<dd><input type="text" name="login" id="login" value="Login" onblur="if(this.value=='') this.value='Login';" onfocus="if(this.value=='Login') this.value='';" /></dd>
 
	    		<dt>Hasło:</dt>
	     		<dd><input type="password" name="password" id="password" value="Hasło" onblur="if(this.value=='') this.value='Hasło';" onfocus="if(this.value=='Hasło') this.value='';" /></dd>
	    	</dl>
		</fieldset>
		<div style="float:right;"><input type="submit" name="logowanie" value="Zaloguj się" onclick="sWyslijFormularz('logowanie'); return false;" /></div>
	</form>
	<img src="http://test.php.pl/www/gfx/login_loading.gif" id="ladowanie" alt="" style="display:none;float:left;" />
	<div id="wynik"></div>
 
	<p><a href="/rejestracja/">Rejestracja</a>
		<br />
        <a href="/przypomnij/">Przypomnij hasło</a>
 
 
 <?
 }
 ?>
 </div>
[PHP] pobierz, plaintext 

Ograniczyłem się do 1 SELECT, ale nie mam pomysłu na 1 UPDATE. Jakieś propozycje polepszenia?