nicht angemeldet
Moin!
hier iss mal der Code fuer Studierwillige:
Fein, dann gibts sogar noch etwas Codekritik.
Zuallererst: Dir ist schon bekannt, daß man Programmzeilen in Schleifen, IF-ELSE-Konstrukten etc. einrückt, ja? Stört nämlich die Lesbarkeit erheblich, wenn das nicht der Fall ist. :) Ich habe das mal für dich übernommen, sonst sieht man ja garnichts mehr.
Dann (auch noch beim ersten durchsehen aufgefallen: Du hast eine Datenbankanbindung. Die wird im Verlauf des Scriptes mehrfach geöffnet und wieder geschlossen. Warum das denn? Einmal zu Beginn aufmachen, und am Ende nur dann schließen, wenn's wirklich notwendig ist - das ist eigentlich nur bei persistenten Datenbankverbindungen der Fall.
Wenn noch ein paar Spezialisten in die Bresche springen und ganz krude Sicherheitsprobleme hervorholen, werde ich natürlich sofort behaupten, daß man die DB-Verbindung nicht-persistenter Art nach der letzten Benutzung im Script schließen sollte - ansonsten übernimmt das aber nach Ende des Scripts PHP für einen.
Das mehrfache Öffnen/Schließen kostet nämlich Zeit. Vielleicht nicht viel, wenn die Datenbank auf der lokalen Maschine untergebracht ist, und doch signifikant viel Zeit, wenn sie auf einer anderen Maschine untergebracht ist. Verbindung einmal aufmachen, diverse SQL-Befehle übermitteln, beliebig häufig das Abfrageergebnis verwenden, Verbindung durch PHP schließen lassen - besser ist das.
Der Loginteil, der auf das Loginformular folgt (im Loginformular werden alle veralteten Eintraege geloescht):
<?
session_start();
session_register('lang');
$db = mysql_connect('localhost', 'dbname', 'passwort');
/* Die Login-Werte legt man am günstigsten nicht in festen Strings ab, sondern man erstellt eine zentrale Konfigurationsdatei, in der Konstanten definiert werden. Was wäre denn sonst, wenn sich das Paßwort einmal ändert, oder die Position der Datenbank? */
if($db) {
$res = mysql_db_query("dbname","select * from login");
$num =mysql_num_rows($res);
$richtig = 0;
for($i=0;$i<$num;$i++) {
$aname[$i] = mysql_result($res, $i, "name");
$apasswort[$i] = mysql_result($res, $i, "passwort");
if($name == $aname[$i] && $passwort == $apasswort[$i]) {
$richtig = 1;
}
}
mysql_close($db);
}
/* Dieser Block zur Abfrage von Username und Paßwort in der Datenbank scheint mir ziemlich aufwendig und performancefressend. Erstens: Warum läßt du die Datenbank nicht für dich arbeiten?
SELECT * FROM login WHERE username = $name
Und schon mußt du nicht mehr alle DB-Einträge abarbeiten, sondern erhälst nur noch einen einzigen Eintrag (hoffentlich - wenn nicht, ist ein User doppelt eingetragen, das darf nicht sein. Du solltest also prüfen, ob mysql_num_rows() immer 1 ergibt. Wenn nein, ist irgendwas faul.
Weiterhin arbeitest du mit mysql_result(). Ganz böse, weil ultraaufwendig - jedes Feld muß einzeln abgefragt werden. Nimm mysql_fetch_array(). Da kriegst du mit einem Befehl gleich eine ganze Tabellenzeile erfaßt, hast außerdem Zugriff über einen Hash (du gibst nur die Spaltennamen an, nicht mehr deren Position, die sich ja bei Änderungen in der DB (neue Spalte hinzu, alte Spalte weg, Namensänderung) durchaus ändern können), und es ist auch noch schneller - es wird empfohlen, nur noch diese Zugriffsweise zu benutzen.
Deine Abfrage vermindert sich dann zu solch einem Code:
if ($db) {
$res = mysql_query("SELECT * FROM login WHERE username = '$name'");
if ($dbresult = mysql_fetch_array($res) {
// Nur die erste und einzige Zeile einlesen - wenn keine
// Zeile gefunden wurde, gibt die Funktion false zurück.
if ($dbresult['password'] == $password { // Paßwort prüfen
$richtig = 1; // Stimmt? Dann Flag setzen.
}
}
}
Ein mögliches Löchlein ergibt sich aus der Tatsache, daß die doch elementar wichtige Variable nur innerhalb der IF-Abfrage initialisiert wird. Ich könnte also Glück haben und eventuell durch URL-Parameter $richtig=1 einschmuggeln, wenn die Datenbank nicht erreicht wird.
$richtig=0; muß vor der IF-Abfrage gesetzt werden, um sicherzugehen, daß das nicht stattfinden kann!
*/
else {
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
/* An dieser Stelle mußt du unbedingt dafür sorgen, daß das Script auch endet, ansonsten werden alle folgenden Befehle weiter ausgeführt! Wäre die Datenbank nicht erreichbar und wäre $richtig=1 eingeschmuggelt worden, würde es hier nämlich weitergehen - die Header-Funktion setzt nur den HTTP-Header. exit() oder die() wären hier angebracht. */
}
if($richtig == 1) {
$db = mysql_connect('localhost', 'dbname', 'passwort');
// Das mehrfache Öffnen hatten wir ja schon...
if($db) {
$date = time();
$sesid = session_id();
$res = mysql_db_query("dbname","insert sessions (id, datum) values ('$sesid','$date')");
/* Das überflüssige Funktionswerte-erst-Variablen-Zuweisen-dann-Variablen-unverändert-in-String-packen sollte eigentlich auch abgeschafft werden. */
$res = mysql_query("INSERT INTO sessions (id, datum) VALUES ('".session_id()."', '".time()."');";
/* Außerdem prüfst du garnicht, ob dieses Statement auch fehlerfrei ausgeführt werden konnte von der Datenbank. :) */
mysql_close($db);
}
else {
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
//Hier fehlt wieder der Abbruch des Skripts.
}
header ('location:'.$lang.'/index.php?site=intern/richtigeseite');
/* Ich frage mich gleich nochmal, unter welchen Umständen dieses Statement ausgeführt werden kann. :) */
}
if($richtig == 0) {
header ('location:'.$lang.'/index.php?site=sites/meldungen&err=14');
}
// Hier wäre ein Abbruch dann tatsächlich entbehrlich. ;)
?>
So, deine diversen Bedingungen sind etwas konfus verstreut und versperren so den Blick auf das wesentliche: Welcher Header wird unter welchen Bedingungen wann gesendet. Und was passiert mit mehrfachen Headerangaben überhaupt? Werden die alle gesendet, oder überschreiben die sich gegenseitig? Ich weiß es nicht, aber dem kann man, wie erwähnt, durch Scriptabbruch abhelfen.
Zurück zu den Bedingungen bei deinem jetzigen Script. Wie ich erwähnte, könnte man es schaffen, daß sowohl die Datenbank down ist, also auch vorab $richtig=0 zu setzen. Das bedeutet: $db ist immer falsch, $richtig==0 ist falsch, $richtig==1 ist wahr.
Ausgeführt würde dann folgender Code:
if($db) { -> falsch
}
else {
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
}
»»
if($richtig == 1) { -> wahr
if($db) { -> falsch
}
else {
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
}
header ('location:'.$lang.'/index.php?site=intern/richtigeseite');
}
»»
if($richtig == 0) { -> falsch
}
Zusammengefaßt:
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
header('location:'.$lang.'/index.php?site=sites/meldungen&err=15');
header ('location:'.$lang.'/index.php?site=intern/richtigeseite');
Böse Falle, die richtige Seite erscheint - und sogar als letztes! Wenn die einzelnen Header sich gegenseitig überschreiben, war's das mit deiner Sicherheit bis hierher. Wenn nicht, kann man trotzdem im HTTP-Header nachgucken, was da alles drinsteht - und was soll der Browser überhaupt machen bei so viel Auswahl? Allerdings wird keine Session in die Datenbank geschrieben - trotzdem würdest du nicht erwartet haben, daß du ein so unsicheres Script geschrieben hast, oder?
Dann das, was in allen internen Seiten steht:
Das schaue ich mir jetzt nicht noch alles an, schließlich gibts im Forum eine maximale Postinggröße von nur 12 KB! :)
- Sven Rautenberg