OpenSSL - Heartbleed-Bug

TyRoXx

Der OpenSSL-Code liest sich als wäre er aus optimiertem Assembly dekompiliert worden. Das beweist wohl, dass dort Experten am Werk sind und deutet auf gute Performance hin.

Sinnvolle Benennung ist total wichtig in einer 289 Zeilen langen Funktion.

static int ssl3_get_record(SSL *s)
	{
	int ssl_major,ssl_minor,al;
	int enc_err,n,i,ret= -1;
	SSL3_RECORD *rr;
	SSL_SESSION *sess;
	unsigned char *p;
	unsigned char md[EVP_MAX_MD_SIZE];
	short version;
	unsigned mac_size;
	size_t extra;
	unsigned empty_record_count = 0;

	rr= &(s->s3->rrec);
	sess=s->session;

Das Benutzen des Präprozessors und printf zum Debuggen zeugt von "besonderer" Qualität.

#if 0
fprintf(stderr, "Record type=%d, Length=%d\n", rr->type, rr->length);
#endif

#ifdef TLS_DEBUG
printf("dec %d\n",rr->length);
{ unsigned int z; for (z=0; z<rr->length; z++) printf("%02X%c",rr->data[z],((z+1)%16)?' ':'\n'); }
printf("\n");
#endif

#if 1
		clear=s->enc_write_ctx?0:1;	/* must be AEAD cipher */
#else
		clear=1;
#endif

Der schnellste Code ist der, der nicht verwendet wird:

#if 0 && !defined(OPENSSL_NO_MULTIBLOCK) && EVP_CIPH_FLAG_TLS1_1_MULTIBLOCK

Kommentare erläutern wichtige Design-Entscheidungen.

for (i=7; i>=0; i--)	/* increment */
					{
					++seq[i];
					if (seq[i] != 0) break; 
					}
				}

Also ich muss schon sagen: Mit Heartbleed hätte niemand rechnen können! So etwas wird sicher nie wieder vorkommen.

Erhard Henkes

http://www.faz.net/aktuell/wirtschaft/netzwirtschaft/nsa-soll-heartbleed-luecke-seit-zwei-jahren-systematisch-ausgenutzt-haben-12892263.html

TyRoXx schrieb:

Der OpenSSL-Code liest sich als wäre er aus optimiertem Assembly dekompiliert worden. Das beweist wohl, dass dort Experten am Werk sind und deutet auf gute Performance hin.

...

Also ich muss schon sagen: Mit Heartbleed hätte niemand rechnen können! So etwas wird sicher nie wieder vorkommen.

Ich wette 10 € dass der Code von Windows XP besser aussieht.

es wird endlich zeit für eine implementierung in c++. alle c libs sind vuln

cya schrieb:

es wird endlich zeit für eine implementierung in c++. alle c libs sind vuln

dabei könnte man auch in C sauber programmieren.
Man muss sich halt mal die Mühe machen und für Datentypen (Strukturen) gewisse Funktionen zurechtzulegen.
Irgendwo im Code ein memcpy ist halt potentiell gefährlich.

Aber, um Euch zu beruhigen: wir entwickeln hier mit C++ und es gibt Kollegen, die verwenden lieber memcpy und diese Dinge, als mal in einen Copy CTOR zu investieren.
Und natürlich gibts an solchen Stellen dauernd Fehler. Selbst Profis machen bei Zeigern, dyn. Speicher und byteweisen Herumkopieren von Daten von Zeit zu Zeit Fehler.
Das einzige was dagegen hilft, ist es, solche Dinge möglichst zu vermeiden.
Und wenn sie sich nicht vermeiden lassen, dann zumindest kapseln.

den Fix in Form von if (1 + 2 + payload + 16 > s->s3->rrec.length) find ich übrigens einen schlechten Scherz.

gfdgfdgdfgdfgd schrieb:

den Fix in Form von if (1 + 2 + payload + 16 > s->s3->rrec.length) find ich übrigens einen schlechten Scherz.

Begründung?

schlechter scherz schrieb:

gfdgfdgdfgdfgd schrieb:

den Fix in Form von if (1 + 2 + payload + 16 > s->s3->rrec.length) find ich übrigens einen schlechten Scherz.

Begründung?

1, 2 und 16 steht für was?
Warum keine aussagekräftigen Namen?
Und warum dann nicht als Konstanten?

Warum keine Klammerung zwecks mehr Übersichtlichkeit, z.B. so:

if ((1 + 2 + payload + 16) > (s->s3->rrec.length))

Fazit:
Dahingeschluderter Müllcode.