Aufbau eines Loginscripts

[Sylnois]

Heyho Leute
Habe mir wieder ein bisschen Zeit genommen PHP zu lernen.
Ich bin also noch ein Newcomer was PHP angeht.
Ich möchte euch mein erstes Loginscript gerne präsentieren.
Könntet ihr bitte Verbesserungsvorschläge bringen.
Kontrollieren ob das Skript gut oder eher schlecht aufgebaut ist.
Vielen Dank
Sylnois

Loginscript:

<?php
    // Session wird gestartet
    session_start(); 
    
    // Verbindung zur Datenbank
    require_once("config.php");
    
    if(!isset($_SESSION['fail'])){
        $_SESSION['fail'] = 0;
    }
    if(!isset($_SESSION['fail2'])){
        $_SESSION['fail2'] = 0;
    }
    
    if(!isset($_SESSION['user'])){
            if(empty($_GET['user']) == TRUE OR empty($_GET['pw'])){
                if($_SESSION['fail'] == 1){
                    echo"User wurde nicht gefunden!";
                    $_SESSION['fail'] = 0;
                }
                if($_SESSION['fail2'] == 1){
                    echo"Falsche Logindaten";
                    $_SESSION['fail2'] = 0;
                }
                if($_GET['controll'] == 1){
                    echo "Bitte alle Felder ausfuellen.";
                    $_GET['controll'] = 0;
                }
                echo'
                <form action="" method="GET" name="Login" id="Login">
                
                <p>Username:
                <input type="hidden" name="controll" id="controll" value="1">
                <input type="Text" name="user" id="user" value="'. $_GET['user'] .'" size="30" maxlength="50" /></p>
                <p>Password:
                <input type="password" name="pw" id="pw" value="" size="30" maxlength="50" /></p>
                <input type="Submit" name="bLogin" id="bLogin" value="Login" />
                </form>
                ';
            }
            else{
                $user = $_GET['user'];
                $pw = md5($_GET['pw']);
                $result = mysql_query("SELECT name, pw FROM login WHERE name LIKE '$user' LIMIT 1");
                $Daten = mysql_fetch_object($result);
                if(mysql_num_rows($result) != 1){
                    $_SESSION['fail'] = 1;
                }
                else{
                    if($Daten->pw == $pw){
                        $_SESSION["user"] = $user;
                    }
                    else{
                        $_SESSION['fail2'] = 1;
                    }
                }
                echo'<meta http-equiv="refresh" content="0; URL=index.php">';
            }
    }
    else{
        if(!isset($_GET['bLogout'])){
            echo"Herzlich Willkommen " . $_SESSION['user'];
            echo'
            <form action="" method="GET" name="Logout" id="Logout">
            <input type="Submit" name="bLogout" id="bLogout" value="Logout" />
            </form>';
        }
        else{
            session_destroy();
            echo'<meta http-equiv="refresh" content="0; URL=index.php">';
        }
    }
?>

 

[DarkDragon193]

1. Du sendest das Formular mittels GET ab. Das ist in der Hinsicht schlecht, dass Menschen, die hinter dem Benutzer stehen, das eingegebenen Passwort lesen können. Das ist ja wohl kaum Sinn der Sache... ;) Besser ist es, das über POST zu versenden.

2.

SELECT name, pw FROM login WHERE name LIKE '$user' LIMIT 1

Das LIKE finde ich auch nicht gut... zumal erstens das ziemlich unsicher ist und zweitens ein falscher Benutzer gefunden werden könnte, wenn 2 Benutzer einen ähnlichen Namen haben.

3.

echo'<meta http-equiv="refresh" content="0; URL=index.php">';

Solange du keine Ausgabe auf dem Bildschirm hast, was du bei den 2 Verwendungen diesen Befehls nicht hast, ist es, denke ich, einfacher

header( "location:index.php" );

zu verwenden.

Ansonsten habe ich nichts zu bemängeln. Aber verlass dich nicht 100% darauf... Man kann ja nie wissen, was ich übersehen habe. ;D

 

[struppi]

Das Skript ist anfällig für SQL Injections

 

[Sylnois]

1. Du sendest das Formular mittels GET ab. Das ist in der Hinsicht schlecht, dass Menschen, die hinter dem Benutzer stehen, das eingegebenen Passwort lesen können. Das ist ja wohl kaum Sinn der Sache... Besser ist es, das über POST zu versenden.

Geht klar Werde es dann updaten.

2. SELECT name, pw FROM login WHERE name LIKE '$user' LIMIT 1


Das LIKE finde ich auch nicht gut... zumal erstens das ziemlich unsicher ist und zweitens ein falscher Benutzer gefunden werden könnte, wenn 2 Benutzer einen ähnlichen Namen haben.

so besser: Select name, pw FROM login WHERE name='$user' LIMIT 1 ?

3. echo'<meta http-equiv="refresh" content="0; URL=index.php">';


Solange du keine Ausgabe auf dem Bildschirm hast, was du bei den 2 Verwendungen diesen Befehls nicht hast, ist es, denke ich, einfacher
PHP-Code:
header( "location:index.php" );


zu verwenden.

Dann kommt jedoch der Fehler:

Warning: Cannot modify header information - headers already sent by (output started at /users/teddibaer/www/login/config.php:17) in /users/teddibaer/www/login/index.php on line 57

Danke
Sylnois

 

[DarkDragon193]

Ich sagte ja

Solange du keine Ausgabe auf dem Bildschirm hast

Was du ja anscheinend in der config.php hast, was mir neu ist... Für gewöhnlich hat man da keine Ausgabe. Wenn du sie nicht brauchst, kannst du sie ja entfernen.

Stimmt, die MySQL-Vulnerability ist mir ja noch gar nicht aufgefallen. Da brauchst du wohl was wie htmlentities oder htmlspecialchars. Vielleicht auch noch einfache und doppelte Anführungszeichen entfernen... Was es da eben alles gibt.

€dith: Zur Frage: Ja, besser. :)

 

[mermshaus]

Stimmt, die MySQL-Vulnerability ist mir ja noch gar nicht aufgefallen. Da brauchst du wohl was wie htmlentities oder htmlspecialchars. Vielleicht auch noch einfache und doppelte Anführungszeichen entfernen... Was es da eben alles gibt.

Nein, mysql_real_escape_string und nichts anderes.

htmlspecialchars (und htmlentities) dient dazu, Daten für die Ausgabe in HTML-Code zu escapen (Stichwort: Cross-Site Scripting).

- Cross-Site Scripting

 

[struppi]

Stimmt, die MySQL-Vulnerability ist mir ja noch gar nicht aufgefallen. Da brauchst du wohl was wie htmlentities oder htmlspecialchars. Vielleicht auch noch einfache und doppelte Anführungszeichen entfernen... Was es da eben alles gibt.

Nein PHP: SQL Injection - Manual

 

[DarkDragon193]

Dann hat der falsche Ansatz ja gewirkt. x) Ich hab gehofft, ihr würdet mit sowas kommen... Zugegeben, etwas blöd, aber gut.