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

[Michael_K]

Hallo Felix,

danke für den Post. Aber ganz ehrlich, das Einzige, was man dem kleinen Code-Schnipsel vorwerfen kann, ist die fehlende Information/inline Kommentierung, was welche Variable beinhaltet bzw. bezweckt. Und FeFe schreibt auch viel Unsinn, wenn der Tag lang ist.

Ohne einen genauen Überblick zu haben, an welcher Stelle und Pipeline in der Architektur dieses Stück Code sitzt, ist es irgendwie sinnfrei über Sicherheit zu diskutieren. Ich habe mir mal den Originalpost durchgelesen, die gewichtigen Fehler werden da an ganz anderer Stelle gemacht.

Das man Request-Headern nicht blind vertrauen darf, sollte bekannt sein. Aber der hier aufgeführte Code hat wohl nicht die Aufgabe, dafür Sorge zu tragen. Und PHP ist eine ziemlich blöde Idee für ein Backend (meine persönliche Meinung).

Die Kernbotschaft ist doch eigentlich stets die Gleiche: Beachte, ob man den Inputdaten vertrauen kann, wenn nicht, dannn müssen die Daten zuvor ausreichend validiert werden. Was "ausreichende" Validierung bedeutet, ist davon abhängig, an welcher Stelle in der Architektur der Code abgearbeitet wird.

Und entgegen FeFe, sollten Usernamen tatsächlich eine bestimmte Länge nicht überschreiten. Aber das ist eben nur ein Teil der Validierung.

Gruss Michael

[Felix Riesterer]

Lieber Michael_K,

danke für den Post. Aber ganz ehrlich, das Einzige, was man dem kleinen Code-Schnipsel vorwerfen kann, ist die fehlende Information/inline Kommentierung, was welche Variable beinhaltet bzw. bezweckt. Und FeFe schreibt auch viel Unsinn, wenn der Tag lang ist.

für mich war Fefe der Erstkontakt mit diesem Code. Das dortige Beispiel war für mich nur ein Aufhänger und Ideengeber, um einen kurzen Beitrag für Hobbyisten in diesem Forum zu schreiben.

Ob sich Fefe hier berechtigterweise amüsiert oder nicht, oder ob ein Backend von Security-Software überhaupt mit PHP realisiert werden sollte, ist nicht mein Anliegen.

Liebe Grüße

Felix Riesterer

[mitleserin]

Moin,

und sollte man nicht besser auch die mb_Funktionen nutzen für den Stringvergleich, bzw. vorher ausschließen. dass es sich um andere Codierungen als 8-Bit handeln kann?

Grüße aus Düsseldorf

M.

[Rolf B]

Hallo mitleserin,

angesichts der Tatsache, dass mit Locales hantiert wird: ja, auf jeden Fall. Das ist wesentlich gravierender als das Umkopieren von $_POST-Feldern in lokale Variablen, ohne zu validieren und ohne zu prüfen, ob der $_POST-Eintrag überhaupt da ist.

In Sprachen mit "klassischem lateinischem" Alphabet sind nur wenige Codepoints dabei, die außerhalb des ASCII-Zeichensatzes liegen. Beispielsweise ein Russe oder Grieche würde sich aber schon wundern, warum sein Name nur 31 Zeichen lang sein darf…

Rolf

[Felix Riesterer]

Liebe mitleserin,

und sollte man nicht besser auch die mb_Funktionen nutzen für den Stringvergleich,

welche? Und wo in meinem Beispiel würdest Du die einsetzen? In user_exists? Warum genügt da kein simpler Vergleich zweier (String-)Variablen?

bzw. vorher ausschließen. dass es sich um andere Codierungen als 8-Bit handeln kann?

Warum sollte das wichtig sein?

$users = ['a-a-A-A', 'o-o-O-O', 'u-u-U-U', 's-S-s'];
$test  = ['a-ä-A-Ä', 'o-ö-O-Ö', 'u-ü-U-Ü', 's-S-ß'];
foreach ($test as $login) {
  if (in_array($login, $users)) {
    echo '„',$login,'“ ist gültig<br>';
  } else {
    echo '„',$login,'“ gilt NICHT<br>';
  }
}
// Ergebnisse:
//„a-ä-A-Ä“ gilt NICHT
//„o-ö-O-Ö“ gilt NICHT
//„u-ü-U-Ü“ gilt NICHT
//„s-S-ß“ gilt NICHT

Liebe Grüße

Felix Riesterer

[Rolf B]

Hallo Felix,

Warum sollte das wichtig sein?

Weil strlen() nur dann richtig zählt. Dass du keine Längentests mehr machst, heißt nicht, dass sie nicht in anderen Prüfungen nötig sein könnten.

Rolf

[Felix Riesterer]

Lieber Rolf,

Dass du keine Längentests mehr machst, heißt nicht, dass sie nicht in anderen Prüfungen nötig sein könnten.

verstehe. Aber eine Architektur so zu bauen, dass eine Funktion mit vielen Parametern selbst eine Plausibilitätsprüfung macht, für deren Aufruf man aber sicherstellen muss, dass die Stringlängen in den Parameter-Werten Grenzen einhalten, ist doch kaum sinnvoll! Das klingt so, als würde man ein Web-Frontend für etwas basteln, das seine Daten dann in einen Aufruf auf der Konsole füttert. Genau das ist übrigens einer der Hauptkritikpunkte an der Story mit PaloAlto, die genau das tun, ohne die Werte für den Kontextwechsel „Konsole“ entsprechend zu prüfen.

Also: Ja, wenn man eine Maximallänge für einen user-generated Stringwert braucht, dann kann man das mit mb_strlen besser als mit strlenprüfen.

Liebe Grüße

Felix Riesterer

[Rolf B]

Hallo Felix Riesterer,

Aber eine Architektur so zu bauen, dass eine Funktion mit vielen Parametern selbst eine Plausibilitätsprüfung macht, für deren Aufruf man aber sicherstellen muss, dass die Stringlängen in den Parameter-Werten Grenzen einhalten, ist doch kaum sinnvoll!

100% ACK.

Aber das Chaos, das da im Artikel beschrieben wurde, erinnert an ein Legacy-Gestrüpp. Mutmaßlich konnte/durfte/sollte derjenige, der den Fix gemacht hat, den eigentlicheen Code nicht ändern und hat deswegen „mal flott“ ein Prüfmodul vorgeschaltet.

Ein Grund für "nicht dürfen" könnte sein, dass der eigentliche Code irgendwie zertifiziert ist und jegliche Änderung eine Rezertifizierung verlangt. Der Vorschalt-Hack hingegen unterlag dieser Vorgabe nicht. Bullshit? Ja. Aber so sind Institutionen nun mal. Wenn der Sinn einer Vorschrift uns im Weg ist, halten wir uns an die Buchstaben und hacken fröhlich vor uns hin.

Rolf

[Auge]

Hallo

Wenn der Sinn einer Vorschrift uns im Weg ist, halten wir uns an die Buchstaben und hacken fröhlich vor uns hin.

Könnte das nicht heißen: „Wenn der Sinn einer Vorschrift uns im Weg ist, halten wir uns an die Buchstaben und hacken fröhlich drumherum.“ 😉😁

Tschö, Auge

[Rolf B]

Hallo Auge,

schon. Aber das würde ja Umsicht bedeuten.

Rolf