✔ Problem bei Klasse, ist das Script sicher?

[hab1cht]

Loginscript Problem + Sicherhheitsfrage

Hallo,
hab mein Login-Script gerade um eine Funktion erweitert und zwar sollen Falschanmeldungen in einer Logdatei gespeichert werden.
Das Speichern von Falschanmeldungen klappt auch wunderbar, allerdings die Anmeldung mit richtigen Daten nicht mehr, es kommt folgender Fehler: "Fatal error: Cannot redeclare class mysql_control".
Hier der Code der login.php:

<?php 

define('thefooter', TRUE);
include("inc/config.inc.php");

require('MySQLc.inc.php');       #Klasse mit Speicherfunktion für Falscheingaben
$mc = new MySQL_Control;

$ip = $REMOTE_ADDR;


if(isset($_POST[login])) { 

    if(empty($_POST[passwort]) or empty($_POST[vorname]) or empty($_POST[nachname])) { 
       $error = true;
	     echo "Bitte füllen Sie alle Lücken aus!";
	     $result = $mc->mc_registerfail($vorname, $nachname, $passwort, $ip);
    } else { 
        $passwort = strip_tags(stripslashes($_POST[passwort]));
        $passwort = MD5($passwort);
        $nachname = strip_tags(stripslashes($_POST[nachname]));
        $vorname = strip_tags(stripslashes($_POST[vorname])); 
        $sql = "SELECT * FROM `user` WHERE `nachname` = '".$nachname."' && `vorname` = '".$vorname."' && `passwort` = '".$passwort."'";
	     $check = mysql_query($sql);
	     $vorhanden = mysql_num_rows($check);
	if($vorhanden != "1") { 
            $error = true;
	    echo "Dieser Benutzer ist in der Datenbank nicht registriert!";
	    $result = $mc->mc_registerfail($vorname, $nachname, $passwort, $ip); 	 
        }     
    } 
     
    if(!$error) { 
        if($check) { 
            $check = mysql_fetch_array($check); 
                $userid = $check["ID"]; 
	$time= date("Y-m-d H:i:s");
	$query = "UPDATE `$my_table` SET `letzter_login` = '".$time."' WHERE `ID` = $userid LIMIT 1"; 
		$result = $mc->mc_mysql_query($query);
	session_start(); 
                session_register("userid"); 
                header("Location: index_profil.html"); 
            } else { 
                   echo "Es ist ein Fehler aufgetreten, bitte versuchen Sie es später noch einmal. Sollte dieser Fehler weiterhin auftreten, wenden Sie sich bitte an das Serviceteam oder fordern Sie ein neues Passwort an."; 
	       	$result = $mc->mc_registerfail($vorname, $nachname, $passwort, $ip);
            } 
        } 
    } 

?>

Der Code der Klasse:

<?php 
    # Filename: MySQLc.inc.php 
    #  
    # Funktion: Falsche Logins werden gespeichert
     
    define ('CRLF', chr(13).chr(10)); # Zeilenumbruch 
    define ('PATH', $_SERVER['DOCUMENT_ROOT'].'/MySQL_Control/'); # Pfad zur Logdatei 
         
    class MySQL_Control{ 
         #Funktion für, falsche Logins
	function mc_registerfail($vorname, $nachname, $passwort, $ip){
            $login_date = date("d.m.y, H:i:s"); # Aktuelles Datum und aktuelle Zeit setzen 

            # Überprüfen ob das Verzeichnis existiert in dem die Logdatei abgelegt werden soll.. Ansonsten Wird das Verzeichnis erstellt.. 
            if(!is_dir($_SERVER['DOCUMENT_ROOT'].'/MySQL_Control')){ 
                mkdir ($_SERVER['DOCUMENT_ROOT'].'/MySQL_Control', 0664); 
            } 
	    
	    $this->sl_writeFile(PATH.'login_report.log', 'Vorname: '.$vorname.' ; Nachname: '.$nachname.' ; Passwort: '.$passwort.' ; IP: '.$ip."\t\t\t# ".$login_date.CRLF); 
	   
	}
         
        # Funktion zum Schreiben in eine Datei 
        function sl_writeFile($file, $data, $mode = 'a'){ 
            $fp = fopen($file, $mode); 
            fwrite($fp, $data); 
            fclose($fp); 
        }

		
    } # End Class MySQL_Control 
?>

Wo ist hier der Fehler? Warum kann ich mich nicht mehr in meinen Useraccount einloggen?

Zusatz: Ist dieses Script sicher? Was sollte ich verbessern?

Vielen Dank im Voraus.

MfG
hab1cht

 

[nkler]

$result = $mc->mc_mysql_query($query);

Wo ist die Funktion mc_mysql_query bestimmt?

require('MySQLc.inc.php');
$mc = new MySQL_Control;

Nebenbei ist das beabsichtigt 2x?

 

[hab1cht]

Das habe ich ausversehen 2mal geschrieben, habe das wieder entfernt.

Der Code für die Funktion "mc_mysql_query " lautet (ist auch in der 'MySQLc.inc.php'):

        # Diese Funktion benutzen anstatt "mysql_query()" 
        function mc_mysql_query($sql){ 
            $sql_date = date("d.m.y, H:i:s"); # Aktuelles Datum und aktuelle Zeit setzen 
             
            # Überprüfen ob das Verzeichnis existiert in dem die Logdatei abgelegt werden soll.. Ansonsten Wird das Verzeichnis erstellt.. 
            if(!is_dir($_SERVER['DOCUMENT_ROOT'].'/MySQL_Control')){ 
                mkdir ($_SERVER['DOCUMENT_ROOT'].'/MySQL_Control', 0664); 
            } 
             
            # Nur SQL Befehle loggen, die einen Datensatz verändern.. Also keine SELECT Befehle mitloggen 
            if(preg_match('/(^insert)|(^update)|(^alter)|(^create)|(^delete)/i', $sql, $type)){ 
                # SQL Befehl in die Logdatei schreiben.. Datum, Zeit und Zeilenumbruch werden angehängt.. 
                $this->sl_writeFile(PATH.'sql_report.sql', $sql.';'."\t\t\t# ".$sql_date.CRLF); 
            } 
            return mysql_query($sql); # "mysql_query()" wird ausgeführt 
        }

 

[Gumbo]

Ist es möglich, dass du die „MySQLc.inc.php“-Skriptdatei mehrmals inkludierst? Denn anders kann ich mir die Fehlermeldung nicht erklären. Am besten arbeitest du bei einmalig einzubindenden Skriptdateien mit der include_once- oder noch besser mit der require_once-Anweisung.

 

[hab1cht]

Jop stimmt, hatte das 2mal drinstehen, habe es dann aber nur hier in meinem Post korrigiert und nicht in meiner login.php.
Zur Sicherheit: kann man das so stehen lassen, oder sollte ich irgendetwas verbessern an dem Code, um Sicherheitslücken zu vermeiden.

MfG
hab1cht

 

[Gumbo]

Ja es gibt Sicherheitsmängel. Informiere dich mal über SQL-Injektionen. Zusätzlich solltest du alle Zeichen auch explizizt als solche auszeichnen. Weitere Anregungen zur Sicherheit in PHP findest du auch hier im Forum, du musst nur suchen.

 

[redX]

require_once / include_once kannst du so schon stehen lassen.
Wenn eine Datei mehrmals inkludiert wird, heisst das eventuell einfach das bei deinem Script irgendwo ein Fehelr unterlaufen ist. Denn mehrmals einbinden zeigt oft, das im Ablauf irgend wo etwas schiefläuft und unerwartet geschiet. Muss natürlich aber nicht sein. Habe auch scripts wo ich die gleichen Dateien mehrmals include, da es einfach nicht anderst geht. Aber sollte man vermeiden, wens geht

    if($vorhanden != "1") {  
            $error = true; 
        echo "Dieser Benutzer ist in der Datenbank nicht registriert!"; 
        $result = $mc->mc_registerfail($vorname, $nachname, $passwort, $ip);       
        }      
    }

Ich würde beim Login dem User nicht solche Fehlermeldungen ausgeben. Denn wenn du detailierte Fehler ausgibt, weiss er, wann er einen Benutzername gefunden hat, welcher existiert. Und dann kann er versuchen sich mit dem Namen einzuloggen. Ich würde einfach nur ausgeben: Falsche Benutzerangaben. Oder so was in der Art.

Pass noch auf SQL Injections auf: SQL-Injektion - Wikipedia
Da die Daten, welche im Query verwendet werden vom User kommen würde ich sie mit PHP: mysql_real_escape_string - Manual escapen.

Und wieso machst du stripslashes auf die $_GET[] Informationen? Das brauchst du erst wenn du Strings anzeigen möchtest, welche du escaped hast.

Sonst fällt mir gerade nix ein.