dedlfix: Frage PHP-Bedingungen

Beitrag lesen

Hi!

nachdem die bedingungen nun festgelegt sind, vielleicht wärd ihr so freundlich und könntet mir bei dem code helfen?

Helfen ja, aber üblicherweise geht es hier um das Helfen beim Verstehen und nicht um das Präsentieren von fertigem Code. Also frag bitte nach, wo genau du ein Verständnisproblem hast. Dazu solltest du soweit Debugging treiben können, um die fehlerhafte Stelle finden zu können. Prüfe also mit Kontrollausgaben (mit var_dump()) ob die Variableninhalte und Ausdrucksergebnisse deinen Vorstellungen entsprechen.

if ($_POST['email'] != "") {

Prüfe lieber mit empty($_POST['email']), denn wenn $_POST['email'] nicht vorhanden ist, ist das ein eigentlich nicht zulässiger Lesezugriff. empty() und isset() berücksichtigen das Nichtvorhandensein von Array-Elementen und einfachen Variablen. Alle anderen Lesezugriffe erzeugen zumindest eine Notice-Meldung, die man sich beim Programmieren immer anzeigen lassen solle. Sie zeigt nämlich auch auf, wenn beispielsweise durch Tippfehler ein falscher Zugriff erfolgt. error_reporting(E_ALL) am Scriptanfang und nur während der Entwicklung oder eine globale Konfiguration auf der Testmaschine kannst du dafür verwenden.

$email = $_POST['email'];
$pass = $_POST['pass'];
$remember = $_POST['remember'];

Das Umkopieren in "einfache" Variablen bringt keine Punkte. Es verschleiert nur die Herkunft von Variablen und ist wegen einer Altlast aus früheren PHP-Tagen leider viel zu oft in Scripten zu sehen.

$email = strip_tags($email);
$pass = strip_tags($pass);

strip_tags() ist in der Regel nicht notwendig. Besonders auf Email-Adressen und Passwörter ist es kontraproduktiv, weil es da völlig legitim ist die <>-Zeichen in einer Form zu verwenden, die wie ein HTML-Tag aussehen.

$email = mysql_real_escape_string($email);
$pass = mysql_real_escape_string($pass);

Das ist schonmal nicht schlecht, aber es steht so separat von der Query. Beim Kopieren der Query, um sie an anderer Stelle zu verwenden, darf man den Teil nicht vergessen.

$sql = sprintf("SELECT * FROM xyz WHERE email='%s' AND Passwort='%s' AND email_activated='1'",
  mysql_real_escape_string($email),
  mysql_real_escape_string($pass));

So hast du alles in einer Anweisung. Der Vorteil von sprintf() ist, dass die Query nicht aufgesplittet werden muss, um die Funktionsergebnisse der mysql_real_escape_string()-Aufrufe einzufügen.

$email = eregi_replace("", "", $email); $pass = eregi\_replace("", "", $pass);

Zum einen sind die ereg-Funktionen veraltet und missbilligt (deprecated). Zum anderen brauchst du bei keine Unterscheidung nach Groß- und Kleinschreibung, weswegen die Variante ohne i ausreichend wäre. Aber, und zum dritten, brauchst du bei solch einer einfachen Stringersetzung keine Funktionen für Reguläre Ausdrücke, da reicht str\_replace(). Zum vierten ist es in deinem Fall nicht notwendig, die-Zeichen zu entfernen weil sie in dem Kontext, in dem du sie einsetzen willst keinen Schaden anrichten. Im Gegenteil, durch das Entfernen von ` aus dem Passwort (und auch schon mit dem strip_tags()) verfälschst du dieses und vereinfachst damit unter Umständen einen Brute-Force-Angriff.

$pass = md5($pass);

Das Passwort nicht im Klartext zu speichern ist eine gute Sache. Aber beachte die Verarbeitungsreihenfolge. Du entfernst zuerst aus dem Passwort Zeichenfolgen, die wie HTML-Tags aussehen, behandelst es dann für den SQL-Kontext (mysql_real_escape_string()), entfernst daraus einfach die `-Zeichen und anschließend erst ermittelst du den MD5-Wert. Du bildest den MD5-Wert also nicht aus der Benutzereingabe sondern aus einer veränderten Form. Wenn du nun das Passwort an anderer Stelle vergleichen wolltest, musst du es auch wieder in dieser Form verändern, damit es den gleichen MD5-Wert ergibt. Das könnte aber sinnlos aussehen, wenn du gar keinen SQL-Kontext hast und trotzdem mysql_real_escape_string() darauf anwendest. Wenn du diese Prozedur vergisst, hast du in schwer zu findenden Fällen, dass der Benutzer mit seinem Passwort nicht weiterkommt. Deswegen lass den ganzen Quatsch und bilde den MD5-Wert aus den Rohdaten. Und behandle Werte erst dann für einen bestimmten Kontext, wenn sie in diesen eingefügt werden sollen und nicht irgendwann vorher.

$sql = mysql_query("SELECT * FROM xyz WHERE email='$email' AND Passwort='$pass' AND email_activated='1'");

Zudem ist es ungünstig, das Ergebnis von mysql_query() in einer Variablen namens $sql abzulegen. Denn SQL ist nur das SQL-Statment. Das Ergebnis ist es nicht mehr. $result wäre schon ein besserer Name, obwohl er immer noch sehr allgemein ist und eventuell in Konflikt mit anderen Results kommen kann.

Eine Fehlerbehandlung fehlt ebefalls. Wenn einer auftritt, den du nicht abfragst, ist es sinnlos einfach weiterzuarbeiten und gibt Folgefehler bei den nachfolgenden mysql_*-Funktionen.

$id = $row["id"];
        session_register('id');
        $_SESSION['id'] = $id;

session_register() - wurde dir ja schon gesagt - ist nicht mehr notwendig. Lass diese Zeile einfach weg. Du brauchst heutzutage nur das session_start() am Anfang und greifst dann nur noch auf das $_SESSION-Array lesend und schreibend zu. Die anderen session_*-Funktionen brauchst du nur in Spezialfällen, und die (un)register-Funktionen gar nicht mehr.

mysql_query("UPDATE xyz SET log_date1=now(), log_date2=log_date1, log_date3=log_date2  log_date4=log_date3 WHERE user='$user'");

Hier fehlt die kontextgerechte Behandlung von $user. Und das Ergebnis von mysql_query solltest du nicht einfach unter den Tisch fallen lassen, denn es gibt dir Auskunft über das Gelingen der Query.

Das war schon mal eine ganze Menge allgemeiner Kram, der falsch oder bedenklich ist ...

Lo!