Bug oder Absicht

Ist der OpenSSL Heartbleed Bug wirklich ein Bug oder war das Absicht?
https://www.openssl.org/news/secadv_20140407.txt

A missing bounds check in the handling of the TLS heartbeat extension can be
used to reveal up to 64k of memory to a connected client or server.

Wie kann jemand der so sicherheitskritische SW wie OpenSSL programmiert einen bounds check vergessen? Bounds checks sind doch wohl die Grundlage für jeden der irgendwas mit sicherheitskritischer Client-Server Kommunikation programmiert. Das ist doch wohl eine der bekanntesten Angriffsmöglichkeiten. Wenn man dann hört, dass US-SW-Firmen Hintertürchen für die NSA einbauen (müssen), dann kann man ja schon auf die Idee kommen, das der OpenSSL Entwickler auch Geld oder sonstwas von der NSA bekommen hat.

inflames2k

Würde erklären, warum die alten Versionen nicht betroffen waren.

Aber ich hoffe immernoch auf das gute im Menschen und sage es war einfach ein dummer Schusselfehler.

Jodocus

Hier mal der Bug:
Der Server soll einfach nur eine Nachricht echo'en, die aus 'nem Header (bestehend aus 2 Bytes für einen Nachrichtentyp und für die Nachrichtenlänge) und einer beliebigem Payload, die zurückgeschickt wird. Die Payload-Größe wird dabei nicht auf Plausibilität überprüft (und kann demnach den größten ushort-Wert annehmen):

int tls1_process_heartbeat(SSL *s)
         {
         unsigned char *p = &s->s3->rrec.data[0], *pl;
         unsigned short hbtype;
         unsigned int payload; // <<--------------
         unsigned int padding = 16; /* Use minimum padding */

         /* Read type and payload length first */
         hbtype = *p++; // Typ einlesen
         n2s(p, payload); // Nachrichtenlänge einlesen
         pl = p;

         if (s->msg_callback)
                 s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
                         &s->s3->rrec.data[0], s->s3->rrec.length,
                         s, s->msg_callback_arg);

         if (hbtype == TLS1_HB_REQUEST)
                 {
                 unsigned char *buffer, *bp;
                 int r;

                 /* Allocate memory for the response, size is 1 bytes
                  * message type, plus 2 bytes payload length, plus
                  * payload, plus padding
                  */
                 buffer = OPENSSL_malloc(1 + 2 + payload + padding);
                 bp = buffer;

                 /* Enter response type, length and copy payload */
                 *bp++ = TLS1_HB_RESPONSE;
                 s2n(payload, bp);
                 memcpy(bp, pl, payload); // Zack!

                 r = ssl3_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, 3 + payload + padding);

Spannend, ob dieser Typ von Bug nicht auch noch anderswo in OpenSSL versteckt ist.

Warum segfault das memcpy() nicht? wenn `payload' groß ist aber `pl' z.B. nur auf ein byte zeigt?

Jodocus

Könnte passieren, aber eher unwahrscheinlich, weil der Memory-Bereich ja zum gleichen Prozess gehört, also auch kein Segfault.

mmazal schrieb:

Warum segfault das memcpy() nicht? wenn `payload' groß ist aber `pl' z.B. nur auf ein byte zeigt?

Weil nur lesend auf diesen Bereich zugegriffen wird und die Daten in einen Buffer kopiert werden, die ordentlich* reserviert wurden.

Hier ist das ganz einfach erklärt:
http://www.heise.de/security/artikel/So-funktioniert-der-Heartbleed-Exploit-2168010.html

* Mit ordentlich meine ich die Reservierung an sich, das nicht geprüft wird, wieviel genau zu reservieren ist, dies ist natürlich ein Teil dieses schweren Bugs.

Der Bug Programmierer meint, dass es wohl keine Absicht war.

http://www.stern.de/digital/online/sicherheitsluecke-in-openssl-deutscher-
programmierer-fabrizierte-heartbleed-2102774.html

Und der Reviewer hat es wohl auch nicht gesehen.
http://www.smh.com.au/it-pro/security-it/man-who-introduced-serious-heartbleed-security-flaw-denies-he-inserted-it-deliberately-20140410-zqta1.html

Warum schreiben C Programmierer eigentlich so schrecklichen Code? Haben die noch nie von sprechenden Variablennamen gehört? Warum verwenden die einfaches memcpy? Warum wird bei so sicherheitskritischem Code nicht eine eigene memcpy funktion geschrieben die structs nimmt, die den Buffer und die Länge enthalten und diese immer prüfen?

ScottZhang

Vllt gabs 100m€+Staatsbürgerschaft dafür

TyRoXx

So widerlicher Code ist leider allgemein akzeptiert. Das ist das eigentliche Problem an der ganzen Sache.

Gregor

Jodocus schrieb:

Hier mal der Bug:
Der Server soll einfach nur eine Nachricht echo'en, die aus 'nem Header (bestehend aus 2 Bytes für einen Nachrichtentyp und für die Nachrichtenlänge) und einer beliebigem Payload, die zurückgeschickt wird. Die Payload-Größe wird dabei nicht auf Plausibilität überprüft (und kann demnach den größten ushort-Wert annehmen)

Die eigentliche Frage hier ist glaube ich, warum in dem Zusammenhang ueberhaupt eine zusaetzliche Payload im Spiel ist, die eine beliebige Laenge haben kann. Laut dem Autor des Codes benoetigt man diese Payload eigentlich gar nicht, sondern sie wurde nur aus "Flexibilitaetsgruenden" eingebaut. ...zumindest nach dem, was Fefe sagt.

Ich kann mir durchaus vorstellen, dass man waehrend einer Dissertation (der Code ist wohl im Rahmen einer Dissertation entstanden) unter Umstaenden bezueglich Aspekten sensibilisiert ist, die nicht die entscheidenden sind. Zum Beispiel kann es durchaus sein, dass der betreuende Professor seinem Doktoranden andauernd irgendetwas von Flexibilitaet und Wiederverwendbarkeit erzaehlt hat. ...ebenso kann ich mir andere Motivationen fuer das Einfuehren dieser Payload vorstellen.

hustbaer

Gregor schrieb:

...zumindest nach dem, was Fefe sagt.

Wenn ich das so lese...

Und auch protokolltechnisch ergibt das keinen Sinn, so eine Extension überhaupt zu definieren. TCP hat seit 30 Jahren keep-alive-Support. Es hätte also völlig gereicht, das für TLS über UDP zu definieren (und auch da würde ich die Sinnhaftigkeit bestreiten).

... denke ich mir, der sollte sich auch lieber auf Sachen beschränken von denen er Ahnung hat.
TCP/IP Keepalive meine Fresse.
Es macht definitiv Sinn in einem L6/L7 Protokoll Keepalives zu haben.

lokoklo schrieb:

Warum verwenden die einfaches memcpy? Warum wird bei so sicherheitskritischem Code nicht eine eigene memcpy funktion geschrieben die structs nimmt, die den Buffer und die Länge enthalten und diese immer prüfen?

Ich vermute dass die Programmierer von OpenSSL hier performanten Code haben wollte, denn die machen das noch an weiteren Stellen im Code so.

Hier muss man einfach daran denken, dass eine Webseite > 10000 Zugriffe/s haben kann und dann sollte der Code halt auch nicht schnarchlahm sein.
Das erklärt dann die Motivation für so eine memcpy Funktion, wie die OpenSSL Macher sie realisiert haben.

TyRoXx schrieb:

So widerlicher Code ist leider allgemein akzeptiert. Das ist das eigentliche Problem an der ganzen Sache.

Hier möchte ich aber auch mal anmerken, dass an Hochschulen nur selten die Qualität des Codes angeschaut wird. Den meisten Profs genügt es leider, wenn das Programm läuft. Da muss man sich dann nicht wundern, wenn so etwas schon im Studium ohne Kritik hingenommen wird.

Ich habe mir jedenfalls schon oft gewünscht, dass die Profs oder die dafür abgestellten Praktikanten meinen Code beruteilen, aber vermutlich haben die auch keine Zeit dafür oder werden dafür nicht bezahlt.

Auskenner schrieb:

lokoklo schrieb:

Warum verwenden die einfaches memcpy? Warum wird bei so sicherheitskritischem Code nicht eine eigene memcpy funktion geschrieben die structs nimmt, die den Buffer und die Länge enthalten und diese immer prüfen?

Ich vermute dass die Programmierer von OpenSSL hier performanten Code haben wollte, denn die machen das noch an weiteren Stellen im Code so.

Hier muss man einfach daran denken, dass eine Webseite > 10000 Zugriffe/s haben kann und dann sollte der Code halt auch nicht schnarchlahm sein.
Das erklärt dann die Motivation für so eine memcpy Funktion, wie die OpenSSL Macher sie realisiert haben.

So wirds sein. Der ganze Code sieht so wohl überlegt aus.

https://github.com/openssl/openssl/commit/731f431497f463f3a2a97236fe0187b11c44aead

Also, defining constants, especially those with human-readable names, is a massive affront to performance! If another developer wants to know that "payload" actually describes the size of the buffer, they should go read the RFC.

lokoklo schrieb:

https://github.com/openssl/openssl/commit/731f431497f463f3a2a97236fe0187b11c44aead

Also, defining constants, especially those with human-readable names, is a massive affront to performance! If another developer wants to know that "payload" actually describes the size of the buffer, they should go read the RFC.

Die ganzen Kommentare für den Patch kannst du getrost ignorieren, die sind von irgendwelchen unbeteiligten Personen, die lustig sein wollen.

Und man darf nicht vergessen, dass das der Codeteil relativ unwichtig ist, und deshalb wenig Beachtung findet.

hustbaer schrieb:

Gregor schrieb:

...zumindest nach dem, was Fefe sagt.

Wenn ich das so lese...

Und auch protokolltechnisch ergibt das keinen Sinn, so eine Extension überhaupt zu definieren. TCP hat seit 30 Jahren keep-alive-Support. Es hätte also völlig gereicht, das für TLS über UDP zu definieren (und auch da würde ich die Sinnhaftigkeit bestreiten).

... denke ich mir, der sollte sich auch lieber auf Sachen beschränken von denen er Ahnung hat.
TCP/IP Keepalive meine Fresse.
Es macht definitiv Sinn in einem L7 Protokoll Keepalives zu haben.

Warum sollte man ein Keepalive in eine SSL Bibliothek einbauen? Was hat das mit Verschlüsselung zu tun? Ältere OpenSSL Versionen haben auch ohne funktioniert. Warum haben die diese Funktion nicht deaktiviert? Die wurde nur von Versionen verwendet, die diesen Bug haben.

Mal eine Frage.

Werden bei dem Bug von belegtem Speicher Daten kopiert oder nur solcher, der zuvor freigegeben* wurde?

* Da OpenSSL seine eigene Speicherverwaltung hat, nehme ich mal an, dass er aus Betriebssystemsicht noch reserviert ist, aber das meine ich nicht, sondern eben, ob er innerhalb des Programms noch belegt war.

Wenn dies nicht der Fall sein sollte, dann hätte ja eine Funktion, die freigegebenen Speicher überschreibt geholfen, den Bug zum PW Diebstahl auszunutzen.

Memcopy schrieb:

Wenn dies nicht der Fall sein sollte, dann hätte ja eine Funktion, die freigegebenen Speicher überschreibt geholfen,

Also eine Funktion, die beim freigeben den Speicher vorher überschreibt meine ich.

sarkasmusdetektor schrieb:

lokoklo schrieb:

https://github.com/openssl/openssl/commit/731f431497f463f3a2a97236fe0187b11c44aead

Also, defining constants, especially those with human-readable names, is a massive affront to performance! If another developer wants to know that "payload" actually describes the size of the buffer, they should go read the RFC.

Die ganzen Kommentare für den Patch kannst du getrost ignorieren, die sind von irgendwelchen unbeteiligten Personen, die lustig sein wollen.

Und man darf nicht vergessen, dass das der Codeteil relativ unwichtig ist, und deshalb wenig Beachtung findet.

Das war jetzt Ironie (hoffentlich).

hustbaer

waruminSSL schrieb:

Warum sollte man ein Keepalive in eine SSL Bibliothek einbauen? Was hat das mit Verschlüsselung zu tun? Ältere OpenSSL Versionen haben auch ohne funktioniert.

Darum geht's nicht. Ich behaupte nicht dass man Keepalives in TLS braucht, oder dass sie super nützlich sind.

Ich behaupte nur dass das Argument "braucht keiner, weil's das ja eh schon in TCP/IP gibt" Blödsinn ist.