[C] Sichere Char-Arrays - Wie?

[MikeG]

Hallo,

ich programmiere nun schon ein wenig hier und da. Hab mir mittlerweile auch Gedanken über Sicherheit in meinen Programmen gemacht.

Folgendes Szenarion:

Ich nehme per recv() einen Char-Array an:

int size_Buffer = 0;
char Incomming_Buffer[4096] = "\0";
char *Temp_Buffer;

size_Buffer = recv(Client, Incomming_Buffer, sizeof(Incomming_Buffer),0);

Temp_Buffer = strtok(Incomming_Buffer,"\n");
printf("\n\nLines:\n%s\n",Temp_Buffer);
while( Temp_Buffer = strtok(NULL,"\n"))
     printf("%s\n",Temp_Buffer);

Wie kann ich nun sicher stellen, ob in Incomming_Buffer nicht irgendwelcher Code drin steht? Reicht es, wenn ich das letzte Byte von Incomming_Buffer auf \0 setze? Meines Erachtens ja, denn dann kann der Puffer ja nicht überlaufen.

Vor ähnlichen Problemen stehe ich bei der Übergabe von Parametern an ein Programm. Mich interessiert generell, wie ich meine "Strings" sicher mache.

Vielleicht regt das ja auch den ein oder andern hier im Forum zum Nachdenken an, seine Codes sicherer zu machen.

Bei mir besteht da auf jeden Fall Lern-Bedarf.

Dankend
Mike

[wraith]

Wie kann ich nun sicher stellen, ob in Incomming_Buffer nicht irgendwelcher Code drin steht? Reicht es, wenn ich das letzte Byte von Incomming_Buffer auf \0 setze? Meines Erachtens ja, denn dann kann der Puffer ja nicht überlaufen.

Na "irgendwelcher Code" wird da auch weiterhin drinstehen, da ändert es auch nichts, das letzte Byte auf 0 zu setzen.
Darüberhinaus tust du das nicht.Annahme dein recv Aufruf empfängt sizeof Incoming_Buffer viele Zeichen, dann ist das Array voll, und nicht Null-terminiert.
Gut, aber du hast ja den Rückgabewert von recv, der dir die Anzahl der gelesenen Zeichen angibt.Benutze diesen, um dein Array korrekt zu terminieren.

Vor ähnlichen Problemen stehe ich bei der Übergabe von Parametern an ein Programm. Mich interessiert generell, wie ich meine "Strings" sicher mache.

Wichtigste Grundregel (nicht nur für String-Operationen): Jeden Rückgabewert testen, das machst du in deinem Bsp. an zwei Stellen nicht (printf lassen wir mal außen vor, aber auch Ausgabeoperationen können fehlschlagen).
Production-Code kommt nicht darum jeden Rückgabewert zu testen.

Zu String-Operationen speziell: Es gibt in der Standardlib viele inherent unsichere Funktionen (zb. gets, wenn man es nicht gerade unter Dos benutzt ;-) ), und viele bei denen man noch Extraarbeit leisten muß (zb. scanf), damit sie sicher sind.
Für viele Funktionen gibt es neue Varianten (zu erkennen am eingebetteten 'n' zb. aus strcat -> strncat), denen man die Größe des Buffers mitübergeben muß, um so Bufferoverflows zu vermeiden.
Aber auch hier sind einige schon vom Design her mit Fehlern behaftet, weshalb es spezielle Libs extra dafür gibt (zb. die BSD Stringlib).

Ok. und damit das auch noch gesagt ist: Benutze C++ und std::string ;-).

[MikeG]

Erstmal Danke für die kompetente Antwort.

Meinst Du, das ich evtl. an dieser Stelle dann mit dynamischen Größen arbeiten und mit realloc dann vergrößern/verkleinern? Wenn ich das im Kopf durch spiele, könnte es mehr Sicherheit bringen.

Damit wir uns richtig verstehen. Shellcode bspw. der durch einen Buffer Overflow zum ausführen gebracht wird, kann in einem dynamischen Puffer nicht zum ausführen gebracht werden, da der Puffer dann ja nicht überlaufen kann.

Statt

char Incomming_Buffer[4096]

ein

char *Incomming_Buffer;

und ein

Incomming_Buffer = realloc(Incomming_Buffer, size_Buffer);

könnte damit das Problem beseitigen, oder?

Du schreibst, das ich an 2 Stellen nicht prüfe. Die erste ist klar, aber wo ist die zweite?

Gruß
Mike

PS: Ich kenne mich damit noch nicht so gut aus, ich komme von Pascal/Delphi, da hatte ich mit sowas keinen Kontakt.

[wraith]

Meinst Du, das ich evtl. an dieser Stelle dann mit dynamischen Größen arbeiten und mit realloc dann vergrößern/verkleinern? Wenn ich das im Kopf durch spiele, könnte es mehr Sicherheit bringen.

Erstmal bringt es dir mehr Verwaltungsoverhead und potentielle Fehlerquellen (Rückgabewert von realloc testen, immer die aktuelle Größe mitführen, beim vergrößern auf Platz für Stringende achten).
Am Ende bringt dir das eher mehr Unsicherheit.

Wenn du ein Problem mit einem statischen Speicher lösen kannst, dann bleib dabei.
Was für deine schlussendliche Lösung wichtiger ist; brauchst du die Resultate von verschiedenen recv-Aufrufen in einem zusammenhängenden Speicherbereich, und du weist vorher nicht wie groß dieser ist?
Dann bleibt dir nur die realloc Variante.

Damit wir uns richtig verstehen. Shellcode bspw. der durch einen Buffer Overflow zum ausführen gebracht wird, kann in einem dynamischen Puffer nicht zum ausführen gebracht werden, da der Puffer dann ja nicht überlaufen kann.

siehe oben; beide können überlaufen, wenn du Fehler machst.
In deinem ersten Codebsp. läuft Incoming_Buffer beim recv-Aufruf auch nicht über, nur ist dieser Buffer möglicherweise nicht null-terminiert, aber das kannst du schnell beheben.

Du schreibst, das ich an 2 Stellen nicht prüfe. Die erste ist klar, aber wo ist die zweite?

Der erste strtok-Aufruf, der kann auch bereits fehlschlagen, dann gibt er NULL zurück.

[MikeG]

Vielen herzlichen Dank! Das hat mich sehr weit gebracht.

Was hältst nun von diesem?

char *Incoming_Buffer, *Temp_Buffer;
int size_Buffer;

Incoming_Buffer = malloc(4096);
bzero(Incoming_Buffer,sizeof(Incoming_Buffer));
Temp_Buffer = malloc(1024);
bzero(Temp_Buffer,sizeof(Temp_Buffer));

[...]

size_Buffer = recv(Client, Incoming_Buffer, 4096, 0);
Incoming_Buffer = realloc(Incoming_Buffer, size_Buffer+1);
Incoming_Buffer[size_Buffer+1] = (char)0;

Temp_Buffer = strtok(Incoming_Buffer,"\n");
if(Temp_Buffer != NULL)
{
	printf("\n\nHTTP-Fields:\n%s\n",Temp_Buffer);
	while( (Temp_Buffer = strtok(NULL,"\n")) != NULL)
		printf("%s\n",Temp_Buffer);
}
printf("Received bytes: %d\n",size_Buffer);

=== Edited ===

Ich seh grad, das Temp_Buffer nach strtok nicht mehr Überlauf und korrektes Ende geprüft wird. Gibt es eine elegante Möglichkeit? strtok gibt keinen Wert der Länge des Arrays zurück. sizeof() berechnet auch nur bis zur maximalen Größe des Puffers, nicht bis zum tatsächlichen Ende (\0). Was kann man da machen, außer eine feste Größe von Temp_Buffer?

===/Edited ===

Ich habe außerdem ein kleines Problem dabei gefunden. Kann es sein, das sizeof() bei dynamischen Arrays einen falschen Wert berechnet? Zumindest ist das im MingW der Fall, wenn man sizeof(Incoming_Buffer) als int buflen an recv() übergibt.

Hat da jemand ähnliche Erfahrungen gemacht?

Viele Grüße
Mike

[wraith]

size_Buffer = recv(Client, Incoming_Buffer, 4096, 0);
Incoming_Buffer = realloc(Incoming_Buffer, size_Buffer+1);
Incoming_Buffer[size_Buffer+1] = (char)0;

Hiermit willst du den Buffer auf die benötige Größe verkleinern, oder?Weil ein Wert größer 4096 wird nicht zurückgegeben (oder -1 bei Fehler).

Weiterhin ist die letzte Zuweisung falsch, wenn du eine Array auf size_Buffer + 1 Anzahl Wert vergrößerst, dann sind gültige Indices 0...size_Buffer.Dh. du bist schon einen drüber.

Achja, warum wird für Temp_Buffer Speicher angefordert?Wenn ich das nur auf den geposteten Code beziehe, dann ist es unnötig (falsch), weil beim ersten strtok die Adresse überschrieben wird, und damit der Speicher weg ist.

Ich habe außerdem ein kleines Problem dabei gefunden. Kann es sein, das sizeof() bei dynamischen Arrays einen falschen Wert berechnet?

sizeof wird zur Compilezeit ausgewertet (mit einer kleinen Ausnahme, die hier nicht greift).Dh. sizeof kann nicht wissen, wie großer der Speicher ist, auf die der Zeiger zeigt, folglich gibt dir sizeof die Größe des Typs (hier: Zeiger auf char) zurück, was bei deinem System wahrschleich 4 ist.
Du kommst bei dynamischer Speicherverwaltung also nicht drumrum die Größe in einer Extravariablen mitzuführen.

[MikeG]

Hiermit willst du den Buffer auf die benötige Größe verkleinern, oder?Weil ein Wert größer 4096 wird nicht zurückgegeben (oder -1 bei Fehler).

Danke, das würde ja im extremen Fall zu einer Fehl-Funktion von realloc() führen, daher sollte ich size_Buffer auf jeden Fall auch prüfen (>0)

Weiterhin ist die letzte Zuweisung falsch, wenn du eine Array auf size_Buffer + 1 Anzahl Wert vergrößerst, dann sind gültige Indices 0...size_Buffer.Dh. du bist schon einen drüber.

Darum hab ich realloc() mit size_Buffer+1 durchgeführt und kann somit auch Index size_Buffer+1 mit \0 füllen. Oder irre ich?

sizeof wird zur Compilezeit ausgewertet (mit einer kleinen Ausnahme, die hier nicht greift).Dh. sizeof kann nicht wissen, wie großer der Speicher ist, auf die der Zeiger zeigt, folglich gibt dir sizeof die Größe des Typs (hier: Zeiger auf char) zurück, was bei deinem System wahrschleich 4 ist.
Du kommst bei dynamischer Speicherverwaltung also nicht drumrum die Größe in einer Extravariablen mitzuführen.

Das erklärt vieles. Und ist damit eine richtige Lücke im Code. 4 ist/war übrigens korrekt.

Ich danke für die Geduld und meine trotteligen Versuche. Ich denke, das ich dadurch sehr viel über sicheren Code lerne und diese Fehler nicht mehr machen werde. Ich probiere es auf jeden Fall weiter. Ich danke Dir sehr, gut das es Leute wie Dich gibt. Manch anderer hätte mich an dieser Stelle sicher schon ausgelacht.

Grüße
Mike

[wraith]

Darum hab ich realloc() mit size_Buffer+1 durchgeführt und kann somit auch Index size_Buffer+1 mit \0 füllen. Oder irre ich?

Wenn du ein Array mit 10 Elementen hast, dann kannst du auf die Indices 0 ... 9 zugreifen (kurz nachzählen, das sind genau die 10 Elemente).
Wenn du ein Array mit size_buffer + 1 Anzahl Elemente hast, dann kannst du auf die Indices 0 ... size_Buffer zugreifen.
size_Buffer + 1 ist also der Index des size_Buffer + 2. Elementes, das du gar nicht hast .

[MikeG]

Du hast natürlich absolut recht. Ich hatte irgendwie einen Denkfehler drin. Das heißt, ich hab das immer falsch gemacht. Du hast mir hier echt viel bei gebracht.

Danke!

[nobody0]

Allgemein sollte man sich an den Grundsatz halten alles zu limitieren, also
fgets statt fget
calloc statt malloc
strncat statt strcat
strncpy statt strcpy
snprintf statt sprintf
strncmp statt strcmp
vsnprintf statt vprintf
. Dies ist auch für sichere Programme wichtig, denn die Nichtverwendung
dieser limitierenden Funktionen ist einer der Hauptgründe für exploits.
Zudem sollten Eingaben immer mit Längenbegrenzung erfolgen, also
beispielsweise mittels scanf Strings mit dem Specifier %<length>s einlesen
und snprintf immer mit 4 Parametern, also Längenbegrenzung, verwenden.