C statische Codeanalyse

[king-crash]

Hallo,

Clang hat es schon eine Weile, gcc seit Version 10 (aktuell ist 11), die statische Codeanalyse. Bei clang mittels "--analyze" und bei gcc mit "-fanalyze".
Die Idee dahinter liest sich sehr gut, also wollte ich es gleich einmal ausprobieren:

Code: Alles auswählen

#include <stdlib.h>

int main(void)
{
	int *a = malloc(sizeof(int)*2);
	if(a != NULL)
	{
		a[100] = 1;
		free(a);
	}
	return 0;
}

Ein Bufferoverflow wie er nahezu nicht offensichtlicher sein könnte. Weder clang, noch gcc, noch splint erkennen das Problem. Ich bin unterwältigt...
Gibt es statische Codeanalysatoren, die diesen Fehler erkennen?
Was sind eure Erfahrungen mit dem Thema?
Ich benutze seither wenn möglich Valgrind, was diesen Fehler zur Laufzeit findet.

[wanne]

Out of bound checks sind alpha. Aber wenn man es anschaltet findet ers:

Code: Alles auswählen

scan-build -enable-checker alpha.security.ArrayBoundV2 clang bla.c
scan-build: Using '/usr/lib/llvm-11/bin/clang' for static analysis
bla.c:8:10: warning: Out of bound memory access (access exceeds upper limit of memory block) [alpha.security.ArrayBoundV2]
                a[100] = 1;
                ~~~~~~~^~~
1 warning generated.
scan-build: Analysis run complete.
scan-build: 1 bug found.
scan-build: Run 'scan-view /tmp/scan-build-2021-04-28-152556-156105-1' to examine bug reports.

Btw: Kein buffer overflow. Da wird ja nichts kopiert. Die wird man auch nie über statische Codeanalyse finden können. Dafür gibt es eventuell protection Mechanismen (also automatisiertes entfernen zur laufzeit) aber sicher kein zuverlässiges finden zur compiletime. Als Dummbeispiel: kopiere ich in einem modernen Linux ein memmapped /dev/mem in voller Größe in einen 1MiB großen Buffer ist das auf modernen Systemen legitim. Auf älteren nicht. Kein Compiler der Welt kann wissen ob das ein valides Programm ist, weil es auf den Einsatzzweck ankommt. Du kannst das jetzt z.b. duch einen test ob /dev/mem kleiner als 1MiB ist verhindern. Es ist aber explizites ziel von C Code schreiben zu können der eben nur auf manchen Systemen funktioniert und unnötige checks sein lassen zu können.

Edit: Der alpha.security.ArrayBound findets auch.

[king-crash]

Ah, vielen Dank für die Info. Gibt es vergleichbare Features auch in gcc oder anderen Analysatoren?

Für einen Pufferüberlauf muss nichts kopiert werden. Es ist ausreichend wenn vor oder hinter einem Puffer geschrieben werden kann.

... wodurch nach dem Ziel-Speicherbereich liegende Speicherstellen überschrieben werden.

Kannst du ein kurzes Beispiel geben, was du als Overflow bezeichnen würdest?

[wanne]

In der wikipedia steht da "Im wesentlichen" vorne dran.
Die haben auch ein schönes Beispiel:
gets() hat die geniale Eigenschaft immer ein potentieller buffer overflow zu sein. Dummerweise haben sie es in den C Standard geschrieben. Siehe auch:

Never use this function.

Ne andere sehr üblichere Variante, die man bis heute oft sieht ist das:

Code: Alles auswählen

char* buff = malloc(sizeof(char)*256);
char* var = getenv("var");
if(var)
  strcpy(buff,var);

Du kannst gegen sowas immer strncpy nutzen. Oder halt dynamisch die größe deines puffers anpassen
oder

Code: Alles auswählen

char *a = argv[1];
int num = atoi(a);
char buffer [20];
sprintf(buffer, "You have entered %d",num);

Kurz du hast ein copy von einer Datentyp (meist char*) von dem du nicht weißt wie groß er ist in einen Speicherbereich mit fester Größe ohne zu checken ob das Ziel größer als die Quelle ist.

[king-crash]

Ja, gets kopiert Daten ohne Rücksicht auf die Puffergröße, in diesem Fall ist dabei allerdings nur der Schreibvorgang das Problem.
Deshalb meinte ich ja, dass es für einen Pufferüberlauf keines Lesens oder Kopierens bedarf.

Ich kann deinem mmap Beispiel nicht ganz folgen. Ich habe mmap schon einige Male verwendet, aus dem Kopf ungefähr so:

Code: Alles auswählen

int fd = open("/dev/mem", O_RDWR);
int size = GROESSE_DES_SPEICHERBEREICHS;
char *m = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, STARTADRESSE_DES_SPEICHERBEREICHS);
m[size-1] = 1; // OK
m[size] = 1; // Overflow

Auch in diesem Fall ist die Größe des Bereichs die ganze Zeit klar und könnte von einen Codeanalysator überprüft werden.

[wanne]

Wie gesagt einfache Array out of Bounds sind IMHO keine buffer overflows. Du wirst auch nie ein Beispiel finden wo der Zugriff in fester Länge auf eine bekannte Adresse geschieht.

[king-crash]

Bei deinen zwei Beispielen kann es eine Codeanalyse tatsächlich nicht wissen, da der Fehler nur abhängig von der Größe des externen Strings auftritt. Hier wäre es dann aber generell sinnvoll eine Warnung auszugeben, dass ein Speicherbereich unbekannter Größe ohne Überprüfung kopiert werden soll.
In meinem Ausgangsbeispiel oder dem mmap-Beispiel sind die Größen der Puffer allerdings bekannt und könnten deshalb auch auf Überlauf getestet.

Wie gesagt einfache Array out of Bounds sind IMHO keine buffer overflows.

Ich habe zur genauen Begrifflichkeit nichts gefunden. Es macht jedenfalls für mich keinen Unterschied, ob ein Array auf dem Stack oder auf dem Heap "überlaufen" wird.
Da Arrays auf dem Stack zu Kompilierzeit exakt bekannt sein müssen ist es für einen Analyzer natürlich potentiell einfacher hier zu überprüfen (wobei der Index nicht notwendigerweise bekannt ist). Letztendlich kann jeder Puffer als Array von Bytes gesehen werden. Aber ich habe dich verstanden, dein Standpunkt ist OOB -> Stack, OV -> Heap.

Du wirst auch nie ein Beispiel finden wo der Zugriff in fester Länge auf eine bekannte Adresse geschieht.

Das kann durchaus vorkommen, wenn beispielsweise für eine Http Statusseite verschiedene Strings aneinander gehängt werden sollen.

[king-crash]

Ok die Analysatoren scheinen nur sehr rudimentär zu funktionieren, ich dachte die wären da bereits weiter. Dieses Programm überschreibt einen Puffer bei 2 oder mehr Kommandozeilenargumenten:

Code: Alles auswählen

#include <stdlib.h>

int main(int argc, char **argv)
	{
	int *a = malloc(sizeof(int)*2);
	if(a != NULL)
		{
		if(argc < 50)
			{
			a[argc] = 2;
			}
		//a[1000000] = 1;
		free(a);
		}
	return 0;
	}

Sobald eine unbekannte Größe hinzukommt die zwar getestet wird aber gegen eine falsche Größe, schlägt der Test nicht mehr an.
Mir ging es vor allen Dingen darum einordnen zu können was im Moment möglich ist und was nicht.
Besten Dank wanne.

[JTH]

Ich habe nicht mehr so ganz den Überblick, was die einzelnen clang- und andere Werkzeuge dir da liefern können. Aber z.B. cppcheck würde dir hier schon was bemängeln:

Zum Schnipsel aus dem ersten Beitrag:

Code: Alles auswählen

/tmp/tmp.abkZkn778Z$ cppcheck --enable=all a.cpp 
Checking a.cpp ...
a.cpp:8:4: error: Array 'a[2]' accessed at index 100, which is out of bounds. [arrayIndexOutOfBounds]
  a[100] = 1;
   ^

Zu dem hier drüber:

Code: Alles auswählen

/tmp/tmp.abkZkn778Z$ cppcheck --enable=all c.cpp
Checking c.cpp ...
c.cpp:10:5: warning: Either the condition 'argc<50' is redundant or the array 'a[2]' is accessed at index 49, which is out of bounds. [arrayIndexOutOfBoundsCond]
   a[argc] = 2;
    ^
c.cpp:8:11: note: Assuming that condition 'argc<50' is not redundant
  if(argc < 50)
          ^
c.cpp:10:5: note: Array index out of bounds
   a[argc] = 2;
    ^

[wanne]

aber ich habe dich verstanden, dein Standpunkt ist OOB -> Stack, OV -> Heap.

Nein. Wie gesagt der relevante Teil ist, ob du es mit Strukturen fester oder variabler Größe als Quelle zu tun hast.

cppcheck sieht das btw. wie ich. (Kannte ich btw. nicht. Danke JTH.) Bei deinem Beispiel sagt er:
arrayIndexOutOfBounds
Bei meinem beispiel mit dem sprintf und strcpy meint der dagegen:
bufferAccessOutOfBounds
Aber eben nur unter der Abwandlung, dass man das int/den String nicht aus der Umgebung bzw. von stdin holt sondern hart ins Programm einprogrammiert. Das dürfte selten für echte Fehler der Fall sein. Man braucht btw. auch die Eingabe das aber nicht sondern kann auch schlicht anders verschleiern, wie groß der Datentyp ist.
beim gets: hat er sogar ne eigene Warnung:
getsCalled

[wanne]

Hier das Ende von ccpcheck
Das erkennt er:

Code: Alles auswählen

int main(void)
{
  int num = 238947;
  char buffer [20];
  sprintf(buffer, "You have entered %d",num);
  return 0;
}

Das nimmer:

Code: Alles auswählen

int main(void)
{
  char *a = "238947";
  int num = atoi(a);
  char buffer [20];
  sprintf(buffer, "You have entered %d",num);
  return 0;
}

Ist für statische Codeanalyse dann auch echt schwer. Die Größe von atoi ist nicht absehbar. Erst wenn du das Programm im Kopf ausführst ist absehbar, wie groß das ist.

Das gcc warnt btw. vor sowas, wenn man ihm -Wformat-overflow=2 mitgibt. Das warnt dann aber halt auch vor dem

Code: Alles auswählen

int main(void)
{
  char *a = "2";
  int num = atoi(a);
  char buffer [20];
  sprintf(buffer, "You have entered %d",num);
  return 0;
}

-Wformat-overflow=2 und -Wstringop-overflow=3 geben im allgemeinen nen Haufn gute Warnungen für bginner. Aber halt auch einen Haufen false positives, weshalb sie im -Wall nicht drin sind.

[king-crash]

Danke für den Tipp, ich hatte noch eine alte Version von cppcheck. Die aus bullseye scheint besser zu sein.
cppcheck erkennt dafür das hier nicht:

Code: Alles auswählen

int b(int i)
	{
	char array[] = "abc";
	return array[i];
	}

void a(void)
	{
	b(10);
	}

Was dafür clang erkennt. Hm eventuell muss man mehrere Checker zum testen verwenden.
Ich hoffe, dass sich da in nächster Zeit noch was tut.

[JTH]

Code: Alles auswählen

  char buffer [20];
  sprintf(buffer, "You have entered %d",num);

Ein sicherheitsbewusster Entwickler würde an der Stelle natürlich sofort und reflexartig zu s*n*printf(buffer, 20, …) greifen Die Variante(n) ohne n ließe(n) sich, wenn man wollte, ja sogar mit grep „statisch analysieren“.

[wanne]

Die Variante(n) ohne n ließe(n) sich, wenn man wollte, ja sogar mit grep „statisch analysieren“.

Defakto ist das in minimal intelligenter das was das gcc mit den passenden Warn-Optionen macht. – Nicht dumm.