Wie man's nicht machen sollte

[Felix Riesterer]

Liebe PHP-Interessierte,

heute habe ich bei Fefe einen Beitrag gelesen, in dem er sich über eine Sicherheitslücke in Software von Palo Alto lustig macht. Dabei zeigt er PHP-Code, den er aber nicht für Laien kommentiert, weil er bei seiner Leserschaft davon ausgeht, dass die den Witz an der Sache selbst erkennen. In dem Code werden Dinge getan, wie man sie gelegentlich auch hier in Fragepostings findet. Deswegen dachte ich, dass ich davon etwas kurz bespreche.

Umkopieren von Variablen / Magic Numbers

$locale = isset($_POST['locale']) ? $_POST['locale'] : $_SESSION['locale'];
$user = $_POST['user'];
$userRole = $_POST['userRole'];
$remoteHost = $_POST['remoteHost'];
$vsys = $_POST['vsys'];
$editShared = $_POST['editShared'];
$protocol = $_POST['prot'];
$serverPort = $_SERVER['SERVER_PORT'];
$rbaXml = $_POST['rbaxml'];
$hideHeaderBg = $_POST['hideHeaderBg'];

if (strlen($user) <= 63
  && strlen ($userRole) < 256
  && strlen ($remoteHost) < 256
  && strlen ($vsys) < 128
  && strlen ($editShared) < 128
  && strlen ($protocol) < 128
  && strlen ($serverPort) < 128
  && strlen ($rbaXml) < 1024 * 1024
  && strlen ($locale) < 256
  && strlen ($hideHeaderBg) < 128
) {
  panCreateRemoteAppwebSession(
    $user,
    $userRole,
    $remoteHost,
    $vsys,
    $editShared,
    $protocol,
    $serverPort,
    $rbaXml,
    $locale,
    $hideHeaderBg
  );
} else {
  error_log("[englische Fehlermeldung]");
}

Die Variable $locale ist ein Wert, der aus zwei verschiedenen Quellen kommen kann ($_POST oder $_SESSION) und hat daher einen Sinn. Die restlichen neun Variablen sind reine Umbenennungen und verschleiern nur, dass die Werte aus dem $_POST-Array kommen, also im Zweifelsfall bösartige und von einem User mit Absicht manipulierte Werte sind, die das System aushebeln sollen.

Bevor die Funktion panCreateRemoteAppwebSession aufgerufen wird, findet noch eine Prüfung statt, ob alle Werte die erforderlichen Längenbeschränkungen einhalten. In der Bedingung der if-Verzweigung sehen wir lauter Vergleiche, bei denen der Rückgabewert der Funktion strlen (Anzahl Zeichen im String) mit einer Zahl verglichen wird. Diese Zahlen stehen einfach so im Code, ohne Erklärung, warum es jeweils genau diese Anzahl an Zeichen höchstens sein dürfen. Solcherlei Zahlenangaben im Code nennt man magic numbers, die zu den Unarten des Programmierens zählen.

Was an dieser Stelle wirklich hätte passieren müssen, wäre eine echte Plausibilitätsprüfung gewesen. Man kann im Ernstfall für jeden String eine eigene Prüffunktion schreiben, da die Strings offensichtlich die unterschiedlichsten Bedeutungen haben. Nehmen wir uns die einfachste Prüfung vor: Benutzername und Benutzerrolle:

function user_exists ($login) {
  $available_user_logins = [];

  // Hier müsste Code stehen, der alle Login-Namen in das Array schreibt.
  // Also so etwas wie eine Datenbankabfrage,
  // oder einen Ladevorgang aus einer Datendatei

  return in_array($login, $available_user_logins);
}

function user_role_exists ($role) {
  $available_roles = [];

  // Hier müsste Code stehen, der alle Benutzerrollen in das Array schreibt.

  return in_array($role, $available_roles);
}

In diesen beiden Plausibilitätsprüfungen kann man sehen, dass der jeweils vom Benutzer übermittelte Wert mit bereits vorhandenen Werten verglichen wird. So können bösartige Werte keinen Schaden anrichten.

// jetzt die Verzweigung mit Plausibilitätsprüfungen
if (user_exists($_POST['user'])
  && user_role_exists($_POST['userRole'])
  && ...
) {
  ...
} else {
  ...
}

Man sieht nun in der Bedingung der if-Verzweigung, dass Benutzereingaben (also potenziell bösartige Werte) auf ihre Plausibilität hin geprüft werden. Und das individuell!

Auf eine sehr ähnliche Art und Weise kann man mit dem Wert für das Protokoll ($_POST['prot']) verfahren, der offensichtlich aus einer Liste ausgewählt wurde. Wenn man die möglichen Werte mit dem tatsächlich empfangenen Wert vergleicht, könnte man zusätzlich die Groß-/Kleinschreibung ignorieren (z.B. HTTP im Vergleich zu http).

Wieso eigentlich darf die Port-Angabe ($_SERVER['SERVER_PORT']) im obigen Beispiel bis zu 127 Zeichen haben? Eine Zahl mit so vielen Stellen könnte ich spontan nicht einmal korrekt benennen. Und dann gibt es da noch technische Begrenzungen, in welchem Zahlenraum sich eine Port-Angabe bewegen kann: 1 - 65535. Da hätte man besser nicht die Maximallänge des Strings geprüft, sondern den tatsächlichen Zahlenwert, den er abbildet:

function is_valid_port ($port) {
  $valid_ports = [
    80, // HTTP
    143, // HTTPS
    8080, // HTTP intranet
  ];

  return in_array(
    (int) $port, // vergleiche Ganzzahl anstatt String-Wert
    $valid_ports
  );
}

Nun darf man einigermaßen darauf vertrauen, dass die Werte im $_SERVER-Array keinen Unsinn beinhalten. Wenn man selbst in seinem Code die Werte dort niemals verändert, sondern immer nur ausliest, dann sind diese einigermaßen weniger unsicher. Eine Plausibilitätsprüfung ist aber trotzdem wichtig!

Liebe Grüße

Felix Riesterer

[Matthias Scharwies]

Servus!

Vielen Dank! Wär das nix für den Blog?

Herzliche Grüße

Matthias Scharwies

PS: Sitze im Elternsprechabend über MS Teams und habe 5min Lücke. Bis jetzt
2x niemand am gebuchten Termin da,
2x mit Handy telefoniert, weil Teams nicht geht
und 12 gute Gespräche.

[Camping_RIDER]

Aloha ;)

Danke fürs Aufschlüsseln!

Trotzdem möchte ich bei allem berechtigtem Spott zum Thema PaloAlto noch eins hier zu bedenken geben:

Kontextbehandlung ist eine Frage des Kontexts. Und der ist hier für ein abschließendes Urteil nicht hinreichend bekannt.

Zum Beispiel ist es sehr relevant, was in der Funktion

panCreateRemoteAppwebSession

passiert. Kann sein die von dir hier vermisste Kontextbehandlung passiert dort. Kann sein die hier kritisch betrachteten magic numbers erschließen sich aus der Funktionsweise bzw. den Annahmen dieser Funktion. Kann sein hier werden Bibliotheken bedient, die aus gutem Grund gekapselt sind aber im Vorfeld keine Kontextbehandlung über die reine Datenmenge (Strings mit maximal 127 Zeichen) hinaus benötigen. Kann sein der böswillige Userinput soll an dieser Stelle noch erhalten bleiben um anschließend unschädlich gemacht und geloggt zu werden. Und so weiter und so fort.

Ich empfinde das Gesamturteil an der Stelle also als verfrüht und möchte davor warnen, nur die augenscheinlichen red flags anzuschauen ohne im Detail zu wissen, ob sie im konkreten Fall denn relevant sind.

Grüße,

RIDER

[Felix Riesterer]

Lieber Camping_RIDER,

Zum Beispiel ist es sehr relevant, was in der Funktion

panCreateRemoteAppwebSession

passiert. Kann sein die von dir hier vermisste Kontextbehandlung passiert dort.

ja, kann. Wenn Du auf den verlinkten Artikel schaust, wirst Du sehen, dass da ursprünglich keine umkopierten Variablen standen. Es ist völlig unklar, warum man diese Änderung am Code vorgenommen und an die Kunden ausgerollt hat. Für unsere Belange bei SELFHTML aber ist das ein real world-Beispiel dafür, wie man es eben nicht machen sollte. Deswegen meine Ausführungen.

Liebe Grüße

Felix Riesterer