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