[PHP] (sicheres Login) programmieren

[speedy00]

Ich überprüfe ob ein User eingeloggt ist so

<?php 
session_start (); 
if (!isset ($_SESSION["user_id"])) 
{ 
  header......

Nun lese ich gerade hier

Wenn der User den richtigen Usernamen und Passwort eingibt wird "Your secret data" ausgegeben. Ein Angreifer kann dieses Login bei aktiviertem register_globals ganz einfach umgehen, wenn er ?logged_in=true an den URL anhängt. Damit macht er aus diesem Code:

// if the user is logged in...
if($logged_in == true) {
    // print secret data
    echo "Your secret data";
}

praktisch diesen:

$logged_in = true;
// if the user is logged in...
if($logged_in == true) {
    // print secret data
    echo "Your secret data";
}

Da ich ja prüfe ob was in der session ist dürfte die gefahr bei mir ja nicht bestehen, oder?
Oder gibts dafür was vergleichbares?
Bei mir sind register_globals auf on.

 

[theHacker]

Da ich ja prüfe ob was in der session ist dürfte die gefahr bei mir ja nicht bestehen, oder?
Oder gibts dafür was vergleichbares?
Bei mir sind register_globals auf on.

Du benutzt schon den richtigen Code. Selbst wenn register_globals = on, ist das hier sicher:

if(!isset($_SESSION['user_id'])) { //...

Die Methode, die man nicht mehr benutzen darf/sollte, ist diese hier:

session_register("user_id");
if(!isset($user_id)) { //...

(siehe auch https://de.php.net/session_register)

Wichtig ist v.a. der Umgang mit GET-/POST-Variablen:
Korrekt:

if(isset($_POST['pw'])) { //...

Sicherheitslücke:

if(isset($pw)) { //...

Ich würde dir ferner den Tip geben, register_globals zu deaktivieren bzw. deinen Hoster bitten, das durchzuführen. Die Gefahr, dass du beim Programmieren eine Sicherheitslücke (s.o.) einbaust, ist damit schon mal weg.

 

[Graccem]

Ich würde dir ferner den Tip geben, register_globals zu deaktivieren bzw. deinen Hoster bitten, das durchzuführen. Die Gefahr, dass du beim Programmieren eine Sicherheitslücke (s.o.) einbaust, ist damit schon mal weg.

Dann gleich mal eine Ergänzung hierzu. Die register_globals kann man mittels htaccess deaktivieren, wenn dies der Hoster erlaubt. Der Befehl heißt hierfür:

php_flag register_globals off

Dann sollte ein phpinfo() in den lokalen Einstellungen ein off aufweisen.

 

[speedy00]

Danke, session_register habe ich rausgeschmissen, das hatte ich vor etwas längerer Zeit in nem Script drin.

Ich hab das so wie du vorschlägst mit den GET/POST Variablen
anmelden

<? 
}elseif(!isset($_POST['password']) || $_POST['password'] == "") {
  echo '<p align="center">Kein Passwort angegeben<br><br><a href="javascript:window.back()">Zurück</a></p>';

}elseif($_POST['password'] != $_POST['password2']) {
  echo '<p align="center">Die Passwörter stimmen nicht überein!<br><br><a href="javascript:window.back()">Zurück</a>';
  }else{

  $query = @mysql_query("SELECT nickname FROM member WHERE nickname = '".$_POST['nickname']."'");
  $result = @mysql_fetch_array($query);
  if($_POST['nickname'] == $result['nickname']) {
  echo '<p align="center">Sorry, dieser Benutzername ist leider schon vergeben!<br><br><a href="javascript:window.back()">Zurück</a></p>';
  die;
  }else{
  $nickname = $_POST['nickname'];
  $passwort = md5($_POST['password']);
  $email = $_POST['email'];
  if($insert = @mysql_query("INSERT INTO member SET nickname = '$nickname', passwort = '$passwort' , email = '$email'")) {
  echo '<p align="center">Der neue Benutzer wurde erfolgreich angelegt!<br><br></p>';
 }else{
 echo '<p align="center">Beim Anlegen des neuen Benutzers trat leider ein Fehler auf!<br><br><a href="javascript:window.back()">Zurück</a></p>';
 echo mysql_error();
   }
   }
   }
?>

Die login die den login prüft

<?php 
// Session starten
session_start ();

// Datenbankverbindung aufbauen 
$connectionid = mysql_connect ("localhost", "xxxxxxx", "xxxxxxxx"); 
if (!mysql_select_db ("xxxxxxx", $connectionid)) 
{ 
  die ("Keine Verbindung zur Datenbank"); 
} 

$sql = "SELECT ". 
    "id, nickname, email, vorname, nachname, passwort ". 
  "FROM ". 
    "member ". 
  "WHERE ". 
    "(nickname like '".$_REQUEST["name"]."') AND ". 
    "(passwort = '".md5 ($_REQUEST["pwd"])."')"; 
$result = mysql_query ($sql); 
echo mysql_error();
if (mysql_num_rows ($result) > 0) 
{ 
  // Benutzerdaten in ein Array auslesen. 
  $data = mysql_fetch_array ($result); 

  // Sessionvariablen erstellen und registrieren 
  $_SESSION["user_id"] = $data["id"]; 
  $_SESSION["user_nickname"] = $data["nickname"]; 
  $_SESSION["user_vorname"] = $data["vorname"];
  $_SESSION["user_nachname"] = $data["nachname"]; 
  $_SESSION["user_email"] = $data["email"]; 

  header ("Location: ../buja.php"); 
} 
else 
{ 
  header ("Location: member/einloggen.php"); 
} 
?>

Müsste so alles stimmen.
Muss ich eigentlich die sesion variablen alle einzeln registrieren oder kann ich das auch einfacher?

 

[theHacker]

Müsste so alles stimmen.

register_globals-technisch ja. SQL-Injections...
Grundsatz: Jede Eingabe vom User ist gefährlich.
Du wirfst aber munter lustig die $_POST-Variablen direkt in deine Querys:

$query = @mysql_query("SELECT nickname FROM member WHERE nickname = '".$_POST['nickname']."'");

Muss ich eigentlich die sesion variablen alle einzeln registrieren oder kann ich das auch einfacher?

Wenns viel is, nehm ich der Übersicht halber ne foreach()-Schleife. Jedem, wies besser gefällt.

$arr=array('nickname' => 'nickname',
           'userid' => 'id',
           'foo' => 'bar', // ...
          );
foreach($arr as $key => $value)
  $_SESSION[$key]=$_POST[$value];

Die Nickname-already-exist-Abfrage is n bissle komisch. Nimm lieber mysql_num_rows(), weil es nicht klar is, ob du überhaupt ein Ergebnis zurückkriegst.

 

[speedy00]

Jupp, das mit SQL injections währ mein nächster Punkt gewesen.
Immo ist das Ding funktionsfähig aber noch nicht Einsatzbereit.
mit der foreach werde ich machen da ne Nickpage geplant ist und da schon einiges kommen wird an variablen.
Das mit dem Nickname already hab ich so verstanden das geschaut wird ob es ihn gibt, wenn ja, dann Fehlermeldung wenn nicht alles ok.

 

[theHacker]

Das mit dem Nickname already hab ich so verstanden das geschaut wird ob es ihn gibt, wenn ja, dann Fehlermeldung wenn nicht alles ok.

Hier gehts eigentlich nur um saubere Programmierung.
Wir nehmen an, es gäbe keinen User mit dem übergebenen Namen.

Dein Code:

$query = @mysql_query("SELECT nickname FROM member WHERE nickname = '".$_POST['nickname']."'"); 
$result = @mysql_fetch_array($query); 
if($_POST['nickname'] == $result['nickname']) {

• mysql_query() verläuft erfolgreich und $query enthält das Ergebnis-Handle
• mysql_fetch_array() verläuft erfolgreich, setzt aber $result auf false, da es keinen Ergebnisdatensatz gibt.
• $result['nickname'] wirft eine E_NOTICE, da $result kein Array ist und somit $result['nickname'] auch nicht gesetzt is.

Mein Vorschlag:

$res = mysql_query("SELECT `nickname` FROM `member` WHERE `nickname` = '".mysql_real_escape_string($_POST['nickname'])."'"); 
if(mysql_num_rows($res)) {

• mysql_query() verläuft erfolgreich und $res enthält das Ergebnis-Handle. SQL-Injection wurde vermieden.
• mysql_num_rows() liefert 0, damit ist die Bedingung "n(Datensätze) > 0" nicht erfüllt. Es tritt keine Warnung auf und ich hab sogar noch eine Zeile Code gespart.

 

[speedy00]

Achso, jo nun verstanden, danke Hab das geändert.