Ankündigung

Einklappen
Keine Ankündigung bisher.

Ist dies eine sichere Loginklasse?

Einklappen

Neue Werbung 2019

Einklappen
X
  • Filter
  • Zeit
  • Anzeigen
Alles löschen
neue Beiträge

  • Ist dies eine sichere Loginklasse?

    Guten Tag.

    Ich bin noch relativ ein newbie was PHP angeht. Aus diesem Grund ist es für mich nicht so einfach zu erkennen ob diese lösung wirklich etwas taugt. Ist es möglich das sich jemand den Code meiner Loginklasse nach Sicherheitslücken checkt? Auch für Optimierungsvorschläge bin ich offen

    Als Vorlage hab ich mir eine Klasse aus dem phpforum wiki genommen. Login-System / Authentifizierung - Forum: phpforum.de
    Sie ist aber zum grössten Teil neu geschrieben.

    Danke im voraus für Eure Antworten.

    PHP-Code:
    <?php

        
    class Login {
            
            private 
    $mysql_hostname '';
            private 
    $mysql_username '';
            private 
    $mysql_password '';
            private 
    $mysql_database '';
            
            private 
    $dbLink;
            
            private 
    $falseUserData 'Login fehlgeschlagen! Bitte überprüfen Sie Ihre Zugangsdaten! <a href="javascript:history.back(-1);">Zurück</a>';
            
            public function 
    __construct() {
                
    $dbLink = new MySQLi($this->mysql_hostname$this->mysql_username$this->mysql_password$this->mysql_database
                or die(
    'Connection Error:  ' mysqli_error());
                
    $this->dbLink $dbLink;
                return 
    $dbLink;
            }
            
            public function 
    valid() {
                try {
                    if(!isset(
    $_POST['login'])) {
                        return 
    FALSE;
                    }else{
                        if (empty(
    $_POST['username']) OR empty($_POST['password'])) {
                            throw new 
    Exception($this->falseUserData);
                        } else {
                            return 
    $this->is_logged_in();
                        }
                    }
                } catch (
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            private function 
    is_logged_in() {
                try {
                    if (isset(
    $_POST['login'])) {
                        
    $username addslashes(htmlentities($_POST['username']));
                        
    $password hash('SHA512'$_POST['password']);
                        
                        
    $sql '
                            SELECT
                                id, 
                                status
                            FROM account 
                            WHERE 
                                (username = ?) 
                            AND 
                                (pwHash = ?)
                        '
    ;
                        
    $stmt $this->dbLink->prepare($sql);
                        
    $stmt->bind_param('ss'
                            
    $username
                            
    $password
                        
    );
                        
    $stmt->execute();
                        
    $stmt->store_result();
                        
                        if (
    $stmt->num_rows 0) {
                            
    $stmt->bind_result(
                                
    $id,
                                
    $status
                            
    );
                            
    $stmt->fetch();
                            switch (
    $status) {
                                case 
    'active':
                                    
    $_SESSION['username'] = $username;
                                    
    $_SESSION['id'] = $id;
                                    echo 
    'Sie wurden erfolgreich eingeloggt!<br /><a href="'.$_SERVER['PHP_SELF'].'">Weiter zur Seite</a>';
                                    return 
    TRUE;
                                    break;
                                case 
    'block':
                                    throw new 
    Exception('Ihr Account wurde gesperrt!');
                                    return 
    FALSE;
                                    break;
                                case 
    'inactive':
                                    throw new 
    Exception('Ihr Account ist momentan nicht aktiviert. Bitte bestätigen Sie Ihre Aktivierungs-Email!');
                                    return 
    FALSE;
                                default:
                                    throw new 
    Exception('Unbekannter Fehler!');
                                    return 
    FALSE;
                                    break;
                            }
                        } elseif (
    $stmt->num_rows() < 1) {
                            throw new 
    Exception($this->falseUserData);
                            return 
    FALSE;
                        }
                    } else {
                        throw new 
    Exception('Authifizierung erforderlich!');
                        return 
    FALSE;
                    }
                } catch(
    Exeptions $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            public function 
    logout() {
                try {
                    if (isset(
    $_SESSION['id'])) {
                        
    $_SESSION = array();
                        
    session_destroy();  
                        
    session_regenerate_id();
                        unset(
    $_SESSION);
                        echo 
    'Sie wurden abgemeldet';
                        return 
    TRUE;
                    } else {
                        throw new 
    Exception('Sie können sich nicht abmelden');
                        return 
    FALSE;
                    }
                } catch (
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            public function 
    check_login() {
                if (isset(
    $_SESSION['id'])) {
                    return 
    TRUE;
                }
            }
            
            public function 
    change_password($userID$newPw) {
                try {
                    if (
    $userID) {
                        
    $newPw hash('SHA512' ,$newPw);
                        
    $sql '
                            UPDATE account
                            SET pwHash = ?
                            WHERE id = ?
                        '
    ;
                        
    $stmt $this->dbLink->prepare($sql);
                        
    $stmt->bind_param('ss',
                            
    $newPw,
                            
    $userID
                        
    );
                        
    $stmt->execute();
                        echo 
    'Passwort wurde erfolgreich geändert';
                        return 
    TRUE;
                    } else {
                        throw new 
    Excetion('Es war nicht möglich das Passwort zu ändern!');
                        return 
    FALSE;
                    }
                } catch(
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            public function 
    ban_user($userID) {
                try {
                    if(
    is_int($userID)) {        
                        
    $status 'block';
                        
    $sql '
                            UPDATE account
                            SET status = ?
                            WHERE id = ?
                        '
    ;
                        
                        
    $stmt $this->dbLink->prepare($sql);
                        
    $stmt->bind_param('si',
                            
    $status,
                            
    $userID
                        
    );
                        
    $stmt->execute();
                        echo 
    'Benutzer '.$_SESSION['username'].' wurde für unbestimmte Zeit gebannt';
                    } else {
                        throw new 
    Exception('Es ist ein Fehler aufgetreten! (banUser)');
                    }
                } catch(
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            public function 
    delete_user($userID) {
                try {
                    if(
    is_int($userID)) {
                        
    $sql '
                            DELETE FROM login
                            WHERE id = ?
                        '
    ;
                        
    $stmt $this->dbLink->prepare($sql);
                        
    $stmt->bind_param('i',
                            
    $userID
                        
    );
                        
    $stmt->execute();
                        return 
    TRUE;
                    } else {
                        throw new 
    Exception('Es ist ein Fehler aufgetreten! (deleteUser)');
                        return 
    FALSE;
                    }
                } catch(
    Exception $e) {
                    echo 
    $e->getMesage();
                }
            }
            
            public function 
    activate_user($activationHash) {
                try{
                    
    //Daten richtig entschlüsseln (noch fehlerhaft)!!!!
                    
    $hashEmail substr($activationHash064);
                    
    $hashID substr($activationHash64,11);
                    
    $sql '
                        SELECT
                            id,
                            username,
                            email,
                            status 
                        FROM account
                        WHERE id = ?
                    '
    ;
                    
    $stmt $this->dbLink->prepare($sql);
                    
    $stmt->bind_param('i',
                        
    $hashID
                    
    );
                    
    $stmt->execute();
                    
    $stmt->store_result();
                    
    $stmt->bind_result(
                        
    $userID,
                        
    $username,
                        
    $email,
                        
    $status
                    
    );
                    
    $stmt->fetch();
                    if(
    hash('SHA256'$email) == $hashEmail) {
                        if(
    $status == 'inactive') {
                            
    $newStatus 'active';
                            
    $updSql '
                                UPDATE account
                                SET status =  ?
                                WHERE id = ?;
                            '
    ;
                            
    $stmt $this->dbLink->prepare($updSql);
                            
    $stmt->bind_param('ss',
                                
    $newStatus,
                                
    $userID
                            
    );
                            
    $stmt->execute();
                            return 
    TRUE;
                        } else {
                            throw new 
    Exception('Der Benutzer '.$username.' wurde bereits Aktiviert');
                            return 
    FALSE;
                        }
                    } else {
                        throw new 
    Exception('Ungültiger Aktivierungscode');
                        return 
    FALSE;
                    }
                } catch(
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
            
            public function 
    insert_user($newName$newPw$newEmail) {
                try {
                    
    // überprüfung ob der Name oder die Email Adresse schon existieren um eine Fehlermeldung auszugeben
                    
    $selSql '
                        SELECT id
                        FROM account
                        WHERE username = ?
                    '
    ;
                    
    $stmt $this->dbLink->prepare($selSql);
                    
    $stmt->bind_param('s',
                        
    $newName
                    
    );
                    
    $stmt->execute();
                    
    $stmt->store_result();
                    if(
    $stmt->num_rows() > 0) {
                            throw new 
    Exception('Dieser Benutzername existiert bereits! Bitte wählen Sie einen anderen.');
                    } else {
                        
    $validEmail trim(stripslashes(strtolower($newEmail)));
                        if(
    strstr($validEmail'@') && strstr($validEmail'.') && strlen($validEmail) >= 6) {        
                            
    $newName addslashes(htmlentities($newName));
                            
    $newPw hash('SHA512'$newPw);
                            
    $status 'inactive';
                            
    $insSql '
                                INSERT INTO account
                                (
                                    username,
                                    pwHash,
                                    email,
                                    status
                                ) 
                                VALUES 
                                (
                                    ?,
                                    ?,
                                    ?,
                                    ?
                                )
                            '
    ;
                            
    $stmt $this->dbLink->prepare($insSql);
                            
    $stmt->bind_param('ssss',
                                
    $newName,
                                
    $newPw,
                                
    $newEmail,
                                
    $status
                            
    );
                            
    $stmt->execute();
                            
    $selSql '
                                SELECT id
                                FROM account
                                WHERE username = ?
                            '
    ;
                            
    $stmt $this->dbLink->prepare($selSql);
                            
    $stmt->bind_param('s',
                                
    $newName
                            
    );
                            
    $stmt->execute();
                            
    $stmt->store_result();
                            
    $stmt->bind_result(
                                
    $userID
                            
    );
                            
    $stmt->fetch();
                            
    $activationHash hash('SHA256'$newEmail).$userID;
                            
    $emailContent 'Aktivierungslink für '.$newName.'<br /><a href="http://localhost/register.php?activationCode='.$activationHash.'>Aktivieren</a>';
                            
    $emailSubject 'Aktivierung';
                            
    $emailRecipient 'sgrossgl@gmail.com';
                            
    $emailHeader 'FROM: Steve';
                            
    $sendEmail = @mail($validEmail$emailSubject$emailContent$emailHeader);
                            if(!
    $sendEmail) {
                                throw new 
    Exception('Es ist ein Fehler beim Absenden der Aktivierungsmail aufgetreten.');
                                return 
    FALSE;
                            } else {
                                return 
    TRUE;
                            }
                        } else {
                            throw new 
    Exception('Die gewählte Mail-Adresse ist ungültig!');
                        }
                    }
                } catch(
    Exception $e) {
                    echo 
    $e->getMessage();
                }
            }
        }
    Grüsse
    Hexagone


  • #2
    Sicherheitstechnisch will ich jetzt nichts sagen, dazu habe ich nicht genug analysiert, aber technisch fallen mir eine Menge Unschönheiten auf:

    Allem zuerst die schlechte Verwendung von Exceptions. Eine Exceptions ist keine Fehlerspeicher. Das solltest DU lieber in Deinem Auth-Objekt ablegen. Auch in einer Methode nen try-Block anlegen, um die Exception der Untermethode als Statusmeldung zu fangen, finde ich eher gruselig. SOnst noch:

    - Konstruktoren liefern immer Objekte, nie false
    - Ein leerer Username/Passworteintrag ist IMHO kein Ausnahmefall sondern normales Fehlerverhalten - rechtfertigt keine Exception
    - addslashes(htmlentities hat in der DB nichts zu suchen
    - $stmt->execute(); wird bspw. überhaupt nicht mit Fehlerbehandlung ausgeführt
    - if (isset($_SESSION['id'])) {
    $_SESSION = array();
    nicht gerechtfertigt. IMHO darf die Auth-Komponente nicht über den gesamten Sessioninhalt verfügen. Es gibt auch andere persistente Daten in einer Anwendung.
    - insert_user() - Login und Registrierung sind m.E. zwei verschiedene Themen und gehören nicht in ein gemeinsames Objekt.
    --

    „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
    Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


    --

    Kommentar


    • #3
      Zu der Klasse gibt es dort im Forum doch bereits eine Diskussion: Login-System / Authentifizierung - Forum: phpforum.de . Hast du dort noch nicht geschaut?
      http://hallophp.de

      Kommentar


      • #4
        Fazit: Die Klasse ist großer Müll.
        --

        „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
        Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


        --

        Kommentar


        • #5
          Da werde ich auch mal meine Kritik dazu abgeben. Ohne mir den Code selber komplett anzuschauen gibt es wirklich ne Menge zu kritisieren.

          Du schreibst selber es soll eine Login Klasse sein, also dann musst das ganze auch wirklich nur auf das Login beschränken.

          Das heist da hat keine Datenbankverbindung was drin zu suchen, keine Registrationssachen nicht. Sondern nur das reine was sich auf den Login bezieht. Auch gehören SQL Statements nicht da rein, sondern so ein Statement sollte als Parameter an eine Methode übergeben.

          Dann machst du da drin deine Prüfungen was die Nutzereingaben angeht und weist bei korrekter Prüfung dann z.B. die User ID und den Usernamen (eventuell) Session Variablen zu, die Session Variablen übergibst du aber auch als Prameter.

          Das was du da oben machst, hat nicht mit einem Login Objekt zu tun bzw. es ist einfach nur viel zu viel Mist drin was nicht rein gehört.

          Mfg litter
          Aus dem Dynamo Lande kommen wir. Trinken immer reichlich kühles Bier. Und dann sind wir alle voll, die Stimmung ist so toll. Aus dem Dynamo Lande kommen wir.
          http://www.lit-web.de

          Kommentar


          • #6
            Auch gehören SQL Statements nicht da rein, sondern so ein Statement sollte als Parameter an eine Methode übergeben.
            Das halte ich für Quatsch.
            --

            „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
            Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


            --

            Kommentar


            • #7
              Zuerst einmal danke für die Antworten.

              Die Diskussion habe ich mir angeschaut. Habe versucht diese Dinge zu berücksichtigen als ich sie neu schrieb.

              Wie es aussieht muss ich wohl nochmal von vorne anfangen.

              In meinem Fall kommt nur die Überprüfung der Logindaten und ob die Session gültig ist in die Loginklasse. Alles andere kommt in eine andere Klasse, ist das richtig?

              Gruss
              Hexagone

              Kommentar


              • #8
                Zitat von nikosch Beitrag anzeigen
                Das halte ich für Quatsch.
                Naja das kannste so pauschal nun auch nicht sagen, da teilen sich die Meinungen, die einen sagen das man ne Statementvariable als Parameter übergibt und nicht das SQL Statement so in der Klasse schreibt und andere sagen es wieder anders, ich denke man kann das machen wie man möchte, es sollte nur einheitlich im System geschehen und nicht mal so und mal so.

                Zitat von hexagone Beitrag anzeigen

                In meinem Fall kommt nur die Überprüfung der Logindaten und ob die Session gültig ist in die Loginklasse. Alles andere kommt in eine andere Klasse, ist das richtig?
                Na überlege doch mal was muss den ein Login machen? Es muss Daten die ein Nutzer eingibt entgegen nehmen und vergleichen mit den Daten in der Datenbank, dann musst du dementsprechend handeln mit einer Kontrollstruktur. Ist ein Datenabgleich erfolgreich dann weist du die User ID und eventuell Usernamen Session Variablen zu. In einer Login Klasse kann noch rein der Datenabgleich auf Basis eines Dauerlogins, sprich Cookie Daten.

                Das überprüfen, bzw. Seitenübergreifende überprüfen machst du wieder in einer anderen Klasse, denn Login und Loggedzustand sind 2 unterschiedliche Sachen die nichts in einer Klasse verloren haben.
                Aus dem Dynamo Lande kommen wir. Trinken immer reichlich kühles Bier. Und dann sind wir alle voll, die Stimmung ist so toll. Aus dem Dynamo Lande kommen wir.
                http://www.lit-web.de

                Kommentar


                • #9
                  die einen sagen das man ne Statementvariable als Parameter übergibt
                  Was soll eine Statementvariable sein? Von wo nach wo soll die Deiner Meinung nach übergeben werden? Wer, wenn nicht die Loginklasse kennt denn die DB-=Anfrage, die sie zur Arbeit braucht?
                  --

                  „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
                  Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


                  --

                  Kommentar


                  • #10
                    Zitat von nikosch Beitrag anzeigen
                    - $stmt->execute(); wird bspw. überhaupt nicht mit Fehlerbehandlung ausgeführt
                    Was genau meinst du damit?

                    Kommentar


                    • #11
                      Warscheindlich das die Exception keinen Fehler bei der $stmt->execute() meldet falls einer auftritt. Die Fehlermeldung wird somit nicht abgefangen.

                      Kommentar


                      • #12
                        Das Objekt nutzt an jedem Fitzel eine Exception, aber an einer Stelle, an der ein echter Datenbank(query)fehler auftreten kann, wird nicht mal die Funktionalität geprüft. Ich kann Dir nur nahelegen, ganz unabhängig von diesem schlechten Beispiel etwas komplett neues zu verfassen.
                        --

                        „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
                        Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


                        --

                        Kommentar


                        • #13
                          Jo, werde ich machen. Zuerst werde ich etwas besser planen bevor ich loslege. Wäre Euch dankbar wenn ihr dann kurz über meine uml schaut bevor ich mich nochmal ans programmieren mache

                          Gruss
                          Hexagone

                          Kommentar


                          • #14
                            Zitat von nikosch Beitrag anzeigen
                            Das Objekt nutzt an jedem Fitzel eine Exception, aber an einer Stelle, an der ein echter Datenbank(query)fehler auftreten kann, wird nicht mal die Funktionalität geprüft.
                            Abgesehen von dem Verbindungsaufbau zur Datenbank nutze ich Exceptions kaum. Jedenfalls nicht bei einfachen Prepared Statements, bei denen ich mühelos die Funktionsweise überblicken kann und mir execute() entsprechend TRUE oder FALSE wiedergibt. Oder übersehe ich hier grundsätzlich etwas?

                            Kommentar


                            • #15
                              Oder übersehe ich hier grundsätzlich etwas?
                              Exceptions sind einfach ein anderes Konzept als TRUE/FALSE Rückgaben. Schon dass sie Methoden, selbst Konstruktoren, vorzeitig beenden, bildet einen großen Vorteil. Noch wichtiger ist das Verhalten, dass Exceptions durch Strukturen „durchgereicht“ werden und auch weit oben zentral behandelt werden können. Das kannst Du mit Bool-Returns kaum lösen ohne Dich in einer Orgie von Prüfbediungungen wiederzufinden. Gerade das spiegelt aber auch meinen Gedanken wieder, dass Exceptions für obige Anwendungsfälle wie „Login failed“, die eigentlich einen Normalfall für die Auth-Komponente darstellen, ungeeignet sind.
                              --

                              „Emoticons machen einen Beitrag etwas freundlicher. Deine wirken zwar fachlich richtig sein, aber meist ziemlich uninteressant.
                              Wenn man nur Text sieht, haben viele junge Entwickler keine interesse, diese stumpfen Texte zu lesen.“


                              --

                              Kommentar

                              Lädt...
                              X