Hallo,

ich programmiere jetzt seit ca 1 Jahr mit php/mysql und habe jetzt versucht ein Loginscript zu schreiben, was einigermaßen sicher ist.
Jetzt wollte ich wissen was ihr darüber denkt , sicher?, kann man die Performance noch irgendwie verbessern ?

kurz ein paar Daten zur Phpkonfiguration:
magic_quotes_gpc Off

session.cache_expire 60
session.cookie_httponly On
session.cookie_lifetime 0
session.gc_maxlifetime 7200
session.use_cookies On
session.use_only_cookies On

hier das Formular zum Einloggen start.php:

PHP-Code:

<?php
if($_GET[nichteingeloggt] == 'true')
{
echo '
Du must dich zuerst einloggen ******
';
}

if($_GET[logout] == 'true')
{
echo '
Sie haben sich erfolgreich ausgeloggt ******
';
}

echo '

<h2>Anmelden</h2>
<form method="POST" action="login.php">


<table cellspacing="1" cellpadding="2" border="0">
<tr>
';
if($_GET[benutzernameerror] == 'true')
{
echo '
<td><font size="2" color="red">Benutzername:</font></td>
<td><font size="2"><input type="text" name="user" value="">&nbsp;Unbekannter Benutzername</font></td>
';
}
else {
echo '
<td><font size="2">Benutzername:</font></td>
<td><font size="2"><input type="text" name="user" value=""></font></td>
';
}
echo '
</tr>
<tr>
';
if($_GET[passworderror] == 'true')
{
echo '
<td><font size="2" color="red">Passwort:</font></td>
<td><font size="2"><input type="password" name="password" value="">&nbsp;Falsches Password</font></td>
';
}
else {
echo '
<td><font size="2">Passwort:</font></td>
<td><font size="2"><input type="password" name="password" value=""></font></td>
';
}
echo '
</tr>
<tr>
<td colspan=2 align=center><input type=submit value=Anmelden></td>
</tr>
</table>
</form> 

';
?>

das Loginscript login.php:

PHP-Code:

<?php
include'connect.php'; 

if($_POST['user'] == '')
{
header("location:start.php?namefehlt=true");
exit();
}

if($_POST['password'] == '')
{
header("location:start.php?passwordfehlt=true");
exit();
}
$user = trim( $_POST['user'] );
$password = trim( $_POST['password'] );
$user = mysqli_real_escape_string($con, $user);
$sql = "SELECT `user` FROM `userdaten` WHERE `user` = '$user' AND `aktiviert` = 1";
$abfrage = mysqli_query($con, $sql);

if (mysqli_num_rows($abfrage) <= 0) 
{
mysqli_close($con);
header("location:start.php?benutzernameerror=true");
}
else
{
$sql = "SELECT * FROM `userdaten` WHERE `user` = '$user' AND `aktiviert` = 1";
$abfrage = mysqli_query($con, $sql);
    $row = mysqli_fetch_row($abfrage);
    if ($row[6] == 1)
    {
        header("location:start.php?acoountgesperrt=true");
        mysqli_close($con);
        exit();
    }
    if ($row[5] == 1)
    {
        header("location:start.php?logingesperrt=true");
        mysqli_close($con);
        exit();
    }
    if ($row[2]==sha1($password.$user)) 
    {
        session_start();
        session_unset();
        $_SESSION = array();
        session_regenerate_id(true);
        $_SESSION['user'] = $user;
        $_SESSION['user_id'] = $row[0];
        $_SESSION['user_info'] = strip_tags($_SERVER['HTTP_USER_AGENT']);
        $_SESSION['letzter_login'] = $row[7];
        $_SESSION['angemeldet'] = true;
        $sql = "UPDATE `userdaten` SET `fehlversuche` = 0, `letzter_login` = NOW() WHERE `user` = '$user' LIMIT 1";
        mysqli_query( $con, $sql );
        mysqli_close($con);
        header("location:sicher.php");
    }
    else
    {
if ($row[3] == 4)
{
    $sql = "UPDATE `userdaten` SET `login_sperre` = 1, `fehlversuche` = 0 WHERE `user` = '$user' LIMIT 1";
    mysqli_query( $con, $sql );
    mysqli_close($con);
    header("location:start.php?logingesperrt=true");
}
else
{
    $sql = "UPDATE `userdaten` SET `fehlversuche` = `fehlversuche` + 1 WHERE `user` = '$user' LIMIT 1";
    mysqli_query( $con, $sql );
    mysqli_close($con);
    header("location:start.php?passworderror=true");
}
    }
}

?>

und zum Schluss die geschützte Seite sicher.php:

PHP-Code:

<?php
session_start();

if(isset($_SESSION['angemeldet']) && $_SESSION['angemeldet'] == 'true')
{

if($_SESSION['user_info'] != strip_tags($_SERVER['HTTP_USER_AGENT']))
{
session_unset();
$_SESSION = array();
session_destroy();
header("location:start.php");
exit();
}

session_regenerate_id(true);
/* hier kommt der geschützte inhalt hin*/
else {

header("location:start.php?nichteingeloggt=true");

}
?>

wo ich mir jetzt noch unsicher bin ist ob ich die session_id bei jedem aufrufen oder nach zb 5 min erneuern soll ( wegen der Performance ) und ob ich bei Falschem Benutzernamen dem User sagen soll dass er falsch war oder einfach nur einen allgemeinen Fehler ausgeben soll. ( Wenn ein Angreifer weis dass der Name existiert, kann er versuchen das PW herauszufinden ).

Kazuma

Hallo kazuma,
ich Persönlich finde dein Vorgehensweise sehr umständlich und überfüllt.
bsp. Auf deinem Formular, die Fehler würde ich eher in einem Array speichern und dann dort ausgeben. Ist aber wohl jedem seins.

Dann ist mir etwas in deinem "login.php" aufgefallen.
Folgende Zeilen

PHP-Code:

$sql = "SELECT `user` FROM `userdaten` WHERE `user` = '$user' AND `aktiviert` = 1";
$abfrage = mysqli_query($con, $sql);

if (mysqli_num_rows($abfrage) <= 0) 
{
mysqli_close($con);
header("location:start.php?benutzernameerror=true");
}
else
{
$sql = "SELECT * FROM `userdaten` WHERE `user` = '$user' AND `aktiviert` = 1";
$abfrage = mysqli_query($con, $sql);
    $row = mysqli_fetch_row($abfrage); 


Du musst nicht 2 Querys dafür verwenden, es reicht eines bsp.:

PHP-Code:

$sql = "SELECT * FROM `userdaten` WHERE `user` = '$user' AND `aktiviert` = 1";
$abfrage = mysqli_query($con, $sql);
        if (mysqli_num_rows($abfrage) <= 0) 
        {
        mysqli_close($con);
        header("location:start.php?benutzernameerror=true");
        }
$row = mysqli_fetch_row($abfrage); 


Nur weil du eine num_rows abfrage machst, wird die Variable $sql nicht geleert.

Dann weiter in Zeile 46

PHP-Code:

session_unset();
        $_SESSION = array(); 


Mit $_SESSION = array(); machst du die Variable leer, mit session_unset() aber auch, also eins reicht. Wobei session_unset besser finde.

Desweiteren finde ich es Persönlich umständlich

PHP-Code:

$row[1] 


etc. zu verwenden. So hast du selbst in Zukunft keinen Überblick, stell dir einmal vor du nimmst eine änderung an der Tabelle vor. Und Plötzlich hat $row[5] nichts mehr mit der Loginsperre zutun. Benutz lieber den Spaltennamen, wie zum beispiel "loginsperre" oder wie auch immer du die Spalte benannt hast.

Dann noch in deiner Sicher.php

PHP-Code:

if(isset($_SESSION['angemeldet']) && $_SESSION['angemeldet'] == 'true') 


Es reicht auch

PHP-Code:

if($_SESSION['angemeldet'] == 'true') 


Denn wenn die Variable "true" enthält existiert diese auch, wenn "false" dann auch, wenn NULL dann eben nicht. Und es wird die else abgearbeitet.

Und auch

PHP-Code:

session_unset();
    $_SESSION = array();
    session_destroy(); 


Ist zwar gut das du die SESSION leer haben möchtest, aber session_unset und session_destroy machen in meinen Augen das selbe. Also eins davon würde reichen.

Das wars erstmal nach dem Überfliegen, sollte ich mich Irren oder Falsche Dinge erzählen, Berichtigt mich bitte.
Mfg Splater

Hallo ihr beiden,

ich möchte mich dann auch mal mit reinhängen und dich Splater korrigieren, denn folgende Zeile ist so, wie sie von kazuma verwendet wird, korregt:

Code PHP:

1	if(isset($_SESSION['angemeldet']) && $_SESSION['angemeldet'] == 'true')

Würde man das isset() weglassen, dann würde das in dem Fall, dass der Index angemeldet nicht gesetzt wurde, zu einer Meldung vom Typ NOTICE führen. Diese Anzeige kann man zwar abschalten, aber als guter Programmierer will man letztendlich überhaupt keine Meldungen erhalten.

Ansonsten sei noch gesagt, dass folgendes falsch ist:

Code PHP:

1	$_GET[nichteingeloggt]

Es müsste so aussehen:

Code PHP:

1	$_GET['nichteingeloggt']

PHP ist zwar so tolerant und wandelt nicht definierte Konstanten in deren entsprechenden Namen um, aber das ändern nichts daran, dass es einfach nur falsch ist.

danke für die Tipps , werde sie gleich umsetzen =)

$row[1] habe ich deswegen genommen , weil ich gelesen habe , dass diese Anweisung am schnellsten ist.

$_SESSION = array(); habe ich deshalb noch genommen, weil ich auf nummer sicher gehen wollte

$_GET['nichteingeloggt'] wurde korrigiert

jetzt bleibt noch eine Frage , soll ich die session id bei jedem Seitenaufruf neu generieren oderzb eine Funktion schreiben dass sie nur alle 5 min neu generiert wird ?

Ich wüsste spontan keinen Grund, warum sie innerhalb einer Sitzung überhaupt neugeneriert werden sollte. Dafür ist es doch eine Sitzungskennungsnummer (Session ID).

aus sicherheitstechnischen Gründen, falls jemand an die Sessionid kommt

Wie soll denn bitte jemand an die Sitzungskennung kommen? Das geht nur per XSS, aber das solltest du zu verhindern wissen, dann kann auch niemand die ID ermitteln (es sei denn jemand sendet seine eigene ID einem Anderen, woran er dann aber selber Schuld hat). Am besten prüfst du noch die IP-Adresse. Damit kannst du die Anzahl der Möglichen "Täter" auf ein Minimum beschränken.

XSS verhindere ich mit htmlspecialchars() oder strip_tags()

Solange du die Sitzungskennung in einem Cookie speicherst, brauchst du sie auch nicht innerhalb einer Situzng ändern.

soweit ich weiß kann man in php5 selbst mit der session id nichts anfangen(sessionlogin in dem falle) wenn man das nich selbst will. Ich hatte vor einiger Zeit aus Testgründen grade im Gegenteil genau dass gewollt, dass man über SessionShare login ins teilen kann, und dass muss man seperat einstellen(session_start($id)) ansonsten kann man damit nichts anfangen