User prüfen

[Michael1967]

Hallo,

ich hätte mal wieder eine Frage. Ich habe ein Script was viele User auf einmal nutzen könnten.

Nun können alle User über ein Formular (von dort wird auch die User-ID des Users an eine Funktion übetragen) auch ihre Daten ändern.

Nun könnte aber User ID 1 auch die Daten von User mit ID 2 ändern. Dazu muss er sich ja nur das Formular anschauen und den Link dementsprechend ändern.

meineDatei.php?daten=update&id=2

Wie lässt sich sowas am besten und auch sicher abprüfen?

Die Funktion schaut so aus -

Code: Alles auswählen

function DatenUpdate()
{
// damit hole ich mir die ID des Users
$abfrage = "SELECT * FROM login_daten where `benutzername` = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "' and aktiv=1";
$ergebnis = mysql_query($abfrage);
$row = mysql_fetch_object($ergebnis);

// Hier vergleiche ich die ID (Formular und Abfrage oben
// Wenn beide gleich sind Update ausführen
if($row->id == $_POST['id']) {

$aendern = "UPDATE login_daten Set 
id = '".$_POST["id"]."', 
benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "', 
firma = '". mysql_real_escape_string($_POST["firma"])."', 
url = '". mysql_real_escape_string($_POST["url"])."', 
name = '". mysql_real_escape_string($_POST["name"])."', 
strasse = '". mysql_real_escape_string($_POST["strasse"])."', 
plz = '". mysql_real_escape_string($_POST["plz"])."', 
ort = '". mysql_real_escape_string($_POST["ort"])."', 
email = '". mysql_real_escape_string($_POST["email"])."'
WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "'";

$update = mysql_query($aendern);

} else {

echo"Kein Zugriff";

}
}

Ich frage mich ob das sicher genug ist? Wenn nein - wie müsste man die Abprüfung machen?

[nerd]

ich mache das so das ich beim einloggen die ganzen benutzerdaten (id, name, email usw.) in die session schreibe. damit muss ich die daten dann nicht jedesmal neu aus der db auslesen wenn ich sie irgendwo brauche und kann dan auf solche 'profil aendern' seiten ohne parameter verlinken.

tip: lieber statt $_SESSION[id] die variablen lieber alle unter $_SESSION[user][id] anlegen, sonst wirds schnell unuebersichtlich wenn man noch andere sachen in der session ablegt. beim session init wenn moeglich dafault werte ([user][id] = 0, [user][name] = anonymous) vergeben, sodas man nicht jedesmal pruefen muss ob die variable gesetzt ist. bei [user][id]>0 weisst du dann das der user eingeloggt ist.

[Michael1967]

Hmmm, ich möchte nicht wissen ob ein User eingeloggt ist, sonder das ein User die Daten von einem anderen User nicht updaten kann. Oder hab ich deine Antwort nun falsch verstanden?

Das Formular dürfen alle User benutzen und da stehen auch keine Parameter im Link. Man könnte sich aber einen Link daraus bilden und somit Blödsinn machen.

[nerd]

ja, falsch verstanden
dein problem ist, das du die id im formular uebergibts. die hat da nix zu suchen. du solltest die user_id in der session behalten und das update auch mit " where id=$_SESSION['user']['id'] " ausfuehren.
die abfrage is auch sehr unguenstig, erstens solltest du bei updates immer den primary key als filter angeben (where id = ... ), dann erlaubst du hier den primary key zu aendern (id = '".$_POST["id"]."' - schlechte idee) und dann hast du noch den benutzernamen im update (benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ).) und spaeter nochmal im filter (WHERE ... benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] )).

[Michael1967]

Habe das fast verstanden - aber ich verstehe nicht ganz, wie ich sonst die jeweiligen Datensätze den Usern zuweisen soll.

Das '".$_POST["id"]." in der Abfrage und dem Update ist aber nicht die Session ID vom User, sondern die ID vom Datensatz. Die User ID befindet sich in einer anderen Tabelle.

if($row->id == $_POST['id']) {
$row->id = die ID vom Datensatz aus der Abfrage zum vergleichen
$_POST['id']) Die ID vom Datensatz aus dem Formular

Set id = '".$_POST["id"]." = ID vom Datensatz vom Formular

Das kann ich auch weg lassen, gg

WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "'

[nerd]

ich glaub ich verstehe nicht ganz was du machen willst.
wenn du die benutzerdaten ueber mehrere tabellen verteilst, solltest du trotzdem in jeder tabelle die selbe 'user_id' as key verwenden, und nicht irgendwelche strings wie 'benutzernamen'.

[Michael1967]

Was ich fragte steht doch im ersten Post. Mir geht es nicht darum ob der Benutzername oder die User-Session-ID in die Datenbank geschrieben wird.

Mir geht es darum - das User1 nicht die Daten von User2 verändern kann.

Das mit der Abfrage war eigentlich nur als Beispiel gedacht! Ich muss doch dann vor dem $aendern = "UPDATE prüfen, sonst könnte da doch jeder......

Also habe ich mir gedacht - schickst du die ID des Datensatzes über das Formular mit und vor dem Update überprüfst du dann ob die mitgeschickte ID auch zu diesem Datensatz gehört.

Ist das der richtige Weg?

[Mork vom Ork]

Ich glaube, ihr redet etwas aneinander vorbei, denn das, was nerd beschreibt, machst du augenscheinlich schon.

Richtig hast du bereits gemacht, dass du nach Authentifzierung eine Sitzung startest, dort die geprüften Benutzerdaten sicherst und -hier wird's konkret- die Datensatzänderung mit den Benutzerdaten absicherst, indem du schreibst, WHERE benutzername = $_SESSION['benutzername'] (als Zitat etwas gekürzt, der Übersichtlichkeit halber).
Deine Frage, ob dein Vorgehen sicher ist, ist also insofern mit Ja zu beantworten, als dass Änderungen an fremden Datensätzen nicht möglich sind, weil zur Änderung der gesicherte, da aus $_SESSION gezogene Benutzername herangezogen wird (und nicht die fragliche, über einen unsicheren Kanal übermittelte ID).

Das ist allerdings nur die halbe Miete, denn dein Skript enthält ein paar andere Ungereimtheiten:

Code: Alles auswählen

function DatenUpdate() {
// damit hole ich mir die ID des Users
$abfrage = "SELECT * FROM login_daten where `benutzername` = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "' and aktiv=1";
$ergebnis = mysql_query($abfrage);

An dieser Stelle fehlt eine Fehlerabfrage, mysql_query() könnte durchaus fehlschlagen. Das ist in diesem Einzelfall nicht gar so dramatisch, weil auch alle nachfolgenden Befehle in die Hose gehen würden, aber trotzdem ein Fauxpas.

Code: Alles auswählen

$row = mysql_fetch_object($ergebnis);

// Hier vergleiche ich die ID (Formular und Abfrage oben
// Wenn beide gleich sind Update ausführen
if($row->id == $_POST['id']) {

Auch an dieser Stelle wieder keine Prüfung des Ergebnisses einer mysql_-Funktion, diesmal, ob in $row auch wirklich Daten stehen. Ordentlich wäre if (($row) && ($row->id == $_POST['id'])).

Code: Alles auswählen

$aendern = "UPDATE login_daten Set 
id = '".$_POST["id"]."', 
benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "', 
[...]
WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "'";

Hier wäre zweierlei zu hinterfragen:

1.
Warum setzt du benutzername auf $_SESSION["benutzername"], wenn doch durch die WHERE-Prüfung schon klar ist, dass benutzername = $_SESSION["benutzername"] ist? Das ist natürlich kein Fehler, aber nichtsdestotrotz sinnlos.

2.
Wozu hast du überhaupt diese separate Benutzer-ID, die du in $_POST obendrein noch über einen unsicheren Weg transportierst, wenn du sie doch eigentlich gar nicht benutzt? Die WHERE-Prüfung jedenfalls basiert auf dem Benutzernamen, nicht auf der Benutzer-ID.
Momentan möchte ich behaupten, dass du diese ID ersatzlos streichen kannst, einschließlich des gesamten $row->id/$_POST["id"]-Prüfungscodes. Von deinem Code bleibt damit nur der UPDATE-Aufruf übrig, bei gleicher Sicherheit und Funktionalität.

Deine Vorgehensweise wirft aber weitere Fragen auf: Zum einen, wo du die ID wirklich einmal nutzt (und nicht, wie derzeit, nur mitschleppst), zum zweiten, ob das Datum benutzername die Funktion einer eindeutigen Kennung erfüllt, als die du es ja benutzt.

[Michael1967]

Nochmal zum Vertändnis beide Funktionen.

// Das Formular

Code: Alles auswählen

function Daten()
{
$abfrage = "SELECT * FROM login_daten where `benutzername` = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "' and aktiv =1";
  $ergebnis = mysql_query($abfrage);
  while($row = mysql_fetch_object($ergebnis)) {

echo"<div id=\"formular\"><h3>Daten eintragen</h3><p>Ihre Domaindaten!</p><form id=\"eintragform\" method=\"post\" action=\"?daten=userdaten&id=$row->id\">
<fieldset>
<label for=\"titel\">Firma&#58; </label><input type=\"text\" name=\"firma\" id=\"firma\" value=\"$row->firma\" /><br />

<label for=\"titel\">Url&#58; </label><input type=\"text\" name=\"url\" id=\"url\" value=\"$row->url\" /><br />

<label for=\"titel\">Name&#58; </label><input type=\"text\" name=\"name\" id=\"name\" value=\"$row->name\" /><br />

<label for=\"titel\">Strasse&#58; </label><input type=\"text\" name=\"strasse\" id=\"strasse\" value=\"$row->strasse\" /><br />

<label for=\"titel\">Plz&#58; </label><input type=\"text\" name=\"plz\" id=\"plz\" value=\"$row->plz\" /><br />

<label for=\"titel\">Ort&#58; </label><input type=\"text\" name=\"ort\" id=\"ort\" value=\"$row->ort\" /><br />

<label for=\"titel\">Email&#58; </label><input type=\"text\" name=\"email\" id=\"email\" value=\"$row->email\" /><br />

<input type=\"submit\" name=\"Update\" id=\"Update\" value=\"Update\" />
<fieldset><
/form></div><p class=\"linie\">
</p>";
 &#125;
&#125;

// Die Funktion fürs Datenupdate

Code: Alles auswählen

function DatenUpdate() 
{ 
// damit hole ich mir die ID des Users 
$abfrage = "SELECT * FROM login_daten where `benutzername` = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "' and aktiv=1"; 
$ergebnis = mysql_query($abfrage); 
$row = mysql_fetch_object($ergebnis); 

// Hier vergleiche ich die ID (Formular und Abfrage oben 
// Wenn beide gleich sind Update ausführen 
if($row->id == $_POST['id']) { 

$aendern = "UPDATE login_daten Set 
id = '".$_POST["id"]."', 
benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "', 
firma = '". mysql_real_escape_string($_POST["firma"])."', 
url = '". mysql_real_escape_string($_POST["url"])."', 
name = '". mysql_real_escape_string($_POST["name"])."', 
strasse = '". mysql_real_escape_string($_POST["strasse"])."', 
plz = '". mysql_real_escape_string($_POST["plz"])."', 
ort = '". mysql_real_escape_string($_POST["ort"])."', 
email = '". mysql_real_escape_string($_POST["email"])."' 
WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "'"; 

$update = mysql_query($aendern); 

} else { 

echo"Kein Zugriff"; 

} 
}

1.) Mit $_SESSION["benutzername"] übertrage ich eigentlich nur den Benutzernamen, denn der muss auch in die Datenbank.

2.) $_POST["id"] - na die wird vom Formular mitgeschickt (das ist die ID des Datensatzes. Der Datensatz wird ja fürs Formular ausgelesen und jeder Datensatz hat eine andere ID und aktiv ist immer der mit aktiv = 1 (Wegen einer Wechselfunktion weil jeder User mehr als nur einen Datensatz haben kann. Da kann man hin und her schalten, gg).

Momentan möchte ich behaupten, dass du diese ID ersatzlos streichen kannst, einschließlich des gesamten $row->id/$_POST["id"]-Prüfungscodes. Von deinem Code bleibt damit nur der UPDATE-Aufruf übrig, bei gleicher Sicherheit und Funktionalität.

Hmmm, und wenn sich User 2 nun einloggt, der hat ja ganz andere Datensätze (IDs). Dann könnte der doch die Daten von User 1 ändern - dann greift WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ) doch gar nicht mehr. Würde bei ihm ja auch zutreffen, obwohl seine Datensätze andere IDs haben.

[Mork vom Ork]

1.) Mit $_SESSION["benutzername"] übertrage ich eigentlich nur den Benutzernamen, denn der muss auch in die Datenbank.

„Eigentlich“ ist ja immer eine Einschränkung :) Und in diesem Falle machst du eigentlich was anderes: Du wählst darüber den Datensatz aus. In die Datenbank wird er jedenfalls in den beiden Funktionen nirgends geschrieben, denn er steht ja schon drin, das erwähnte ich oben bereits.

Wo kommt denn $_SESSION["benutzername"] her? Doch sicher über einen Login?

2.) $_POST["id"] - na die wird vom Formular mitgeschickt (das ist die ID des Datensatzes.

Ganz grundsätzlich, unabhängig von den anderen Sachen: Du hast $_SESSION, da kannst du auch diese ID reinpacken, die muss (und darf) nicht ins Formular.

Hmmm, und wenn sich User 2 nun einloggt, der hat ja ganz andere Datensätze (IDs). Dann könnte der doch die Daten von User 1 ändern - dann greift WHERE aktiv = 1 and benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ) doch gar nicht mehr.

Wieso, Benutzer B hat doch nicht den gleichen Benutzernamen wie Benutzer A? Könnte es sein, dass du glaubst, $_SESSION wäre für alle Benutzer gleich? Dem ist nicht so, $_SESSION ist an den jeweiligen Browser gekoppelt - das ist der ganze Witz an der Sache.


Und, nur als Tipp nebenbei, sowas hier:

echo "<div id=\"formular\"><h3>Daten eintragen</h3><p>Ihre Domaindaten!</p><form id=\"eintragform\" method=\"post\" action=\"?daten=userdaten&id=$row->id\">";

&#8230; kannst du etwas leichter schreiben:

echo '<div id="formular"><h3>Daten eintragen</h3><p>Ihre Domaindaten!</p><form id="eintragform" method="post" action="?daten=userdaten&id=' . $row->id . '">';

Vielleicht hast du ja den Überblick, aber mich irritieren diese ganzen \" ganz fürchterlich ;)


Weiterhin hast du noch ein HTML- bzw. sogar Sicherheitsproblem:

In <form &#8230; action="?daten=userdaten&id=123"> muss das & als & geschrieben werden. Das & hat im gesamten (!) HTML-Code, einschließlich aller URLs, eine Sonderrolle und muss maskiert werden.

In der URL fällt das noch nicht auf, weil alle Browser diesen Fehler selbst beheben, aber beim nächsten Punkt wird's kritisch:

label for=\"titel\">Firma: </label><input type=\"text\" name=\"firma\" id=\"firma\" value=\"$row->firma\" /><br />

Du gibst die Daten ungefiltert in deinen HTML-Code aus. Ein Firmenname wie Meier & Co. dürfte wegen der besagten Sonderrolle des Und-Zeichens so nicht im Browser erscheinen. Korrekt ist nur Meier & Co., dies erreichst du, indem du alle Daten vor der Ausgabe durch htmlspecialchars() filterst (quasi das Gleiche, was du schon bei MySQL mit mysql_real_escape_string() machst, nur nicht bei mysql_query(), sondern bei echo):

echo 'label for="titel">Firma: </label><input type="text" name="firma" id="firma" value="' . htmlspecialchars($row->firma) . '" /><br />';

Der Sicherheitsaspekt ist in diesem Fall wegen des Benutzerumfelds vermutlich nicht so dramatisch, nichtsdestotrotz erlaubst du den Nutzern momentan sogar, beliebigen Code, einschließlich Javascript, in deine Seiten einzuschleusen - einfach, indem der gewünschte Code in ein Datenbankfeld eingegeben wird. Eine feine Methode, Viren zu verbreiten.

Deshalb von Benutzern kommende Daten immer mit htmlspecialchars() behandeln! Einzige Ausnahme: Du willst ausdrücklich HTML & Co. zulassen.

[Michael1967]

Erstmal danke für deine Hilfe - da habe ich wieder eine Menge dazu gelernt

Wegen der Sicherheit benutze ich sowas hier -

Code: Alles auswählen

foreach ($_POST AS $key => $post) {

	$_POST[$key] = nl2br(htmlspecialchars($post));
}


foreach ($_GET as $check_url) {

	if &#40;&#40;eregi&#40;"<&#91;^>&#93;*script*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*object*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*iframe*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*applet*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*meta*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*style*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*form*\"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"\&#40;&#91;^>&#93;*\"?&#91;^&#41;&#93;*\&#41;", $check_url&#41;&#41; || &#40;eregi&#40;"\"", $check_url&#41;&#41;&#41; &#123;
	
		die &#40;"Fehler&#58; Unerlaubter Zugriff!"&#41;;
		
	&#125;

&#125;

unset&#40;$check_url&#41;;

Sollte doch eigentlich das selbe machen, wie htmlspecialchars($row->firma). Den Rest deiner Tipps werde ich mal umzusetzen.

Nochmal danke!

[Michael1967]

Nun habe ich das mal so geändert.

Code: Alles auswählen

function DatenUpdate()
{

$aendern = "UPDATE login_daten Set 
firma = '". mysql_real_escape_string($_POST["firma"])."',
url = '". mysql_real_escape_string($_POST["url"])."',
name = '". mysql_real_escape_string($_POST["name"])."',
strasse = '". mysql_real_escape_string($_POST["strasse"])."', 
plz = '". mysql_real_escape_string($_POST["plz"])."', 
ort = '". mysql_real_escape_string($_POST["ort"])."', 
email = '". mysql_real_escape_string($_POST["email"])."'
WHERE benutzername = '" .mysql_real_escape_string( $_SESSION['benutzername'] ). "' and aktiv = 1"; 
$update = mysql_query($aendern);
}

Der Datensatz mit der ID 5 gehört mir.

Nun habe ich mich eingeloggt und per URL einfach mal daten=userdaten&id=4 aufgerufen. Es wird wohl nun Datensatz 4 nicht verändert, aber in der Datenbank werden bei ID 5 alle Felder geleert.
Was muss ich denn machen, damit die Inhalte in der Datenbank nicht gelöscht werden?

Voher prüfen ob die Felder im Formular ausgefüllt wurden?

[Mork vom Ork]

Wegen der Sicherheit benutze ich sowas hier -

Code: Alles auswählen

foreach ($_POST AS $key => $post) {

	$_POST[$key] = nl2br(htmlspecialchars($post));
}

Achso, da hast du htmlspecialchars() ja bereits drin, dann geht's ja. Mir persönlich ist es allerdings lieber, wenn ich die Daten unkodiert speichere und die Kodierung erst da hinzufüge, wo sie gebraucht wird, in diesem Falle also bei der Ausgabe. Ansonsten würde ich möglicherweise die Übersicht verlieren - aber das ist sicher Geschmackssache.

Code: Alles auswählen

foreach ($_GET as $check_url) {
	if &#40;&#40;eregi&#40;"<&#91;^>&#93;*script*"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*object*"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*iframe*"?&#91;^>&#93;*>", $check_url&#41;&#41; || &#40;eregi&#40;"<&#91;^>&#93;*applet*"?&#91;^>&#93;*>", $check_url&#41;&#41; ...	
		die &#40;"Fehler&#58; Unerlaubter Zugriff!"&#41;;
	&#125;
&#125;

Das ist überaus unklug, denn es hindert pöse Puben&#8482; nicht daran, etwas à la <div onmouseover='location.url="ganz.böse/mit/virus.bmp"'> einzubauen.
Ich möchte diese Vorgehensweise glatt als typisches Beispiel dafür anführen, dass Sicherheitsfunktionen, die Einzelfälle im Dutzendpack sperren, keine allgemeine Sicherheit bieten, weil doch immer die Hälfte vergessen wird. Irgendwer ist immer schlauer.

Benutze durchgängig htmlspecialchars(), das sperrt in Sachen HTML alles. Willst du irgendwo einzelne HTML-Elemente erlauben, erlaube diese einzeln - aber niemals andersrum, einzelne Elemente sperren und alles andere, nicht genannte erlauben.

[Michael1967]

Jo, okay vielen Dank hast mir sehr geholfen!

[Mork vom Ork]

Nun habe ich mich eingeloggt und per URL einfach mal daten=userdaten&id=4 aufgerufen. Es wird wohl nun Datensatz 4 nicht verändert, aber in der Datenbank werden bei ID 5 alle Felder geleert.
Was muss ich denn machen, damit die Inhalte in der Datenbank nicht gelöscht werden?

Voher prüfen ob die Felder im Formular ausgefüllt wurden?

Eine Prüfung, ob das Formular akzeptable Daten enthält, wäre natürlich irgendwo durchaus sinnig :)

Aber da du schreibst, du hättest die URL &#8222;aufgerufen&#8220; ( = in die Adressleiste eingetippt?), vermute ich mal, dass das Problem noch einen Schritt weiter vorne liegt: Du prüfst nicht einmal, ob die URL überhaupt als Formular eingesandt wurde. Hast du, bevor du DatenUpdate() aufrufst, eine Abfrage in der Art if (isset($_POST["name_des_submit-Elements_deines_Formulars"])) einbaut?