Login - Brute force protection - PDO

Ja, zolang je eerst maar de databaseverbinding sluit, anders bouw je zelf een DDoS-aanval op "Too many connections".

Het is niet ongebruikelijk om de vertraging steeds langer te maken, bijvoorbeeld met 2 ^ x voor x inlogpogingen:

2 ^ 0 = 1 s
2 ^ 1 = 2 s
2 ^ 2 = 4 s
2 ^ 3 = 8 s
2 ^ 4 = 16 s
...

Verder moet je niet slechts naar losse inlogpogingen kijken, maar naar het grotere geheel. Daarvoor moet je de omvang van de site weten (of meten). Als je normaal gesproken bijvoorbeeld gemiddeld 1 login per 10 minuten hebt, weet je dat er iets loos is wanneer er plotseling 20 mislukte logins in 1 minuut zijn.

Tortuga web op 10/08/2014 08:47:23

Ja, want de $results (met s) is alles wat er uit de database opgehaald wordt, en omdat username uniek is, behoort, indien juist ingegeven, er 1 resultaat teruggegeven te worden.

Zoals ik al eerder zei, ik vind die query om te beginnen vreemd, ik kan niet doorgronden waarom die nuttig is. Verder vind ik het ook vreemd dat je binnen een foreach loop, waarin je over de $results array loopt, een check op het aantal elementen in $results plaatst.
Ik zeg niet dat het allemaal fout is, maar ik begrijp je logica in elk geval niet.

Dit is wat ik nu heb, ik heb de login-fail er volledig uitgehaald. En Erwin, ik ben het met je eens dat er iets niet helemaal logisch is, want omdat het resultaat altijd maar 1 rij op moet leveren, zou er geen foreach-loop in moeten zitten, maar die is er eigenlijk vanwege try and error ingeslopen. Het script werkte niet zonder en wel met, maar ik weet niet waarom.

<?php
	ini_set('display_errors',1); // 1 == on , 0 == off
	error_reporting(E_ALL | E_STRICT);

	session_start();  

	if ($_SERVER['REQUEST_METHOD'] == 'POST') {
		$errors = array();
		if (trim($_POST['username']) == ''	or (!preg_match('~^[0-9xyzXYZ][0-9]{7}[A-Za-z]$~',($_POST['username']))))
			$errors['username'] = 'Rellena número del NIE o NIF';
		if (trim($_POST['password']) == '' or (!preg_match('~^[0-9]{4}$~', $_POST['password'])))
			$errors['password'] = 'Rellena número socio.<br>Un número menos que 1000, empieza con 0 hasta que complete 4 dígitos.';
		if (!empty($_POST['name']))
			$errors['name'] = 'No valido';
		if (count($errors) == 0) {
			//Start procedure
			try {
				//Connect to the dBase
				require_once ('sql_link_sherpa.php');
				//Make the query
				$sql_select = "SELECT id, nombre, pass FROM socios WHERE dni = :dni";
				$userStmt = $db->prepare($sql_select);
				$userStmt->execute(array(
								':dni' => $_POST['username']
							));
				$results = $userStmt->fetchAll();
				$db = NULL;
				foreach ($results as $result) {
					//Check	password and set session values			
					if (count($results) == 1) {
						$hash = $result['pass'];
						if (!password_verify(($_POST['password']), $hash)) {
							sleep(3);
							header ('Refresh: 3; url=login.php');
							echo 'Numero incorrecto';
							exit();
						}
						else {
							$_SESSION['logged_in'] = TRUE;
							$_SESSION['id'] = $result['id'];
							$_SESSION['user'] = $result['nombre'];
							$_SESSION['start'] = time(); // Taking now logged in time.
							// Ending a session in 30 minutes from the starting time.
							$_SESSION['expire'] = $_SESSION['start'] + (30 * 60);
							header("Location: index_socio.php");  
							exit();
						}
					}
					else {
						header ('Refresh: 3; url=login.php');
						echo 'NIF/NIE desconocido';
						exit();
					}	
				}
			}
			catch(PDOException $e) {
				$sMsg = '<p>
						Linenumber: '.$e->getLine().'<br />
						File: '.$e->getFile().'<br />
						Errormessage: '.$e->getMessage().'
						</p>';
				trigger_error($sMsg);
			} 
		}
	}
?>

@Ward, kun je misschien een tip van de sluier oplichten van hoe ik dat in mijn script implementeer?

Als je zelf niet meer weet waarom je sommige regels code in je script hebt zitten dan wordt het tijd om je hele script opnieuw te schrijven. Of dan in elk geval het stuk waarin de regels zitten.
Ook wordt het dan tijd om serieus naar het commentaar in je script te kijken. Het enige wat je nu hebt is commentaar over wat je doet, maar dat is overbodig. Wat het doet is wel te zien, niet waarom je het doet. Commentaar is nodig om juist dat uit te leggen, en nu blijkt waarom. Ga ook niet denken 'dat komt later wel, ik wil het eerst werkend hebben'. Want dan ben je te laat. In de eerste plaats heb je er op dat moment geen zin meer in om het te doen, maar ook heb je al de helft van de momenten waarop je het commentaar juist nodig had al gehad. Dat blijkt nu maar weer.

Dus mijn tip: haal het hele stuk uit je script, ga opnieuw de logica bedenken, schrijf het opnieuw en bouw vanaf nu altijd direct commentaar in. Je hebt nu een duidelijk voorbeeld waarom je dat commentaar nodig hebt, dus elk argument dat je nu bedenkt om dit niet te doen is een slap excuus.

Ik kan natuurlijk iedere keer als iets niet werkt, hier de vraag dumpen, zonder eerst zelf te zoeken, maar dat is mijn stijl niet. Dus zit er nu iets (wellicht) onlogisch in het script, maar dat is er met try and error ingekomen en zou ik graag willen weten hoe dat anders te doen.
Dus: Als ik de foreach-loop niet gebruik, dus zo:

<?php
//Get the results from the database and close the connection
$result = $userStmt->fetchAll();
$db = NULL;
//Check the result from the database
if (count($result) == 1) {
 	$hash = $result['pass'];
	if (!password_verify(($_POST['password']), $hash)) {
		sleep(3);
		header ('Refresh: 3; url=login.php');
		echo 'Numero incorrecto';
		exit();
	}
	else {
        	$_SESSION['logged_in'] = TRUE;
		$_SESSION['id'] = $result['id'];
		$_SESSION['user'] = $result['nombre'];
		$_SESSION['start'] = time(); 
		$_SESSION['expire'] = $_SESSION['start'] + (30 * 60);
		header("Location: index_socio.php");  
		exit();
	}
}
else {
	header ('Refresh: 3; url=login.php');
	echo 'Username desconocido';
	exit();
}
?>

Dan wordt er (bij juiste inloggegevens) het password gecontroleerd en foutgemeld. Terwijl als ik de loop er wel in heb zitten, dan wordt de juiste controle uitgevoerd en foutgemeld als het password fout is en ingelogged als het goed is.
Wat dan niet werkt is de check op de username.

Waarom het nu fout gaat is dat je de array niet goed interpreteert. De array die je terug krijgt uit de fetchAll is een multidimensionale array van records uit de database. Die ziet er zo uit:

Array
(
    [0] => Array
        (
            [id] => ...
            [nombre] => ...
            [pass] => ...
        )
) 

$hash = $result['pass']; moet dus worden $hash = $result[0]['pass'];

Wel jammer dat je niet mijn advies uit de vorige post eerst hebt overgenomen.....

Oké, dus de fout zit in de array in de array. Dat had ik niet begrepen, maar ik zie nu hoe het werkt.
Heb het in mijn script aangepast en nu werkt het wel goed. Dank je wel.
Wat denk je van de, wel heel simpele, toevoeging van de vertraging op een foute login, met de sleep() functie dus?

Ik zou die sleep veel eerder doen. Nu doe je het zo te zien alleen op een specifiek punt. Ik zou het gewoon aan het begin doen, zodat elke login poging wordt vertraagd. 2 of 3 seconden wachten is voor een gewone gebruiker geen probleem.

Kun je uitleggen waarom je dat zou doen, ik zie dat niet helemaal. Als de login juist is hoeft er geen vertraging op te zitten om brute force te voorkomen, want dan ben je al binnen. En omdat dit script alleen een login bevat, is de password-check het punt waar opnieuw geprobeerd kan worden om in te loggen.
Of zie ik iets helemaal over het hoofd?

Ja en nee. Als élke login, ook een geldige, 3 seconde op zich laat wachten, gaat daarvan al op voorhand een signaal uit: bij deze site gaat een brute force-aanval héél lang duren.