PHP Code Überprüfung - Secure Code

King Domi

Cadet 4th Year
Registriert
Sep. 2013
Beiträge
125
Hallo,

ich habe folgendes Anliegen, nämlich dass jemand über meinen Code schaut, ob ich Fehler zur Sicherheit gemacht habe.

Es geht hierbei um die Erweiterung einer bestehenden Website im privaten Bereich. Technisch gesehen schreibe ich beim Login die Session ID, den Benutzernamen, Timestamp, Active (0/1) in eine Datenbanktabelle (mySQL). Bein Aufruf der Erweiterung wird geprüft, ob der Session Cookie gesetzt ist (wird ausgelesen und mit den in der DB Gespeicherten verglichen), ob Session ID in der Tabelle vorhanden ist und active = 1 ist. Ist das der Fall folgt User abhängige Ausgabe. Weiterhin läuft ein Cronjob der alle 15 min prüft ob die Sessions älter als 60min sind und setzt dann den Status active auf 0. Ich habe vor den Code noch so zu erweitern, dass der DB Eintrag bei jedem Seitenaufruf erneuert wird (Timestamp), damit der Benutzer nicht einfach so ausgeloggt wird. Der Cronjob dient hierzu nur wenn der Browser geschlossen wurde und die Session über den Logout nicht abgemeldet wurde. Um 01:00 läuft ein Job der alle Einträge mit active = 0 in eine Archiv-Tabelle verschiebt. Die config.php wird nachher außerhalb des Webroots aufbewahrt.

Hier schreibe ich die Sessiondetails in die Datenbank: (Code ist nach dem Login und vor dem Redirect)
PHP:
if($this->userIsLoggedIn)
{
        // DB Logindaten holen und in db_link packen
       require_once("config.php");
       $db_link = mysqli_connect(MYSQL_HOST, MYSQL_BENUTZER, MYSQL_KENNWORT, MYSQL_DATENBANK);
			
        // Variablen 
	date_default_timezone_set("Europe/Berlin");
	$session_id = $this->frontendController->fe_user->id;
	$uid = $this->frontendController->fe_user->user["uid"];
	$username = $this->frontendController->fe_user->user["username"];
	$in_timestamp_unix = time();
	$in_timestamp_date = date("d/m/Y H:i:s");
	$out_timestamp_unix = "";
	$out_timestamp_date = "";
	$active = 1;

        // SQL Statement prepared, da das Session Cookie in jedem Fall gelesen wird (sofern vorhanden), unabhängig vom 
        Inhalt: SQL Injection	
	$stmt = $db_link->prepare('INSERT INTO fe_users_currloggedin(session_id, uid, username, in_timestamp_unix, 
        in_timestamp_date, out_timestamp_unix, out_timestamp_date, active) VALUES(?,?,?,?,?,?,?,?)');
        $stmt->bind_param('sisssssi', $session_id, $uid, $username, $in_timestamp_unix, $in_timestamp_date, 
        $out_timestamp_unix, $out_timestamp_date, $active);
	$stmt->execute();
	$stmt->close();
			
}else
{
        // DB Logindaten holen und in db_link packen
	require_once("config.php");
	$db_link = mysqli_connect(MYSQL_HOST, MYSQL_BENUTZER, MYSQL_KENNWORT, MYSQL_DATENBANK);
  
        // Variablen 
	$session_id = $this->frontendController->fe_user->id;
	$active = 0;
	$out_timestamp_unix = time();
	$out_timestamp_date = date("d/m/Y H:i:s");
	
        // SQL Statement prepared, da das Session Cookie in jedem Fall gelesen wird (sofern vorhanden), unabhängig vom 
        Inhalt: SQL Injection		
	$stmt = $db_link->prepare('UPDATE fe_users_currloggedin SET active=?, out_timestamp_unix=?, 
        out_timestamp_date=? WHERE session_id=?');
	$stmt->bind_param("isss", $active, $out_timestamp_unix, $out_timestamp_date, $session_id);
	$stmt->execute();
	$stmt->close();
}

Das ist der Code der Erweiterung:
PHP:
if(isset($_COOKIE["fe_typo_user"])) // prüfen, ob Cookie gesetzt
{
	$session_id = $_COOKIE["fe_typo_user"]; // Cookie-Value auslesen

        // DB Logindaten holen und in db_link packen
	require_once("config.php");
	$db_link = mysqli_connect(MYSQL_HOST, MYSQL_BENUTZER, MYSQL_KENNWORT, MYSQL_DATENBANK);
      
        // SELECT Abfrage, um Session ID prüfen zu können
	$sql = "SELECT session_id,username,active FROM fe_users_currloggedin";

	$db_erg = mysqli_query($db_link, $sql);
	while ($zeile = mysqli_fetch_array($db_erg))
	{
                // wenn Session ID existiert und active = 1, dann Usernamen aus DB lesen
		if(strcasecmp($zeile["session_id"], $session_id) == 0 && $zeile["active"] == 1)
		{
			$username = $zeile["username"];
			break;
		}
	}
	if(!empty($username)) // prüfen, ob Username-Variable existent (wenn nicht ist DB Abfrage schiefgegangen)
	{
		echo "Successfully logged in.<br>";
		echo "Welcome ".$username;

                // MORE CODE HERE
	}
}
else
{
	echo "You need to login";
}

Die einzige Schwachstelle, die ich sehe, ist das Session Cookie im Falle von Hijacking.
 
Zuletzt bearbeitet:
Also Erweiterung Z. 10 - 21 gehört in SQL. Sonst spam ich dir dein currloggedin zu und mache ne schöne DoS. (mach ein select * from loggedin where session_id=$id and active=1).

​Warum speicherst du deine in und out Zeiten doppelt ab? Username gehört in eine andere Tabelle (Normalform). "Active" würde ich nicht speichern, sondern im Fall =0 die Zeile löschen. Dann sparst du dir auch den Cron-Job.
​Strcasecmp ist bei Sicherheitssachen oft gefährlich. Besser === verwenden. Aber der Code gehört sowieso weg...
 
hi,

lass bitte die DB direkt prüfen, ob die sessionId noch aktiv ist, anstatt alle eingeloggten User zu iterieren und dort dann die SessionId und das aktiv-Flag zu prüfen. Damit das noch schneller geht kann man auch noch Indizes drauf legen (aktiv + sessionId in der Reihenfolge).
Muss man aber wieder auf SQL-Injektion prüfen, evtl vorher die SessionId auf Sinnhaftigkeit prüfen.
Zumindest das prüfen des aktiv-Flags sollte man direkt in die Query packen.

Hijacking kann man noch verhindern in dem man die IP noch in der Session speichert, dann hat man allerdings evtl Probleme bei Mobilnutzern, da manche ISPs NAT verwenden oder evtl. Proxies bei denen sich die User-IP ändern kann.

Ansonsten natürlich die typische OWASP Top Ten Liste durchgehen.
Kleines Beispiel:
http://www.securityweek.com/owasp-proposes-new-vulnerabilities-2017-top-10

Und folgende Stichwörter noch für den Code:
Early Bailout / Guard Statements (Der Welcome Code steckt unnötig tief in einer Code-Ebene)
Clean Code (Kapitel Funktionen)

hth
 
Wenn ich mir das prep'd statement so anschau, könntest Du - solltest Du -- Dir die übergebenen Typen nochmal anschauen. Da sind ein paar Zahlwerte dabei, die als String übergeben wurden.

Re: secure code fehlen mir persönlich die Variablentypen. Sind das Strings oder Integer oder doch Doubles oder was? Ist es nicht festgelegt, kann das alles mögliche sein. Merken wird man dann es nicht, wenn ups-aus-Versehen was nicht-ganz-so-richtiges übergeben wurde.

An einer Stelle holst Du was aus _COOKIE[], prüfst das aber nicht.

Außerdem, aber das hat mit Sicherheit weniger zu tun:

- Du kannst einmal die DB aufmachen und einmal zumachen. Weniger Streß für alle beteiligten.
- Kein Grund für den Cronjob. Alter prüfen kann das PHP-Script zur Laufzeit selber.
- Gibt es einen Grund für das active-Flag? Insbesondere: spricht was gegen das Löschen abgelaufener Sessions? Irgendwann solltest DU das auf jeden Fall irgendwie machen, sonst hast Du mehr oder weniger bald 1 Million Records in der Sessiontabelle, mit weit über 99% active=0, was die Suche ewiglich verlangsamt.

Re: Sessioncookie selber, naja Du könntest zusätzliche Information in die Sessiontabelle schreiben, zB die IP-Adresse des Clients. Dann prüfst Du, ob Anfrage-IP und Session-IP zusammenpassen. Tun sie es nicht: sesssion_destroy() und weg damit aus der Sessiontabelle und der Benutzer muß sich neu anmelden.
 
Hier nur ein mal ein Beispiel wie man den Code (basierend auf den Stichwoertern von nyromant) etwas schlanker gestalten koennte.
https://de.wikipedia.org/wiki/Don’t_repeat_yourself

- Die Datenbankverbindung braucht man nur einmal pro Datei (am besten den Teil ebenfalls in eine db.php oder so auslagern)
- Doppelte und nicht ganz so nuetzliche Variablen entfernt (z.B. $active brauchst man eigentlich nicht wenns im SQL Statement direkt steht)
- Da das aktuelle Datum und Uhrzeit in die Datenbank geschrieben wird reichen doch auch SQL NOW() und DATE() oder nicht?
- Session loeschen wie von den anderen bereits beschrieben.

VORSICHT dieser Code ist nicht getestet und ohne Kenntnis des restlichen Codes nur als grobes Beispiel anzusehen!

PHP:
// DB Logindaten holen und in db_link packen
require_once('config.php');
$db_link = mysqli_connect(MYSQL_HOST, MYSQL_BENUTZER, MYSQL_KENNWORT, MYSQL_DATENBANK);
date_default_timezone_set('Europe/Berlin');
$session_id = $this->frontendController->fe_user->id;

if($this->userIsLoggedIn)
{
    $uid = $this->frontendController->fe_user->user['uid'];
    $username = $this->frontendController->fe_user->user['username'];
    $stmt = $db_link->prepare('INSERT INTO fe_users_currloggedin(session_id, uid, username, in_timestamp_unix,
        in_timestamp_date, out_timestamp_unix, out_timestamp_date, active) VALUES(?,?,?,NOW(),DATE(),"","",1)');
    $stmt->bind_param('sis', $session_id, $uid, $username);
} else {
    $stmt = $db_link->prepare('DELETE fe_users_currloggedin WHERE active=1 AND session_id=?');
    $stmt->bind_param('s', $session_id);
}

$stmt->execute();
$stmt->close();
Code:
// DB Logindaten holen und in db_link packen
require_once('config.php');
$db_link = mysqli_connect(MYSQL_HOST, MYSQL_BENUTZER, MYSQL_KENNWORT, MYSQL_DATENBANK);
if(!isset($_COOKIE['fe_typo_user'])) // prüfen, ob Cookie gesetzt
{
    // direkt aussteigen, spart riesige if Verschachtelungen
    // return/exit oder was an dieser Stelle am besten passt (keine Ahnung wie es eingebunden wird)
    return 'You need to login';
}

$session_id = $_COOKIE['fe_typo_user']; // Cookie-Value auslesen
$stmt = $db_link->prepare('SELECT session_id,username,active FROM fe_users_currloggedin WHERE active=1 AND session_id=?');
$stmt->bind_param('s', $session_id);
$stmt->execute();
$result = $stmt->get_result();
$row = $result->fetch_assoc(); // nur das erste ergebnis
$stmt->close();
if(!empty($result))
{
    echo 'Successfully logged in.<br>';
    echo 'Welcome '.$row['username'];
    // MORE CODE HERE
}
 
Meine Anmerkungen als gut gemeinter Rat:

Generell: Schlecht zu lesender Code lässt sich auch nur schlecht warten und ist generell anfällig für Fehler.

Speziell hier: Ein wilder Mix aus Objektorientierten und Prozeduralen Design macht den Code erst recht schwer les- und wartbar.

Naiv: Verwendet Typo3 und macht sich Sorgen um die Sicherheit :)

Frage: Was habt ihr alle gegen Standards? (PHP Standards and Recommendations - PSR)

Security I: Ohne genau zu wissen, wo die SessionID herkommt und wie das Cookie behandelt wird, wird es schwer etwas zu der Sicherheit des Systems zu sagen. Typo3 ist (war) anfällig gegen einen Angriffsvektor namens "Session Fixitation". Das muss man dabei betrachten und gegebenenfalls nachsteuern.

Security II: Cookies sind als Benutzereingabe zu werten und auch genau so zu behandeln. Das heißt: Die Superglobalen Variablen (die mit $_ anfangen) sind Tabu. filter_input(INPUT_COOKIE, ... )

Generell gilt (leider), dass PHP-Sessions mehrere Probleme haben:
* Sie sind am Cookie angreifbar und anfällig.

* Sie sind blocking - man muss sich manuell darum kümmern nicht in ein "death lock" zu geraten bei der Verwendung von asynchronen Requests.

* Sie sind Clientseitig nicht geschützt. Die einzige Sicherheit besteht aus einer "Zeit zu Zufall" Korrelation. Es ist keine Krypto im Spiel. Dadurch kann ein Angreifer mit den Cookie-Inhalten "herumspielen". Man kann sie in seltenen Fällen mit einer Web Application Firewall mit Krypto vor Manipulationen schützen (HMAC Signatur). Eigentlich muss man dafür aber das Rechenzentrum selber unter seiner Kontrolle haben.

* Die session.gc_maxlifetime trifft keine genaue Aussage darüber, ob und wann der Garbage Collector genau ausgelöst wird und die Session auch wirklich killt und aufräumt. Hat man den GC abgeschaltet und nicht an die Sessions gedacht bleiben die IDs ewig gültig obwohl man ja eine "maximale Lebensdauer" konfiguriert hat. An solche Abhängigkeiten denken einige leider nicht. Um eine wirklich realistisch eintreffende Maximallaufzeit einer Sitzung muss man sich also selber kümmern (wie es hier ja getan wird) -> Fummelkram.

Es gibt bessere Systeme zur Identifikation und Autorisierung, die Claims und Crypto samt Prüfung gleich eingebaut haben und Scopes (Systemberechtigungen) und anderen Payload mitliefern können (JWT zum Beispiel). Edit: Vorsicht - Cryto heißt hier nicht "verschlüsselter Inhalt" sondern kryptographisch signiert - das ist ein Unterschied... Klartextkennwörter und andere Geheimnisse gehören da natürlich nicht hinein.
 
Zuletzt bearbeitet:
ayngush schrieb:
Security II: Cookies sind als Benutzereingabe zu werten und auch genau so zu behandeln. Das heißt: Die Superglobalen Variablen (die mit $_ anfangen) sind Tabu. filter_input(INPUT_COOKIE, ... )
Wozu? Die Session ID wird bzw. sollte direkt und ausschließlich mit der DB genutzt werden, die kann komplett durchgereicht werden. Um das Escaping kümmert sich die DB bzw. der Layer selbstständig. Aber ja: Cookies sind genauso User Input.

filter_* ist mir zudem viel zu suspekt... Was ist der Unterschied zwischen (int)$_COOKIE["foo"] und filter_input( INPUT_COOKIE, "foo", FILTER_SANITIZE_NUMBER_INT ) außer dem immens höheren Schreibaufwand? Einige Filter (siehe Doku; FILTER_VALIDATE_URL, FILTER_VALIDATE_INT, FILTER_VALIDATE_EMAIL) sind falsch und/oder haben unerwartetes Verhalten.

Hier ist mir ne manuelle Prüfung sicherer. Wenn ich ein int erwarte, caste ich auf int, im Fehlerfall wird dies 0. Je nach Anwendungsfall kann sich die Prüfung natürlich noch unterscheiden (null korrekt? (vielleicht doch filter_var?), 0 korrekt?, leerer String?, Exception?, ...).

Weiterhin:
Code:
<?php

$_GET["a"] = $a = "siudhfiaushfisdf";
var_dump( [
    "int" => (int)$_GET["a"], // korrekt => 0
    "input" => filter_input( INPUT_GET, "a", FILTER_VALIDATE_INT ), // null? 
    "var" => filter_var( $a, FILTER_VALIDATE_INT ) // korrekt => false, Filter nicht gültig
] );

$_GET["a"] = $a = "20";
var_dump( [
    "int" => (int)$_GET["a"], // korrekt => 20
    "input" => filter_input( INPUT_GET, "a", FILTER_VALIDATE_INT ), // null?
    "var" => filter_var( $a, FILTER_VALIDATE_INT ) // korrekt => 20, Filter gültig
] );

$_GET["a"] = $a = 20;
var_dump( [
    "int" => (int)$_GET["a"], // korrekt => 20
    "input" => filter_input( INPUT_GET, "a", FILTER_VALIDATE_INT ), // null?
    "var" => filter_var( $a, FILTER_VALIDATE_INT ) // korrekt => 20, Filter gültig
] );
Aha, warum per filter_input() null, per filter_var() korrekt/wie erwartet? Laut Doku or NULL if the variable_name variable is not set. Aha. Aber wo ist $_GET["a"] nicht gesetzt? Wo zieht das Ding die Daten her? Ich habe a in $_GET gesetzt. Steht auch irgendwo in den Kommentaren drunter.

Sorry, für mich sind die Funktionen sehr fragwürdig. Es erspart einem bei ner RFC5322 validen eMail vielleicht ein wenig Recherchierarbeit (keine fünf Minuten: https://regex101.com/library/gJ7pU0), lädt aber doch zum Schlampen ein bzw. produziert Unerwartetes. Übrigens ist die Mail Validierung (FILTER_VALIDATE_EMAIL) nach RFC822 komplett obsolet (822 -> 2822 -> 5322). Wenn man es wenigstens updaten würde, aber eine eigene Abstraktion ist da bedeutend besser.
nyromant schrieb:
Hijacking kann man noch verhindern in dem man die IP noch in der Session speichert, dann hat man allerdings evtl Probleme bei Mobilnutzern, da manche ISPs NAT verwenden oder evtl. Proxies bei denen sich die User-IP ändern kann.
Dann kann man natürlich den User Agent nutzen. Ist aber auch keine Sicherheit, nur ein weiteres Merkmal. Irgend ein Merkmal zur weiteren Identifikation ist aber immer ein Muss, sonst kann die Session jeder übernehmen.

@ TE: Muss es dieses MySQLi Geschwurbel sein? Gibts nicht bereits einen DB Layer (vorzugsweise PDO)? Dieses bind_param Gedöns ist in jeder Form gruselig. Nutz lieber PDO mit Named Parameters, das ist wartbar.

Zeile 22 bis 28 sind auch gruselig.
King Domi schrieb:
// prüfen, ob Username-Variable existent (wenn nicht ist DB Abfrage schiefgegangen)
Initialisier (sichtbar) mit null! Nichts ist schlimmer als nicht initialisierte Variablen, die dann irgendwo bei Durchlauf x auftauchen, weil der User plötzlich ein ö im Namen hat. Macht keinen Spaß im Debugger.
King Domi schrieb:
Weiterhin läuft ein Cronjob der alle 15 min prüft ob die Sessions älter als 60min sind und setzt dann den Status active auf 0
Code:
DELETE FROM fe_users_currloggedin WHERE (lastAction + :timeout) < NOW()
currloggedin - Ist das Suaheli? Schreib es aus, dann weißt du auch in drei Jahren noch was das heißt oder der Nächste, der sich damit rumschlagen muss.

Zumindest ist der Code bedeutend besser als das eBay Modul von Prestashop oder auch Prestashop selbst (Preiskalkulation mit 20 Parametern ahoi). :freaky:
 
Ich weiß nicht, ob Du das in Dein bestehendes Script integrieren kannst, aber beachte folgendes:
Du fährst mit deiner Lösung zweigleisig. PHP verwaltet die Sessions intern als Dateien, Du möchtest Sie jedoch in einer Datenbank haben und spiegelst die Informationen daher bei jedem Aufruf. Womöglich ist es sinnvoll, stattdessen die Sessions direkt in der Datenbank zu verwalten; dazu kannst Du den Default SessionHandler mit einer eigenen Klasse überschreiben, die die Informationen stattdessen in der Datenbank ablegt. Ich vermute, dass Dein Code dadurch deutlich eleganter und wiederverwertbarer wird.
 
Yuuri schrieb:
Hier ist mir ne manuelle Prüfung sicherer. Wenn ich ein int erwarte, caste ich auf int, im Fehlerfall wird dies 0. Je nach Anwendungsfall kann sich die Prüfung natürlich noch unterscheiden (null korrekt? (vielleicht doch filter_var?), 0 korrekt?, leerer String?, Exception?, ...).

bzgl. dem cast, leider nein:
Code:
<?php

$arrrr = [
        '12345',
        'b12345',
        '123cdefg',
];

foreach ($arrrr as $val) {
        echo $val. ' int: '.((int)$val).PHP_EOL;
}
12345 int: 12345
b12345 int: 0
123cdefg int: 123
 
Zuletzt bearbeitet: (Textzeichen harmonisiert)
Yuuri schrieb:
Wozu? Die Session ID wird bzw. sollte direkt und ausschließlich mit der DB genutzt werden, die kann komplett durchgereicht werden. Um das Escaping kümmert sich die DB bzw. der Layer selbstständig. Aber ja: Cookies sind genauso User Input.

Weil man ausnahmslos alle Benutzereingaben filtert. Nicht castet, filtert.
Das macht man, weil es nicht sicher ist, dass bei einer Typumwandlung nicht unvorhergesehene Dinge passieren.
Ja, die Filter und deren "eigenartiges Verhalten" muss man eben verstehen lernen, wie leider so viel "eigenartiges Verhalten" bei PHP.

Yuuri schrieb:
filter_* ist mir zudem viel zu suspekt... Was ist der Unterschied zwischen (int)$_COOKIE["foo"] und filter_input( INPUT_COOKIE, "foo", FILTER_SANITIZE_NUMBER_INT ) außer dem immens höheren Schreibaufwand?

Der Unterschied ist, dass ich beim Casten auf Integer kein false oder null erhalte.
Der Unterschied ist, dass man Warnungen bekommt, wenn das Cookie nicht gesetzt ist, wodurch man wieder vorweg mit isset oder !empty rumfuhrwerken muss.
Der Unterschied ist, dass eine Typumwandlung zu unvorhergesehenen ungewollten Effekten führen kann.


Yuuri schrieb:
Einige Filter (siehe Doku; FILTER_VALIDATE_URL, FILTER_VALIDATE_INT, FILTER_VALIDATE_EMAIL) sind falsch und/oder haben unerwartetes Verhalten.

Ich weiß, ich kenne die Verhalten. Die sind nicht unerwartet, die sind nur dann unerwartet, wenn man sich mit der Materie des Filterns von Eingaben einfach nicht beschäftigen möchte.

Yuuri schrieb:
Hier ist mir ne manuelle Prüfung sicherer. Wenn ich ein int erwarte, caste ich auf int, im Fehlerfall wird dies 0. Je nach Anwendungsfall kann sich die Prüfung natürlich noch unterscheiden (null korrekt? (vielleicht doch filter_var?), 0 korrekt?, leerer String?, Exception?, ...).

Ich halte das so generell gesprochen für einen Fehler im Typfehlerfall auf Integer 0 zu casten aber nunja... bin ja nicht dein Prof aber für mich ist das ein schlechtes Codedesign.

Yuuri schrieb:
Weiterhin:
Code:
<?php

$_GET["a"] = $a = "siudhfiaushfisdf";
var_dump( [
    "int" => (int)$_GET["a"], // korrekt => 0
    "input" => filter_input( INPUT_GET, "a", FILTER_VALIDATE_INT ), // null? 
    "var" => filter_var( $a, FILTER_VALIDATE_INT ) // korrekt => false, Filter nicht gültig
] );

Aha, warum per filter_input() null, per filter_var() korrekt/wie erwartet? Laut Doku or NULL if the variable_name variable is not set. Aha. Aber wo ist $_GET["a"] nicht gesetzt? Wo zieht das Ding die Daten her? Ich habe a in $_GET gesetzt. Steht auch irgendwo in den Kommentaren drunter.

filter_input ... validate_int ist richtigerweise null, wenn integer erwartet wird und ein string übergeben wird.
Er validiert den vorhandenen Input auf Integer und so ein Input ist nicht vorhanden, deswegen null.
Auch das vertritt Sicherheitsaspekte, dass nicht aus einem erwarteten Integer ein gültiger Boolean Wert wird! Aber ja, Du hast Recht, vile PHPler tuen sich einfach immer noch sehr schwer mit der korrekten Typisierung ihrer Sprache!

Anderes sieht es bei filter_var aus, dann ist die Variable ja dennoch vorhanden, passt aber, wie von Dir richtig dargestellt, nicht in den Integer Filter, dadurch ist das Ergebnis des Filters logisch falsch.

Der kleine aber feine Unterschied ist: Das eine verwendet man bei Inputs, das andere nicht!

Yuuri schrieb:
Sorry, für mich sind die Funktionen sehr fragwürdig. Es erspart einem bei ner RFC5322 validen eMail vielleicht ein wenig Recherchierarbeit (keine fünf Minuten: https://regex101.com/library/gJ7pU0), lädt aber doch zum Schlampen ein bzw. produziert Unerwartetes. Übrigens ist die Mail Validierung (FILTER_VALIDATE_EMAIL) nach RFC822 komplett obsolet (822 -> 2822 -> 5322). Wenn man es wenigstens updaten würde, aber eine eigene Abstraktion ist da bedeutend besser.

Für E-Mail habe ich den Filter noch nie verwendet, welches RFC da aktuell ist kann ich nicht beurteilen. Sollte es veraltet sein, sollte man es aktualisieren, das stimmt.

Yuuri schrieb:
Dann kann man natürlich den User Agent nutzen. Ist aber auch keine Sicherheit, nur ein weiteres Merkmal. Irgend ein Merkmal zur weiteren Identifikation ist aber immer ein Muss, sonst kann die Session jeder übernehmen.

Krypto!

User Agents sind schlecht (können wechseln, TOR mit wechselnden Agens-Strings zur Obfuskation)
IPs sind schlecht (können wechseln, TOR)

Yuuri schrieb:
@ TE: Muss es dieses MySQLi Geschwurbel sein? Gibts nicht bereits einen DB Layer (vorzugsweise PDO)? Dieses bind_param Gedöns ist in jeder Form gruselig. Nutz lieber PDO mit Named Parameters, das ist wartbar.

bind_param verwendet man, um eine lokale Variable an die Middleware zu binden.
Das macht man aus Zwei verdammt guten Gründen:
Grund 1: Wenn man ein Statement loopt. Außerhalb der Schleife vorbereiten und die lokale Variable (korrekt benannt natürlich - Semantik!) binden, innerhalb der Schleife der lokalen Variable einen Wert zuweisen und das Statement executen -> für mehr Performance und weniger CPU cycles und Speicherverbrauch.

Grund 2: Um aus einem Integer kein String für das Statement zu machen, was die DB dann wieder in ein Integer Casten muss!
Dadurch wird (je nach DBMS) teilweise auch verhindert, dass die Idizes verwendet werden, da gibt es dann auf einmal "Hash Matches" anstatt "Index-Seeks" im Ausführungsplan des DBMS. Je nach Umfang der Webanwendung will man das nicht.

Grüße


Edit:

nyromant schrieb:
bzgl. dem cast, leider nein:
Code:
<?php

$arrrr = [
        '12345',
        'b12345',
        '123cdefg',
];

foreach ($arrrr as $val) {
        echo $val. ' int: '.((int)$val).PHP_EOL;
}

Danke! Gar nicht gesehen :)

Also hier nochmal das Mantra zum Mitschreiben:
Cookies sind Benutzereingaben.
Ich werde alle Benutzereingaben filtern.
Typumwandlungen sind keine Filter und führen zu unvorhergesehen Effekten.
 
Zuletzt bearbeitet:
Erm, nur zur Klarstellung: Der Hinweis re: MySQLi und PDO bezog sich auf die Abstraktion durch PDO und hat mit prepared statements nicht das Geringste zu tun.

MySQL/ MySQLi *ist* Geschwurbel (um den Ausdruck einfach mal zu übernehmen), schon deswegen, weil man eine komplett neue DB-Klasse implementierung muß, wenn man das Backend wechseln will. Mit PDO geht das relativ problemlos.

Siehe dazu auch hier.


Ansonsten ist -jedenfalls meines Erachtens -- eine zuverlässige Eingabevalidierung viel wichtiger als die Antwort auf die Frage, WIE man das gemacht hatte. Wenn ich ne Variable hab, die mir übergeben wird, und ich will sicherstellen, daß die a) den übergebenen Wert hat, wenn sie als Eingabe gültig war oder aber b) einen definierten anderen, falls nicht, dann bau ich da ne fünfzeilige Funktion drumrum, dann ist die gekapselt und ich muß keinen Gedanken mehr dran verschwenden.

Ob da am Ende was mit preg_match() dort drin steht oder was mit
Code:
$validate_result = (int)$input; 
return $input == $validate_result? $validate_result : $default_value;
ist relativ wumpe.

Besonders, da man dann dort gleich eine Plausibilitätsprüfung übergeben kann. Aha, der Benutzer hat ne 5 übergeben. Das ist ein int. Der ist > 0. Schonmal gut. Aber eigentlich hätte das ein Wert zwischen 8 und 80 sein sollen. Nicht ganz so gut.
Das fängt mir keine Typvalidierung ab, ist aber nicht minder wichtig für sauberes Codeverhalten.
 
Zuletzt bearbeitet von einem Moderator:
ayngush schrieb:
Weil man ausnahmslos alle Benutzereingaben filtert. Nicht castet, filtert.
Ja, da stimm ich dir auch komplett zu. Aber wo wird die Session ID anderweitig verwendet, außer die Daten irgendwo zu hinterlegen oder abzurufen? Von daher muss genau dieser Wert an dieser Stelle überhaupt nicht validiert werden. Entweder existiert ein Datensatz für genau diesen Wert/diesen String irgendwo oder nicht.
ayngush schrieb:
Der Unterschied ist, dass ich beim Casten auf Integer kein false oder null erhalte.

Ich halte das so generell gesprochen für einen Fehler im Typfehlerfall auf Integer 0 zu casten aber nunja...
Hab ich wie gesagt geschrieben. Je nach Fehlerfall kann natürlich entsprechend reagiert werden. Aber je nachdem wie bspw. mein ORM arbeitet und es den Eintrag (int)"asdf" aus der DB ziehen soll, wird es eben auf die ID 0 gecastet und eine ID 0 gibt es nicht, ergo ist es ein nicht gültiges Model bzw. ohne Daten usw. Es kommt halt immer auf den Anwendungsfall an und wie etwas implementiert wird. Wenn es hierbei um Zahlungsdaten geht, arbeitet man sicherlich viel restriktiver als wenn man einen Kommentar irgendwo hinterlegt.
ayngush schrieb:
Krypto!

User Agents sind schlecht (können wechseln, TOR mit wechselnden Agens-Strings zur Obfuskation)
IPs sind schlecht (können wechseln, TOR)
Gehen wir mal von billigen Shop-Systemen in ner Shared Hosting Umgebung aus. Was für Merkmale nutzt du bspw. hierbei?
ayngush schrieb:
bind_param verwendet man, um eine lokale Variable an die Middleware zu binden.
Was ich meine: Positionelle Parameter sind scheußlich unübersichtlich und fehleranfällig.
Code:
<?php

$a = $mysqli->prepare( <<<SQL
INSERT INTO foo
SET
	a = ?,
	b = ?,
	c = ?,
	d = ?,
	e = ?,
	f = ?
ON DUPLICATE KEY UPDATE
	b = ?,
	c = ?,
	d = ?,
	e = ?,
	f = ?
SQL
);
$a->bind_param( "sissiiissii", ... );
Code:
<?php

$a = $pdo->prepare( <<<SQL
INSERT INTO foo
SET
	a = :i1,
	b = :i2,
	c = :i3,
	d = :i4,
	e = :i5,
	f = :i6
ON DUPLICATE KEY UPDATE
	b = :u1,
	c = :u2,
	d = :u3,
	e = :u4,
	f = :u5
SQL
);
$a->bindValue( ":i1", "", PDO::PARAM_STR );
$a->bindValue( ":i2", 0, PDO::PARAM_INT );
$a->bindValue( ":i3", "", PDO::PARAM_STR );
...
$a->bindValue( ":u1", "", PDO::PARAM_STR );
...
Und jetzt stell dir eine Tabelle mit 30 Spalten vor... Viel Spaß beim Durchzählen aller Parameter und wehe du fässt einen oder mehrere an oder änderst die Reihenfolge.
RalphS schrieb:
MySQL/ MySQLi *ist* Geschwurbel (um den Ausdruck einfach mal zu übernehmen), schon deswegen, weil man eine komplett neue DB-Klasse implementierung muß, wenn man das Backend wechseln will. Mit PDO geht das relativ problemlos.
Theoretisch ja, aber in der Praxis sind die Dialekte dann doch zu unterschiedlich. Und stur ANSI SQL zu schreiben...
 
Deswegen auch das "relativ". :)

Aber es erleichtert schon mal ungemein. Und wie man ja aus Deiner Gegenüberstellung sehen kann, auch übersichtlicher/nachvollziehbarer.

Notfalls kann man ja kapseln. Bei mir gibt's zB eine Methode insert(array columns, array pk, int DB_INSERT_METHOD) wobei columns assoziativ ist, pk den Primärschlüssel beinhaltet (soweit erforderlich) und DB_INSERT_METHOD einfach eine numerische Konstante ist, welche sagt, ob DO NOTHING, DO UPDATE oder sonstwas.
Ändert sich das Backend, wird die Funktion erweitert um case neuesbackend. Fertig. Weitere Codeänderungen nicht erforderlich und für tatsächliche INSERTs bleibt nur noch das Wesentliche übrig.

Klar, für jeden Anwendungsfall paßt das natürlich nicht, aber zumindest der Erfahrung nach sind INSERTs soo variabel nicht.

-- In jedem Fall kommt es halt drauf an, daß der zugehörige DB-Code nur genau EINmal da ist, damit man ihn auch warten kann. Aber, das wurde ja im Thread bereits mehrfach(?) angesprochen.
 
Es ist übrigens egal, wo die SessionID verwendet wird. Sie ist ein User-Input und wird deswegen gefiltert.
Das Paradigma dazu lautet FIEO - Filter Inputs, Escape Outputs.

Das macht man einfach, weil man nicht vorhersehen kann (hellsehen, Kristallkugel uns so...), was einem alles mit "komischen" Anfragen zukünftig dabei oder dadurch um die Ohren fliegt. Hier geht es ja in der Intention um sicheren Code. Und sicherer Code ist: Filter Inputs, Escape Outputs. Bedingungslos. Ausnahmslos. Ja, man muss mehr schreiben. Schafft euch eine gute IDE an, dann spart man gleich wieder das mehr an Schreibarbeit durch gute Autovervollständigungen ein.

Das filter_input nicht auf

$_GET['foo'] = 'bar';

"zugreifen" kann ist kein Fehler. Das ist in den PHP-Kommentaren schon "etwas falsch" dargestellt. filter_input holt sich die Daten aus dem Request Header oder Request Body (bei Post, File), von der selben Stelle, wo das Backend sich die Daten auch herholt, um die Superglobalen Arrays aufzubauen.
Und natürlich ändert man den Request Header nicht, wenn man nachträglich $_GET['foo'] setzt / verändert.
Aus meiner Sicht müsste $_GET, $_POST und $_FILE schreibgeschützt sein - mindestens eine Notice oder Warning werfen... PHP ist da einfach zu gnädig.

Bei der "bind_param"-Geschichte dachte ich Du beziehst dich auf das "binden" an sich und nicht auf das ordentliche Benennen der Parameter. Ich würde niemals mit "?"-Parametern im SQL arbeiten. Das ist auch ein auf Faulheit basierendes, schlechtes Codestyling, was am Ende für "Fremde" oder nach ein paar Monaten auch für einen selbst einfach nur unübersichtlich ist. Oder soll man wirklich jedes mal "abzählen"...
Natürlich benennt man immer alle Parameter, Variablen, Platzhalter, Konstanten, Methoden, Klassen sinnvoll und einheitlich und macht sich über eine standardisierte Konvention vorher kundig (CamelCase oder snake_case - nicht wild mischen usw.) und entlehnt davon sein eigenes Vorhaben. Das vereinfacht es für alle. Daher ja auch oben mein Rat: PSR anschauen.

Hier noch ein guter Link: http://www.phptherightway.com/
 
Zurück
Oben