Nochmal: Ist dieses Login-Script sicher

code_junkie · #1 (**permalink**) 03.11.2007, 14:26

Hallo..

Auch mich würde interessieren, ob mein Login-Script sicher ist...
Die Technik dahinter ist recht simpel...
Es wäre nett, wenn Du mal drüber schauen könntest, ob ich irgendwas nicht bedacht habe!

Hier der Code:

PHP-Code:


			
// Annahme der Formulardaten (Login Button, Username, Passwort)
// Wenn leer, werden die Variablen auf false gesetzt
$nws_login    = empty($_POST["login_form"]) ? false : f_ilter_input($_POST["login_form"], "textonly");
$nws_username = empty($_POST["username"])   ? false : f_ilter_input($_POST["username"], "textonly");
$nws_passwd   = empty($_POST["passwd"])     ? false : f_ilter_input($_POST["passwd"], "textonly");


// Wenn der User auf "Login" geklickt hat:
if ($nws_login === "Go") {

    // Wenn Username und Passwort nicht gesetzt sind,
    // wird eine Fehlermeldung in $emsg gespeichert, ansonsten das PW überprüft

    if (!$nws_username || !$nws_passwd)  {

        $emsg = "<b>Username und Password? Beides "
            . "notwendig um sich hier einzuloggen.</b>";

    } elseif (login_sys_admin_check($nws_username, $nws_passwd) === true) {

        // Obige Funktion überprüft Username & PasswortRückgabe: true/false
        // also: Hier stimmt das Passwort, es werden jetzt zwei Cookies gesetzt..
        // Diese Cookies haben aber nichts mit der Loginfunktion an sich zu tun,
        // sondern dienen, um den Namen beim Betreten der Seite darzustellen..

        $time_int = (time() + 86400 * 31 * 6);
        if ($usertrack_numlogins <= 0) { $usertrack_numlogins = 0; }
        $usertrack_numlogins++;
        setcookie("user", $nws_username, $time_int);
        setcookie("logins", $usertrack_numlogins, (time() + 86400 * 2500));

    } else { 

        // die Login-Funktion hat false zurückgegeben:
        // 
        $nws_login = $nws_passwd = "";
        $emsg = "<b>Bad Username or Password!</b>";

    }

    // nun wird geprüft, ob es einen Fehler gab

    if (isset($emsg)) {

        // Anzahl der Loginversuche wird in Session protokolliert:

        if (empty($_SESSION["bad_lp_counter"])) $_SESSION["bad_lp_counter"] = 0;
        $_SESSION["bad_lp_counter"]++;

        // Und eine Fehlermeldung angezeigt        

        do_error(3,$emsg."<br /><br />Login try number " 
                . $_SESSION["bad_lp_counter"] . ".");

        // do_error() beendet das Skript mit die() !

    }

} elseif (isset($_SESSION['IP']) && $_SESSION['IP'] !== get_user_ip()) {

    // der User hat *nicht* auf Login geklickt
    // Aaaber die aktuelle IP-Adresse des Users stimmt nicht
    // mit der in der SESSION gespeicherten überein.
    // (wird von [I]login_sys_admin_check()[/I] gespeichert)

    // folglich wird der User ausgeloggt

    if (isset($_COOKIE[session_name()]))     setcookie(session_name(), '', time()-42000, '/');
    $_SESSION = array();
    session_destroy();
    do_error(3,"<p>Ihre IP hat sich seit dem letzten Login verändert. "
        ."Aus Sicherheitsgründen wurden Sie automatisch ausgeloggt!</p>"
        ."<p> Sie können sich mit ihrem Passwort erneut einloggen.</p>");

}

Die dazugehörigen Funktionen login_sys_admin_check() zum Überprüfen von Username/Passwort und f_ilter_input() zum Filtern der _POST Variablen:

PHP-Code:


			
/******************************************************************************
*  returns false or true if the session password matches
*******************************************************************************/
function login_sys_admin_check($s_username="", $s_passwd="") {
    global $nws_mysql_db;
    $db = $nws_mysql_db . '.userdb';

    $result = mysql_cr("SELECT uid, nick, pw, name, name2, email, gender,
                ulevel, titel FROM $db WHERE `nick` = '$s_username'");

    // Wenn $result ein Array ist, und der Name
    // Sowie der Passwort-Hash übereinstimmen,
    if (
        is_array($result) === true
        && (strtolower($s_username) === strtolower($result[1])
        && crypt($s_passwd, $result[2]) === $result[2])
    ) { 
        // Dann speichere ich alles mögliche in der Session
        $_SESSION['uid'] = (int) $result[0];
        $_SESSION['nick'] = $result[1];
        $_SESSION['realname'] = $result[3];
        $_SESSION['realname2'] = $result[4];
        $_SESSION['email'] = $result[5];
        $_SESSION['gender'] = (int) $result[6];
        $_SESSION['level'] = (int) $result[7];
        $_SESSION['IP'] = get_user_ip();
        if ($result[8]) $_SESSION['titel'] = $result[8]; // "Titel" kann leer sein
        unset($_SESSION['bad_lp_counter']); 
        // alles paletti: true
        return true;
    }

    // standard-rückgabewert ist false
    unset($result);
    return false;
}

// Bedarf dies einer weiteren Erklärung?
// Funktion löscht alles ausser a-zA-Z0-9 - _ aus einem String.
function f_ilter_input($type, $text) {
    switch ($type) {
        case "textonly":
            $text = filter_var($text, FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES | FILTER_FLAG_STRIP_HIGH | FILTER_FLAG_STRIP_LOW);
            $text = preg_replace("/[^-_0-9a-z]/iu","",$text);
        break;
    }
}

Irgendwelche Lücken oder Unzulänglichkeiten ersichtlich?

Gruß

ct2oo4 · #2 (**permalink**) 03.11.2007, 14:42

Warum neuer Thread?
Und etwas besser erläutern könntest du das Ganze auch!
MfG

inta · #3 (**permalink**) 03.11.2007, 14:43

Und ich würde dir folgendes empfehlen:
PEAR :: Manual :: Coding Standards

Ich empfinde deinen Code als sehr schwer lesbar.

code_junkie · #4 (**permalink**) 03.11.2007, 14:49

Ja warum ein neuer Thread.. Wenn in einem Thread über verschiedene Scripte gesprochen wird, wird das ein wenig unübersichtlich, dachte ich mir.
Ich werde jetzt ein paar mehr Kommentare einfügen und etwas whitespace einfügen, um es besser lesbar zu machen.

code_junkie · #5 (**permalink**) 03.11.2007, 15:36

Hoffe mit den Änderungen entspricht das jetzt ungefähr deinen Vorstellungen inta.

nochmal @ct2oo4: In dem anderen aktuellen Thread wird ja ein durchaus nicht unkompliziertes Verfahren allein zur Hashbildung angewandt... Insofern interessiert es mich schon, ob solch einfache Prüfungen wie bei mir ausreichend sind oder nicht. Eigentlich kann ich mir kaum vorstellen wie man da herum kommen könnte, daher wundert es mich ein wenig, dass andere Scripte teilweise einen großen Aufwand betreiben.. (Damit meine ich jetzt nicht explizit das Script im angesprochenem Thread.. eher generell.)

Nun ja, ich lasse mich gerne eines besseren belehren!

Gruß

inta · #6 (**permalink**) 03.11.2007, 16:13

Es geht ja nicht um mich, in erste Linie musst du mit deinem Code klarkommen. Einen ordentlichen Stil zu entwickeln ist aber nicht verkehrt, ich würde noch auf die Kurzschreibweisen verzichten.

Ich hab dein Login jetzt nur mal überflogen, aber folgendes solltest du zumindest ändern:

Code:

WHERE `nick` LIKE '$s_username'

Davon abgesehen, dass LIKE natürlich langsam ist, macht es an der Stelle keinen Sinn. Du möchtest genau den User, der sich gerade versucht einzuloggen und nicht irgendeinen ähnlichen Nickname, also frag es auch so ab

Warum einige das Passwort verschlüsseln verstehe ich ehrlich gesagt nicht. Als Betreiber einer Seite brauch ich das Passwort nicht wissen, also brauche ich auch keine Möglichkeit es zu entschlüsseln. Ein Hash wie md5 ist da mehr als ausreichend und nur durch Brute-Force-Attacken zu "knacken" (eigentlich nur irgendeine Kollision zu erzeugen).

Warum setzt du eigentlich Cookies? Die Daten speicherst du doch schon in der Session, dann würd ich gleich alles über die Session abfackeln.

code_junkie · #7 (**permalink**) 03.11.2007, 16:26

Ich komme mit meinem Code ohne Leerzeilen wunderbar zurecht, und bin Fan von Kurzschreibweisen wie (expr ? true : false) oder <?=$foo?>

Aber wenn ich hier um Rat frage, denk ich mir, sollte ich mich auf die Befragten einstellen

Das mit dem LIKE ist mir auch gerade aufgefallen, als ich im anderen Thread las, dass LIKE % und _ als Sonderzeichen akzeptiert. Ist schon ein "=" draus geworden.

Zum Verschlüsseln ... da stehe ich auf deinem Standpunkt: eine Einweg-Verschlüsselung reicht.
Was haltet Ihr denn von solchen Verfahren zur Hashbildung - übertrieben oder notwendig? Als Beispiel: Portable PHP password hashing framework

Zitat:

Warum setzt du eigentlich Cookies? Die Daten speicherst du doch schon in der Session, dann würd ich gleich alles über die Session abfackeln.

Die Cookies "User" und "numlogins" sind quasi nur Gimmick.. User wird wenn man nicht angemeldet ist im Loginformular angezeigt, und numlogins wird über die Sessions hinweg gezählt - bis der User seine Cookies löscht

Das wird nur auf der Sitemap ausgegeben a la "schon x mal angemeldet, ich hoffe sie finden sich zurecht." Also .. belanglos (ps: Wird natürlich nicht unvalidiert ausgegeben.)

Gruß

Gumbo · #8 (**permalink**) 03.11.2007, 17:17

Verschlüsselungen können gewöhnlich auch wieder entschlüsselt werden, was bei Hash-Werten jedoch nicht der Fall ist. Deswegen ist es auch falsch, hier von einer Verschlüsselung zu sprechen.
Allgemein ist es jedoch nicht nötig, ein Kennwort zu verschlüsseln, da auch Hash-Werte die Identität eines eingegebenen Kennworts verifizieren können. Sind dies zusätzlich so genannte Salted-Hash-Werte, können sie auch nicht mit den Hash-Werten anderer Kennwörter verglichen werden um so mögliche identische Kennwörter auf unterschiedlicher Systemen festzustellen.
Ich benutze da gerne folgende Funktion, bei der der Salt-Wert zusätzlich zufällig ist, wodurch jeder Aufruf ein anderen Hash-Wert erzeugt:

PHP-Code:


			
function generateSaltedHash( $data, $salt=null )
{
    if( is_null($salt) ) {
        $salt = substr(md5(uniqid(rand())), 0, 8);
    }
    return $salt.md5($salt.$data);
}
function checkSaltedHash( $data, $hash )
{
    return $hash === generateSaltedHash($data, substr($hash, 0, 8));
}

inta · #9 (**permalink**) 03.11.2007, 19:53

Ich hab mich ne ganze Weile gefragt wie du später wieder an das salt kommst, bevor ich gesehen hab, dass du es dem Hash voranstellst. Schicke Funktion, gut ausgeknobelt, es lohnt sich diese Methode zu übernehmen

spacegaier · #10 (**permalink**) 04.11.2007, 11:43

Deine Funktion ist so definiert:

PHP-Code:


			
function f_ilter_input($type, $text)

Also erst der Type und dann der Text, der kontrolliert werden soll.

Bist du dir hier bei den übergebenen Variablen sicher?

PHP-Code:


			
$nws_login    = empty($_POST["login_form"]) ? false : f_ilter_input($_POST["login_form"], "textonly");

Sieht für mich so aus, als ob du zuerst den Text un dann den Typ übergeben würdest, also genau andersrum als definiert.

Ähnliche Themen
Thema	Autor	Forum	Antworten	Letzter Beitrag
PHP Login Script	newbie88	Serveradministration und serverseitige Scripte	10	21.05.2011 14:45
Login Script verändern	andre-ne	Serveradministration und serverseitige Scripte	13	08.04.2011 17:39
Login Script	bluub	Serveradministration und serverseitige Scripte	4	11.01.2010 18:08
PHP Script mit GET-Parametern als Img-Src	Ares	Serveradministration und serverseitige Scripte	2	13.09.2008 21:19
PHP Login Script	Pixel Design	Serveradministration und serverseitige Scripte	19	19.01.2007 13:36