PHP Sicheres Login-Skript

[MCStreetguy]

Hallo liebe Community

Ich bastel im Moment an einer Internetseite
Grade versuche ich den Login auf 5 falsche Eingaben zu begrenzen und danach der IP 15 Minuten lang den Login zu verbieten
Mein bisheriger Ansatz sieht so aus:

    if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
		$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
	} else {
		$ip = $_SERVER['REMOTE_ADDR'];
	}
	$searchforfailure = "SELECT * FROM `failed_logins` WHERE `ip`='".$ip."'";
	$res = mysql_query($searchforfailure, $dblink);
	if(mysql_num_rows($res)>= 5) {
		while($fail = mysql_fetch_object($res)) { $timestamp = $fail->ts_int; }
		
		if($timestamp+900 > time()) {
			header('Location: index.php?login=2');
		} else {
			$res = mysql_query($searchforfailure, $dblink);
			while($fail = mysql_fetch_object($res)) {
				$delete = "DELETE FROM `failed_logins` WHERE `id`='".$fail->id."'";
				mysql_query($delete, $dblink);
			}
		}
	}

Also ich trage jeden fehlgeschlagenen Login in der Datenbank ein (siehe Spoiler) und überprüfe dann hiermit ob dort 5 oder mehr Einträge für diese IP vorliegen.

Spoiler: Spoiler

.....  } else {
			$id = time();
			if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
				$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
			} else {
				$ip = $_SERVER['REMOTE_ADDR'];
			}
			$account = $username;
			
			$insertfailure = "INSERT INTO `failed_logins` (`id`, `ip`, `ts_int`, `timestamp`, `affected_account`) VALUES ('".$id."','".$ip."','".time()."',NOW(),'".$account."')";
			mysql_query($insertfailure, $dblink);
			
			header('Location: index.php?login=1');
		}

Könnt ihr mir eventuell weiterhelfen? Ich find leider den Fehler nicht :/
Immer wenn ich ganz oft mich absichtlich falsch anmelde um das auszuprobieren, lande ich immer nur bei dem Hinweis das der Login fehlgeschlagen ist aber nie bei der Sperrung :/

Danke schonmal im Vorraus, MCStreetguy

 

[SchwarzeBeere]

1. https://www.chriswiegman.com/2014/05/getting-correct-ip-address-php/ - HTTP_X_FORWARDED_FOR ist ein simples HTTP-Headerfeld, das leicht zu manipulieren ist und damit als Angriffspunkt für ne SQL Injection dienen könnte.
2. Ingesamt hast du recht viele gleichbedeutende Felder. Du könntest dich problemlos auf drei Felder beschränken: IP, Anzahl von Logins, Letzter fehlgeschlagener Login. IP ist hierbei das primary-Field in der DB, über das der Eintrag identifiziert wird. Bei jedem fehlgeschlagenen Login setzt du den Counter hoch und aktualisierst den Timestamp. Bei einem erfolgreichen Login löscht du den Eintrag wieder. Wenn die Anzahl an Logins > 4 und der letzte timestamp < time()+900, dann ist kein Login möglich. Darüber hinaus brauchst du eine Aufräumroutine, die alte Einträge mind. alle 15min löscht - denn was passiert, wenn die IP sich danach nicht mehr versucht anzumelden?
3. Da du hier nur Bruchstücke aus dem Code gepostet hast: Werden diese Codestellen überhaupt durchlaufen? Hast du geprüft, ob die Werte in der DB korrekt sind?

 

[Fluffy]

Hallo Mc,

Es ist sehr schwierig deinen Code einzusehen da er zerissen ist und nicht als Einheit vorliegt.

Der Grund wiso du nicht zu der Sperre kommst ist der das der letzte Header den 1. Überschreibt.
Dein Code sollte einen einzigen Exit-Punkt haben und nicht verschiedene Zustände unnötig annehmen.
Darüber hinaus ist der Code nicht sicher.
Er ist anfällig für SQLi-Angriffe und du bist immer noch anfällig für Brute-Force Angriffe.
Ich muss nur den HTTP_X_FORWARDED_FOR- Parameter anders setzten und schon bin ich wieder im Rennen.
Die While-schleife ist überflüssig, man kann dies mit einer COUNT-Query lösen.
Die Datenhaltung ist zu überdenken, da man hier einfach einen Zähler hochzählen kann.
mysql_* ist deprecated, benutzte lieber mysqli.
Benutzte Prepared-Statements.
Du greifst unnötiger weise auf die IP-Adresse zu.
Mache keine Zuweisungen in den Konditionalen einer Kontrollstruktur.
Hoffe ich konnte helfen.
Gruß

Fluffy

P.s.:
Es ist schwer solche Sachen sicher zu schreiben, ich an deiner Stelle würde auf Frameworks zugreiffen, welche das machen.
Hier seien Yii, Symfony, Zend oder Laravel genannt.

//edit:
Verdammt 8 min zu langsam, verdammten Diskussionsruden beim Wein.