Login-Skript - Ich kann den Fehler nicht finden!

[Hannes]

<?php

error_reporting(E_ALL);
ini_set('display_errors', 1);

$db = @new mysqli('***', '***', '***', '***');

readfile('header.html');

$ret = array();
$ret['filename'] = 'login2.html';
$ret['data'] = array();
if ('POST' == $_SERVER['REQUEST_METHOD']) {
    if (!isset($_POST['Username'], $_POST['Password'], $_POST['submit'])) {
        return INVALID_FORM;
    }
    if (('' == $Username = trim($_POST['Username'])) OR
            ('' == $Password = trim($_POST['Password']))) {
        return EMPTY_FORM;
    }
    $stmt->close();

$sql = 'SELECT
                ID
            FROM
                User
            WHERE
                Username = ?';
    $stmt = $db->prepare($sql);
    if (!$stmt) {
        return $db->error;
    }
    $stmt->bind_param('s', $Username);
    if (!$stmt->execute()) {
        return $stmt->error;
    }
    $stmt->bind_result($UserID);
    if (!$stmt->fetch()) {
        return 'Es wurde kein Benutzer mit den angegebenen Namen gefunden.';
    }
    $stmt->close();

$sql = 'SELECT
                Password
            FROM
                User
            WHERE
                ID = ? AND
                Password = ?';
    $stmt = $db->prepare($sql);
    if (!$stmt) {
        return $db->error;
    }
    $Hash = md5(md5($UserID).$Password);
    $stmt->bind_param('is', $UserID, $Hash);
    if (!$stmt->execute()) {
        return $stmt->error;
    }
    $stmt->bind_result($Hash);
    if (!$stmt->fetch()) {
        return 'Das eingegebene Password ist ungültig.';
    }
    $stmt->close();

$_COOKIE['UserID'] = $UserID;
    $_COOKIE['Password'] = $Hash;
    return showInfo('Sie sind nun eingeloggt.');

}
return $ret;

readfile('footer.html');
?>

Das soll ein Login-Skript sein. Ganz einfach - Ich will das erstmal nnur hinbekommen, dass ich mich irgendwie einloggen kann. Aber bei diesem Skript zeigt er mir einfach kein Formular an. Was mache ich falsch? Die Formular-Datei, die ich oben einfüge, stimmt hundertprozentig. Kann mir jemand sagen, was ich an dem Skript ändern muss, damit ich das Formular sehe, in das ich meine Logindaten eingebe?

Gruß,
Hannes

[suit]

Kann mir jemand sagen, was ich an dem Skript ändern muss, damit ich das Formular sehe, in das ich meine Logindaten eingebe?

ich sehe kein forumlar

ggf wäre es nicht verkehrt, jenenigen zu fragen, der das ding verbrochen hat

oder du beschäftigst dich mit prepared statements bzw stored procedures

alternativ kannst du das ding auch wegschmeissen und neu schreiben, damit du auch verstehst, was du da tust

[Hannes]

Das Formular wollte ich nicht in das Forum stellen. Deshalb sage ich ja, daran liegt es nicht. Das habe ich extra geschrieben. Genau wie den footer und den header. Der footer und der header werden auch aufgerufen aber nciht das Formular. Ich bin auch der Meinung, dass ich verstehe was ich da mache. Ich bin das Skript auch bestimmt 10mal durchgegangen und hab den Fehler nicht gefunden.
Am besten ich lösche die Datei und fange nochmal von vorne an. Ich dachte ich kann die Arbeit umgehen. Aber so kann ich nur hoffen, den Fehler nicht ein zweites Mal zu machen.

[Steel]

Moin!

Dein Formular wird per POST aufgerufen?

[Andreas]

ggf wäre es nicht verkehrt, jenenigen zu fragen, der das ding verbrochen hat

Hi,

ist es richtig das ich hier rauslese, dass dieses Script deiner Meinung nach nichts taugt?

Wenn es so ist, würde mich mal interessieren was an diesem Script schlecht ist. Das soll kein Vorwurf oder dergleichen sein sondern mir beim Verständnis/Lernen helfen, denn ich finde das Script so wie es ist, OK.
Soll heißen ich hätte es genauso gemacht.

Deshalb würde ich gerne wissen, wenn es schlecht ist, was schlecht ist.

Gruß
Andreas

[suit]

Deshalb würde ich gerne wissen, wenn es schlecht ist, was schlecht ist.

ich hab nicht gesagt, dass es schlecht ist - es ist sogar recht gut, wenn man sich sonst so ansieht, was hier herumfleucht - aber es gibt immer dinge die imho zu bemängeln sind:

return $db->error; ist keine fehlerbehandlung, ein normaler mensch sieht eine fehlermeldung die meistens das layout schrottet, das hilft ihm nicht weiter - hier gehört eine ordentliche geschichte hin, die dem normalen menschen erklärt, dass es grade nicht geht und dass bereits ein admin informiert ist

return 'Das eingegebene Password ist ungültig.';
das würd ich so auch nicht machen, früher oder später will man seine seite übersetzen, texte ändern oder sonstwas - strings oder einzelne wörter die änderbar sind oder übersetzt werden können, sollte man immer zentral auslagern

weiters ist die trennung der benutzername- und password-prüfung einerseits ressourcenlastiger (2 abfragen statt einer) und andererseits sicherheitskritisch, da man problemlos erst den benutzernamen bruteforcen kann (sofern dieser nicht bekannt ist und das ist bei vielen systemen der fall anzeigename != loginname, da es zum password zusätzlich noch einen zweiten string gibt, der geheimgehalten werden muss)

bruteforce-angriffe macht das wie erwähnt schon um einiges schwieriger

session mit der user id und dem hash des passwords zu setzen ist auch extrem schlau - zwar kann man damit noch nicht die daten rekonstruieren, aber man kann sich zumindest einloggen, wenn man die möglichkeit hat die cookie-informationen eines anderen auszuspähen da scheinbar nirgens hinterlegt wird, wie lange das login tatsächlich gültig ist

zudem ist mit bekanntwerden des algorithmus auch die sicherheit des hashes nicht mehr sichergestellt - die user id, und somit die ersten zeichen des klartext, sind bekannt - somit ist ein rainbow-angriff noch viel einfacher durchzuführen, da bei einem treffer sofort überprüft werden kann, ob es sich um das klartext-passwort handelt (zwar ist das extrem rechenintensiv, aber ein weiterer potentieller angriffspunkt)

ich würde ein INSERT ON DUPLICATE KEY UPDATE verwenden um einen eintrag in einer session-tabelle zu erzeugen, wenn benutzername und kennwort beim login-vorgang übereinstimmen mit einem beliebigen hash (zeit * zufallzahl * schuhgröße von kelly bundy) welcher dann als einzige information in einem cookie liegt - wenn die abfrage fehlschlägt, ist benutzername oder kennwort (oder beides) falsch, wenn die abfrage erfolgreich ist ist der benutzer eingeloggt - es fehlt dann lediglich eine weitere abfrage die die session-tabelle wieder bereinigt

alternativ lässt sich statt dem beliebigen hash natürlich auch die session-information von php dazu verwenden (das ist sogar schlauer), man speichert dann lediglich die session-id in der datenbank bzw in die session-tabelle

für verbesserungsvorschläge an meinen vorschlägen bin ich offen, ich lerne auch gerne dazu ;)

[dedlfix]

echo $begrüßung;

return $db->error; ist keine fehlerbehandlung, ein normaler mensch sieht eine fehlermeldung die meistens das layout schrottet, das hilft ihm nicht weiter - hier gehört eine ordentliche geschichte hin, die dem normalen menschen erklärt, dass es grade nicht geht und dass bereits ein admin informiert ist

Ein return ist kein echo. Es ist nicht bekannt, was mit dem Rückgabeergebnis passiert. Die Annahme, dass der Anwender das angezeigt bekommt, kann aufgrund der Zeile so nicht getroffen werden.

echo "$verabschiedung $name";

[suit]

Ein return ist kein echo. Es ist nicht bekannt, was mit dem Rückgabeergebnis passiert. Die Annahme, dass der Anwender das angezeigt bekommt, kann aufgrund der Zeile so nicht getroffen werden.

da hast du schon recht, meine vermutung begründet sich auf der annahme, dass jeder andere return in diesem script aber ebenfalls ähnliches tut

return 'Das eingegebene Password ist ungültig.';

es wäre schon arg verwunderlich, wenn am anderen ende geprüft wird, ob der rückgabewert "Das eingegebene Password ist ungültig." oder eben die benutzername geschichte ist und dann eben ein echo ausführt ;)

entweder man gibt "fehlernummern" zurück die dann wirklich wo anders ausgewertet werden oder man gibt fehlermeldungen zurück - aber beides mischen ist imho absurd

[Steel]

Hi!

entweder man gibt "fehlernummern" zurück die dann wirklich wo anders ausgewertet werden oder man gibt fehlermeldungen zurück - aber beides mischen ist imho absurd

Stimmt nur teilweise. Wenn meine Rueckgabewerte kein stichwortartigen Fehlermeldungen waeren (z.B. username invalid), koennte ausser mir hier niemand den Code warten. Das ist eigentlich so schon zuviel verlangt. Ich muesste in der aufrufenden Funktion und in der ausgebenden Funktion jeweils eine komplette Tabelle mit Fehlernummern haben, um hoffen zu duerfen, dass jemand den Code ohne mich anpassen kann. Da ist mir ein kurzes 'invalid password' doch lieber.

Aber im Grunde geb ich Dir recht.

[suit]

Da ist mir ein kurzes 'invalid password' doch lieber.

darum hab ich auch "fehlernummern" geschrieben - hätte besser noch fehler"nummern" schreiben sollen ;)

ich bevorzuge auch ein "success" "failed_cookie" oder sonstiges ;)

[Stefanie]

Hallo,

Ich muesste in der aufrufenden Funktion und in der ausgebenden Funktion jeweils eine komplette Tabelle mit Fehlernummern haben, um hoffen zu duerfen, dass jemand den Code ohne mich anpassen kann.

Ich kenne das gar nicht anders. Und wenn man ein zentrales Objekt oder Modul für's Error-Handling hat (das natürlich auch die Definitionen für sämtliche Fehlernummern des Projektes enthält) ist das doch auch gar kein Problem... Wie soll man sonst auch in einer zentralen Fehlerbehandlung festlegen, welche Fehler(-gruppen) wie behandelt werden - jeden möglichen Text abprüfen...?

Ciao, Stefanie

[suit]

jeden möglichen Text abprüfen...?

ob du einen text oder eine zahl (oder nur eine einzelne ziffer) als schlüssel für deine fehlertabelle oder dein fehlerarray verwendest ist egal

strings sind nur menschenlesbarer

stell dir html mit nummern vor ;)

<1>  
  <2>  
    <4>titel der seite</4>  
  </2>  
  <3>  
    <5>ich bin eine überschrift erster ordnung</5>  
    <6>ich bin ein textabsatz</6>  
  </3>  
</1>

[Stefanie]

Hi,

ob du einen text oder eine zahl (oder nur eine einzelne ziffer) als schlüssel für deine fehlertabelle oder dein fehlerarray verwendest ist egal

hm, stimmt, im Bezug auf PHP hast Du damit recht... Ich sah das halt mit meinen "C-Augen". Da hast Du den Nachteil durch die schlechtere Lesbarkeit nicht, weil enums oder defines benutzt werden (hoffentlich jedenfalls ;-) ), die darüber hinaus aber auch leichter zu händeln sind, weniger Speicher brauchen und sich bei Schreibfehlern bereits beim compilieren und nicht erst zur Laufzeit bemerkbar machen.

Ciao, Stefanie

[suit]

hm, stimmt, im Bezug auf PHP hast Du damit recht... Ich sah das halt mit meinen "C-Augen".

was hat eine datenbank in der eine fehlernummernzuorndungstabelle liegt mit php zu tun? das geht in sogut wie jedem dbms ;)

und in fast jeder scriptsprache lassen sich arrays mit nicht nummerischen keys erzeugen

[Steel]

Hi!

Ich rede von 1500 Zeilen Javascript in Windows' Notepad und nochmal soviel in ASP Dateien auch in Notepad.

[Andreas]

Hi suit,

vielen Dank für deine Ausführung.

Jetzt hab ich wieder etwas dazugelernt.

Vor allem das du auch immer eine plausible Begründung geliefert hast hat mir sehr geholfen, dass auch nachzuvollziehen. Und ich muss leider zugeben das ich wohl doch noch ein paar Dinge nicht gerade optimal eingesetzt habe.

Aber wie sagt man so schön: Besser gehts immer!

Gruß
Andreas

[dedlfix]

echo $begrüßung;

Das soll ein Login-Skript sein. Ganz einfach - Ich will das erstmal nnur hinbekommen, dass ich mich irgendwie einloggen kann. Aber bei diesem Skript zeigt er mir einfach kein Formular an. Was mache ich falsch?

Du betreibst vermutlich kein Debugging. Verwende Kontrollausgaben, um Wunsch und Wirklichkeit miteinander zu vergleichen. Desweiteren fehlt teilweise die Fehlerbehandlung.

Wenn gar keine Ausgabe kommt, kann man versuchen, ein die('test'); am Anfang einzubauen und zu schauen, ob diese Stelle erreicht wird. Dann versetzt man das immer weiter nach hinten. Wenn das "test" nicht mehr zu sehen ist, ist die fehlerhafte Stelle davor zu suchen.

Hier noch ein paar allgemeine Anmerkungen, auch auf Nachfrage von Andreas.

error_reporting(E_ALL);
ini_set('display_errors', 1);

Das nützt nichts mehr, wenn bereits Parser-Fehler auftraten, die aber aufgrund vorrangigerer Konfiguration nicht angezeigt werden.

$db = @new mysqli('***', '***', '***', '***');

Ein @ ist keine Fehlerbehandlung. Prüfe gemäß der Beispiele im Handbuch auf Fehlerbedingungen. Es interessiert sicher den Administrator, wenn das DBMS nicht verfügbar ist oder andere Fehler auftreten. Den Anwender interessiert das übrigens nicht, weswegen man die Fehlermeldung nicht direkt ausgeben sollte.

$ret = array();
$ret['filename'] = 'login2.html';
$ret['data'] = array();

Nebenbei: Das kann man zusammenfassen zu einer Zeile:

$ret = array('filename' => 'login2.html', 'data' => array());

if ('POST' == $_SERVER['REQUEST_METHOD']) {

Diese Prüfung ist im Prinzip überflüssig. Es reicht, den Inhalt der $_POST-Werte zu testen. Ohne POST-Request sind die nicht vorhanden.

if (!isset($_POST['Username'], $_POST['Password'], $_POST['submit'])) {
        return INVALID_FORM;
    }

Ein Submit-Button muss nicht unbedingt angeklickt worden sein, wenn man ein Formular mit Enter absendet. In dem Fall verhält sich der FF meiner Meinung nach falsch und tut so, als ob ein Submit-Button aktiviert wurde. Der IE macht es richtig und sendet für einen nicht geklickten Submit-Button auch dessen name-value-Pärchen nicht mit.

if (('' == $Username = trim($_POST['Username'])) OR
            ('' == $Password = trim($_POST['Password']))) {
        return EMPTY_FORM;
    }

Anstelle der drei obigen Prüfungen hätte ich lediglich auf empty($_POST['Username']) und empty($_POST['Password']) getestet. Leerzeichen zur Prüfung entfernen ist auch nicht erforderlich. Es geht hier um Anmeldedaten, dir korrekt sein müssen, nicht um versehentlich eingegebene Leerzeichen bei "normalen" Daten.

$stmt->close();

Wo kommt denn $stmt her, auf das du hier zuzugreifen versuchst?

$stmt = $db->prepare($sql);

Ach, hier wird es erst angelegt. Dann ist das obige Zugreifen ein Fehler.

if (!$stmt) {
        return $db->error;
    }

Sieh an, ab hier gibt es sogar Fehlerbehandlung. Zum Rest habe ich keine Anmerkungen.

echo "$verabschiedung $name";