[PHP] Ist dieses Loginscript gut und sicher?

[Cyperfriend]

Dann möchte ich hiermit den Bereich "Webentwicklung" feierlich eröffnen und direkt meine erste Frage stellen.

Ich baue gerade ein Projekt und bin jetzt dabei das Loginsystem zu entwickeln. Kann man das so stehen lassen oder seht ihr da Verbesserungsbedarf?

<?php session_start();
# Wenn der Login-Button gedrückt wurde die eingegebenen Daten in der Datenbank überprüfen.
if($_POST['login']){
	# Datenbankinformationen
	$db_user = "*****";
	$db_pass = "*****";
	$db_name = "*****";
	$db_connect = mysql_connect("*****", $db_user, $db_pass);
	mysql_select_db($db_name);

	# Nach Benutzernamen und Passwort in der Datenbank suchen
	$db_read = "select benutzername, passwort from db_benutzer 
				where (benutzername='".htmlspecialchars($_POST['benutzername'])."') and (passwort='".md5($_POST['passwort'])."')";
	$db_result = mysql_query($db_read, $db_connect) or die (mysql_error());

	# Wenn eine Übereinstimmung gefunden wurde die SESSION setzen und in den internen Bereich weiterleiten
	if(mysql_num_rows($db_result) > 0){
		$_SESSION['benutzername'] = htmlspecialchars($_POST['benutzername']);
		$_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
		header ("Location: intern/index.php");
	}
	else{
		header ("Location: index.php&error=1");
	}
}
else{
	header ("Location: index.php");
}
?>

Was mich noch ein bisschen stört ist, dass ich bei einem Fehler den Errorcode über die URL mitgeben muss. Kann man das auch eleganter lösen? Bin für Vorschläge und anregungen offen.

 

[Larius]

Bzgl. dem Errorcode: Du hast ja die Session offen. Das könntest du wunderbar über die Session regeln, sprich du schreibst dort einfach den Fehlercode hin, verweist auf eine error.php site und liest dort einfach das Ganze aus.

 

[epiphora]

htmlspecialchars() ist zur Maskierung von Sonderzeichen im HTML-Kontext erforderlich, um beispielsweise XSS-Lücken zu schließen. In Deinem Query geht es natürlich nicht darum, die Zugangsdaten von HTML zu reinigen, sondern von SQL-Statements.

Insofern würde ich beim Bauen vom Query nicht htmlspecialchars, sondern mysql_real_escape_string verwenden.

Den Benutzernamen würde ich auch nicht in der Session maskieren, sondern bei jeder Ausgabe. Das ist nicht nötig, aber meiner Meinung nach der sauberere Weg. So stellst Du sicher, dass er auch dann noch korrekt gespeichert ist, falls er mal nicht im HTML-Kontext ausgegeben wird.

 

[braegler]

Ich würde an Deiner Stelle dazu übergehen mysqli zu verwenden. Es gibt dafür einige Wrapper, welche die sichere Kommunikation mit dem MySQL Server kinderleicht machen.

Ich persönlich verwende für einige Kleinprojekte den hier:
http://blog.vjeux.com/2009/php/mysqli-wrapper-short-and-secure-queries.html

 

[Kugelfisch]

Ich kann mich epiphoras und braeglers Empfehlungen nur anschliessen. Beachte, dass die Verwendung von htmlspecialchars() in diesem Fall nicht nur wenig sinnvoll ist, sondern (zumindest ohne ENT_QUOTES-Option) auch eine SQL-Injection-Schwachstelle bietet, da die '-Zeichen nicht maskiert werden.

Ausserdem schreibt HTTP 1.1 vor, dass ein Location-Header immer einen vollständigen, absoluten URI enthalten muss, siehe http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.30. Auch wenn moderne Browser auch relative URIs korrekt interpretieren, ist dieses Verhalten nicht definiert - du solltest daher einen absoluten URI für die Weiterleitung nutzen (ggf. dynamisch aus den Informationen von $_SERVER erzeugt).

 

[aNtiCHrist]

Ein Beispiel dazu gibt es übrigens unter http://www.php.net/manual/en/function.header.php in der sechsten Anmerkung. Sofern der Code auf Sites zum Einsatz kommen soll, die sowohl per HTTP als auch per HTTPS zu erreichen sind, müsste der Code natürlich noch erweitert werden, etwa mit

if ($_SERVER['HTTPS'] == 'on'){
  $scheme = 'https';
} else {
  $scheme = 'http';
}
header('Location: '.$scheme.'//$host$uri/$extra');

 

[Cyperfriend]

Danke euch für die Antworten.
htmlspecialchars und mysql_real_escape_string hatte ich natürlich verwechselt. Die Idee Fehler, bzw. Meldungen mittels Session zu übergeben gefällt mir und habe ich auch direkt versucht umzusetzen (dazu gleich mehr). Ich würde die Sache gerne mit reinem SQL machen, ohne mysqli. Ich habe so schon immer mit Programmierung und Scripting zu kämpfen und bin froh, dass ich da jetzt durchsteige und schonmal verstehe was ich mache.
Mein Script sieht jetzt so aus.
login.php

<?php session_start();

include("codes.php");

# Wenn der Login-Button gedrückt wurde die eingegebenen Daten in der Datenbank überprüfen.
if($_POST['login']){
	# Datenbankinformationen
	$db_user = "*****";
	$db_pass = "*****";
	$db_name = "*****";
	$db_connect = mysql_connect("*****", $db_user, $db_pass);
	mysql_select_db($db_name);

	# Nach Benutzernamen uhnd Passwort in der Datenbank suchen
	$db_read = "select benutzername, passwort from db_benutzer 
				where (benutzername='".mysql_real_escape_string($_POST['benutzername'])."') and (passwort='".md5($_POST['passwort'])."')";
	$db_result = mysql_query($db_read, $db_connect) or die (mysql_error());

	# Wenn eine Übereinstimmung gefunden wurde die SESSION setzen und in den internen Bereich weiterleiten
	if(mysql_num_rows($db_result) > 0){
		$_SESSION['benutzername'] = mysql_real_escape_string($_POST['benutzername']);
		$_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
		$_SESSION['code'] == 0;
		header ("Location:  http://".$_SERVER['HTTP_HOST']."/neu/intern/index.php");
	}
	else{
		$_SESSION['code'] == 1;
		header ("Location: http://".$_SERVER['HTTP_HOST']."/neu/index.php");
	}
}
else{
	$_SESSION['code'] == 0;
	header ("Location: http://".$_SERVER['HTTP_HOST']."/neu/index.php");
}

?>

codes.php

<?php session_start();
switch($_SESSION['code']) {
	case 0:
	$msg = "Keine Message";
	break;
	case 1:
	$msg = "Fehler beim Login.";
	break;
	default:
	$msg = "Sollte niemals vorkommen";
}
?>

Es wird immer nur "Keine Message" ausgegeben und ich gebe definitiv falsche Zugangsdaten ein und habe auch schon die Umleitung umgebaut. Das Scricript login.php führt die richtige header-Zeile aus.

Habt ihr sonnst noch Schwachstellen entdeckt, die ich beheben sollte?

 

[Larius]

        $_SESSION['code'] == 0;

Ist natürlich falsch @ index.php

 

[Cyperfriend]

        $_SESSION['code'] == 0;

Ist natürlich falsch @ index.php

Autsch, aber sowas von. Habs korrigiert.

 

[Kugelfisch]

Was bezweckst du mit der Maskierung in

        $_SESSION['benutzername'] = mysql_real_escape_string($_POST['benutzername']);

Das ist wenig sinnvoll, da du den Benutzernamen nicht in einen MySQL-String-Kontext verwendest - daher wird der Benutzername verfälscht. Oder nutzt du später $_SESSION['benutzername'] unmaskiert in einem MySQL-Query? Dann wäre sinnvoller, direkt dort zu maskieren.

Ausserdem solltest du den aus der Datenbank abgefragten Benutzernamen verwenden oder $_POST['benutzername'] per Typecast explizit in einen String konvertieren, da PHP aus POST-Daten automatisch Arrays erteugen kann (wenn der Name des POST-Feldes auf `[]` endet), was du dann an anderer Stelle möglicherweise nicht erwartest.

 

[Cyperfriend]

Was bezweckst du mit der Maskierung in

        $_SESSION['benutzername'] = mysql_real_escape_string($_POST['benutzername']);

Das ist wenig sinnvoll, da du den Benutzernamen nicht in einen MySQL-String-Kontext verwendest - daher wird der Benutzername verfälscht. Oder nutzt du später $_SESSION['benutzername'] unmaskiert in einem MySQL-Query? Dann wäre sinnvoller, direkt dort zu maskieren.

Ich habe das getreu dem Motto "lieber einmal zu viel, als einmal zu wenig" gemacht. Außerdem bin ich mir nicht sicher, in welchen Situationen genau ich "mysql_real_escape_string" einsetzen sollte.

Ausserdem solltest du den aus der Datenbank abgefragten Benutzernamen verwenden oder $_POST['benutzername'] per Typecast explizit in einen String konvertieren, da PHP aus POST-Daten automatisch Arrays erteugen kann (wenn der Name des POST-Feldes auf `[]` endet), was du dann an anderer Stelle möglicherweise nicht erwartest.

Das verstehe ich nicht. Wie meinst du das, bzw. wie sollte das in der Praxis aussehen?

 

[drfuture]

wie sollte das in der Praxis aussehen?

vor der Verwendung z.B. des Benutzernamens hier
$username = (string)$myVar;

$_SESSION['benutzername'] = (string)$_POST['benutzername'];

und dann mit der Session weiterarbeiten - damit stellst du sicher das jemand einen unerwarteten Fehler erzeugt den du nicht kalkuliert hast ...

 

[Cyperfriend]

und dann mit der Session weiterarbeiten - damit stellst du sicher das jemand einen unerwarteten Fehler erzeugt den du nicht kalkuliert hast ...

Fehlt da was, lese ich da Ironie oder irgendwie verstehe ich es nicht.

Klar, mit deinem Code wandle ich den Benutzernamen in einen String um, aber ist er das nicht sowieso? So wie ich das verstehe ist es weniger clever mit dem Benutzernamen in der Session weiter zu arbeiten?

 

[drfuture]

Nein ironie war da keine - ich schreibe immer Ernst ;D ... nu ja meistens

Nein der Code war auf Kugelfischs Hinweis mit der String-Konvertierung zurückzuführen. "da PHP aus POST-Daten automatisch Arrays erzeugen kann"
wenn man dem login-script in den Postdaten der name eines Feldes auf [] endet wird in php ein Array erstellt. Das heißt wenn der Seitenbesucher die Postdaten zu deinem login-script so manipuliert das er dir benutzername[]=Test, benutzername[]=Test2 sendet - dann kommt in $_POST['benutzername'] in deinem login-script ein Array mit 2 Werten an.. Das ist ja so nicht gewünscht und würde dann evtl. einen Fehler erzeugen.

 

[Cyperfriend]

Richtig, das soll ja verhindert werden. Mache ich das jetzt bereits oder noch nicht? Mir fehlt dafür noch das Verständnis bzw. das Verstehen was im Hintergrund passiert und wie die Daten verarbeitet werden.

Ich habe die Zeile jetzt so abgeändert

$_SESSION['benutzername'] = string($_POST['benutzername']);

Das hier sollte bereits manipulationssicher sein?

$db_read = "select benutzername, passwort from db_benutzer 
				where (benutzername='".mysql_real_escape_string($_POST['benutzername'])."') and (passwort='".md5($_POST['passwort'])."')";

 

[drfuture]

Das hier sollte bereits manipulationssicher sein?

$db_read = "select benutzername, passwort from db_benutzer 
				where (benutzername='".mysql_real_escape_string($_POST['benutzername'])."') and (passwort='".md5($_POST['passwort'])."')";

Du greifst dort "direkt" auf $_POST['benutzername'] zu das heißt dort wäre das gleiche Problem mit dem Array...
deswegen am Anfang des Scripts am besten alle Daten die in das Script fließen auf Post/Get passend filtern und Casten und dann später verwenden, wenn du wie oben angegeben den Benutzernamen in einen String Castest und dann so in die Session-Variable speicherst kannst du im ganzen Script auf die Session-Variable zugreifen ($_Session) ist eh ein Superglobal und ist global zugreifbar - sollte jedoch normal nicht als universelle Variablenabnahme genutzt werden... für den User ist das aber denke ich ein guter Speicherort

 

[Cyperfriend]

Kannst du mir das Script entsprechend anpassen? Vielleicht verstehe ich es, wenn ich es sehe und auch den Unterschied habe. Ich glaube sonnst dokter ich noch ewig an dem Script rum.

 

[drfuture]

Jo morgen im laufe des Tages - falls mir keiner zuvor kommt

 

[Cyperfriend]

Danke. Noch eine Bitte. Orientier dich nach Möglichkeit dabei an meinem Code. Wenn der jetzt total Kopf gestellt wird fehlt mir zum einen der Vergleich und zum anderen würde ich vermutlich wieder nichts verstehen.

... es sei den mein Code ist jetzt schon totaler Mist. Es muss nicht das Non-Plus-Über-Ultra als Perfektion sein. Es soll nur sicher sein.

 

[accC]

db.php

<?php
function dbconnect ()
{
    # Datenbankinformationen
    $db_user = "*****";
    $db_pass = "*****";
    $db_name = "*****";
    $db_connect = @mysql_connect("*****", $db_user, $db_pass);
    @mysql_select_db($db_name);
    return $db_connect;
}
?>

login.php

<?php
if(empty($_SERVER['HTTPS']))
{
    // Verhindere unsichere Verbindung!
    /* URI muss angepasst werden! */
    header('Location: https://www.meineseite.de/login.php');
}

session_start();
include('db.php');

// Logindaten wurden vollständig gesendet
if (!empty($_POST['benutzername']) && !empty($_POST['passwort']))
{
    // Datenbank
    $dbhandle = dbconnect();

    // Benutzername für die Datenbank aufbereiten
    /* An dieser Stelle darf theoretisch "böses Script" im Namen stehen, wir müssen nur sql-injections verhindern
       Man könnte den Namen aber auch "ordentlich" in die Datenbank legen (mysql_real_escape_string(html_special_chars($_POST['benutzername']), $dbhandle) */
    $benutzer = mysql_real_escape_string($_POST['benutzername'], $dbhandle);

    // wir sind paranoid und glauben an böse hash-Algorithmen!
    /* beachte, dass md5 nicht mehr als sicher gilt, beachte auch, dass hier der Einfachheit halber davon ausgegangen wird, sha512 stünde zur Verfügung.
        davon darfst du nicht per se ausgehen und müsstest erst ermitteln, welche guten Algorithmen dir zur Verfügung stehen! */
    $passwort = mysql_real_escape_string(hash('sha512', $_POST['passwort']));

    // Query
    $string = "SELECT 1
                     FROM db_benutzer
                     WHERE `benutzername`='$benutzer' AND
                                  `passwort`='$passwort'";
    $query = @mysql_query($string, $dbhandle);

    // Login war erfolgreich
    if($query && mysql_num_rows($query)==1)
    {

        // Benutzername schön machen:
        /* an dieser Stelle müssen wir spätestens darauf achten, dass kein cross site scripting oder böses html/css/javascript im Namen ist, falls wir das oben schon gemacht haben, ist es hier nicht mehr zwangsweise nötig */
        $_SESSION['benutzername'] = htmlspecialchars($benutzer);
        $_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];

        // Weiter zur Mitgliedsseite
        $_SESSION['ecode'] = 0;
        /* URI muss angepasst werden! */
        header ("Location: https://meineseite.de/intern/index.php?".session_id());
    }
    else
    {
        // Login war fehlerhaft, zurück
        $_SESSION['ecode'] = 1;
        /* URI muss angepasst werden! */
        header ("Location: http://meineseite.de/index.php?".session_id());
    }
}
else
{
    // Der Benutzer hat nicht alle nötigen Daten angegeben, zurück
    $_SESSION['ecode'] = 2;
    /* URI muss angepasst werden! */
    header ("Location: http://meineseite.de/index.php?".session_id());
}
?>

Ich hoffe ich habe jetzt nichts vergessen oder übersehen.

Grob gesagt:
htmlspecialchars verhindert, dass eine Eingabe im Kontext der generierten Seite interpretiert wird.
Beispielsweise, dass <b>ich bin fett</b> als ich bin fett interpretiert wird.

mysql_real_escape_string verhindert, dass eine Eingabe im Sinne von MySQL interpretiert wird.
Zum Beispiel:
$string = '1"; DROP WHERE "1" = "1';
In einer Abfrage:
mysql_query('SELECT * FROM `bla` WHERE `var` = "$string"');
Wird auf Datenbank-Ebene zu:
SELECT * FROM `bla` WHERE `var` = "1"; DROP WHERE "1" = "1"
Wenn du aber mysql_real_escape drauf los lässt passiert Folgendes:
SELECT * FROM `bla` WHERE `var` = "1\"; DROP WHERE \"1\" = \"1"

Bin da aber auch kein Profi..