Warum ist dieser Code anfällig für Pufferüberlaufangriffe?

148
int func(char* str)
{
   char buffer[100];
   unsigned short len = strlen(str);

   if(len >= 100)
   {
        return (-1);
   }

   strncpy(buffer,str,strlen(str));
   return 0;
}

Dieser Code ist anfällig für einen Pufferüberlaufangriff, und ich versuche herauszufinden, warum. Ich denke, es hat damit zu tun, lendass man als a shortstatt als deklariert wird int, aber ich bin mir nicht sicher.

Irgendwelche Ideen?

Jason
quelle
3
Es gibt mehrere Probleme mit diesem Code. Denken Sie daran, dass C-Zeichenfolgen nullterminiert sind.
Dmitri Chubarov
4
@DmitriChubarov, nicht null Beenden der Zeichenfolge ist nur dann ein Problem, wenn die Zeichenfolge nach dem Aufruf von verwendet wird strncpy. In diesem Fall ist es nicht.
R Sahu
43
Die Probleme in diesem Code ergeben sich direkt aus der Tatsache, dass er strlenberechnet, für die Gültigkeitsprüfung verwendet und dann wieder absurd berechnet wird - es ist ein DRY-Fehler. Wenn die zweite strlen(str)durch ersetzt lenwürde, gäbe es keine Möglichkeit eines Pufferüberlaufs, unabhängig von der Art der len. Die Antworten sprechen diesen Punkt nicht an, sie schaffen es nur, ihn zu vermeiden.
Jim Balter
3
@CiaPan: Wenn eine nicht nullterminierte Zeichenfolge übergeben wird, zeigt strlen ein undefiniertes Verhalten.
Kaiserludi
3
@ JimBalter Nein, ich denke ich werde sie dort lassen. Vielleicht hat jemand anderes das gleiche dumme Missverständnis und lernt daraus. Fühlen Sie sich frei, sie zu markieren, wenn sie Sie ärgern, jemand könnte vorbeikommen und sie löschen.
Asad Saeeduddin

Antworten:

192

Bei den meisten Compilern beträgt der Maximalwert von a unsigned short65535.

Jeder darüber liegende Wert wird umbrochen, sodass 65536 zu 0 und 65600 zu 65 wird.

Dies bedeutet, dass lange Zeichenfolgen mit der richtigen Länge (z. B. 65600) die Prüfung bestehen und den Puffer überlaufen.


Verwenden Sie size_tdiese Option , um das Ergebnis von strlen()nicht zu speichern unsigned shortund lenmit einem Ausdruck zu vergleichen , der die Größe von direkt codiert buffer. Also zum Beispiel:

char buffer[100];
size_t len = strlen(str);
if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;
memcpy(buffer, str, len + 1);
orlp
quelle
2
@PatrickRoberts Theoretisch ja. Beachten Sie jedoch, dass 10% des Codes für 90% der Laufzeit verantwortlich sind. Daher sollten Sie die Leistung nicht vor der Sicherheit beeinträchtigen. Und denken Sie daran, dass sich der Code im Laufe der Zeit ändert, was plötzlich bedeuten kann, dass die vorherige Prüfung weg ist.
Orlp
3
Verwenden Sie einfach lenals drittes Argument von strncpy, um einen Pufferüberlauf zu verhindern . Strlen wieder zu benutzen ist auf jeden Fall dumm.
Jim Balter
15
/ sizeof(buffer[0])- Beachten Sie, dass sizeof(char)in C immer 1 ist (auch wenn ein Zeichen eine Unmenge Bits enthält). Dies ist überflüssig, wenn keine Möglichkeit besteht, einen anderen Datentyp zu verwenden. Trotzdem ... ein großes Lob für eine vollständige Antwort (und danke, dass Sie auf Kommentare reagiert haben).
Jim Balter
3
@ rr-: char[]und char*sind nicht dasselbe. Es gibt viele Situationen, in denen a char[]implizit in a umgewandelt wird char*. Dies ist beispielsweise char[]genau das Gleiche wie char*bei Verwendung als Typ für Funktionsargumente. Die Konvertierung erfolgt jedoch nicht für sizeof().
Dietrich Epp
4
@Controll Denn wenn Sie die Größe von bufferirgendwann ändern, wird der Ausdruck automatisch aktualisiert. Dies ist für die Sicherheit von entscheidender Bedeutung, da die Deklaration von buffermöglicherweise einige Zeilen vom Einchecken des tatsächlichen Codes entfernt ist. Es ist also einfach, die Größe des Puffers zu ändern, aber vergessen Sie, an jedem Ort, an dem die Größe verwendet wird, zu aktualisieren.
Orlp
28

Das Problem ist hier:

strncpy(buffer,str,strlen(str));
                   ^^^^^^^^^^^

Wenn die Zeichenfolge größer als die Länge des Zielpuffers ist, kopiert strncpy sie weiterhin. Sie verwenden die Anzahl der Zeichen der Zeichenfolge als die zu kopierende Anzahl anstelle der Größe des Puffers. Der richtige Weg, dies zu tun, ist wie folgt:

strncpy(buffer,str, sizeof(buff) - 1);
buffer[sizeof(buff) - 1] = '\0';

Dadurch wird die Menge der kopierten Daten auf die tatsächliche Größe des Puffers minus eins für das Null-Abschlusszeichen begrenzt. Dann setzen wir das letzte Byte im Puffer als zusätzlichen Schutz auf das Nullzeichen. Der Grund dafür ist, dass strncpy bis zu n Bytes kopiert, einschließlich der abschließenden Null, wenn strlen (str) <len - 1. Wenn nicht, wird die Null nicht kopiert und Sie haben ein Absturzszenario, da Ihr Puffer jetzt nicht abgeschlossen ist Zeichenfolge.

Hoffe das hilft.

BEARBEITEN: Nach weiterer Prüfung und Eingabe durch andere folgt eine mögliche Codierung für die Funktion:

int func (char *str)
  {
    char buffer[100];
    unsigned short size = sizeof(buffer);
    unsigned short len = strlen(str);

    if (len > size - 1) return(-1);
    memcpy(buffer, str, len + 1);
    buffer[size - 1] = '\0';
    return(0);
  }

Da wir die Länge der Zeichenfolge bereits kennen, können wir memcpy verwenden, um die Zeichenfolge von der Position, auf die str verweist, in den Puffer zu kopieren. Beachten Sie, dass auf der Handbuchseite für strlen (3) (auf einem FreeBSD 9.3-System) Folgendes angegeben ist:

 The strlen() function returns the number of characters that precede the
 terminating NUL character.  The strnlen() function returns either the
 same result as strlen() or maxlen, whichever is smaller.

Was ich so interpretiere, dass die Länge der Zeichenfolge nicht die Null enthält. Aus diesem Grund kopiere ich len + 1 Bytes, um die Null einzuschließen, und der Test prüft, ob die Länge <Größe des Puffers - 2 ist. Minus eins, da der Puffer an Position 0 beginnt, und minus eins, um sicherzustellen, dass Platz vorhanden ist für die Null.

BEARBEITEN: Es stellt sich heraus, dass die Größe von etwas mit 1 beginnt, während der Zugriff mit 0 beginnt. Daher war die -2 vorher falsch, da sie einen Fehler für alles> 98 Bytes zurückgeben würde, aber> 99 Bytes sein sollte.

BEARBEITEN: Obwohl die Antwort auf eine vorzeichenlose Kurzform im Allgemeinen korrekt ist, da die maximal darstellbare Länge 65.535 Zeichen beträgt, spielt dies keine Rolle, da der Wert umbrochen wird, wenn die Zeichenfolge länger ist. Es ist, als würde man 75.231 (das ist 0x000125DF) nehmen und die oberen 16 Bits maskieren, was 9695 (0x000025DF) ergibt. Das einzige Problem, das ich dabei sehe, sind die ersten 100 Zeichen nach 65.535, da die Längenprüfung das Kopieren zulässt, aber in allen Fällen nur bis zu den ersten 100 Zeichen der Zeichenfolge kopiert und die Zeichenfolge mit Null beendet wird . Selbst mit dem Wraparound-Problem wird der Puffer immer noch nicht überlaufen.

Dies kann an sich ein Sicherheitsrisiko darstellen oder nicht, abhängig vom Inhalt der Zeichenfolge und dem, wofür Sie sie verwenden. Wenn es sich nur um geraden Text handelt, der von Menschen gelesen werden kann, gibt es im Allgemeinen kein Problem. Sie erhalten nur eine abgeschnittene Zeichenfolge. Wenn es sich jedoch um eine URL oder sogar eine SQL-Befehlssequenz handelt, liegt möglicherweise ein Problem vor.

Daniel Rudy
quelle
2
Stimmt, aber das geht über den Rahmen der Frage hinaus. Der Code zeigt deutlich, dass der Funktion ein Zeichenzeiger übergeben wird. Außerhalb des Funktionsumfangs ist uns das egal.
Daniel Rudy
"der Puffer, in dem str gespeichert ist" - das ist kein Pufferüberlauf , worum es hier geht. Und jede Antwort hat dieses "Problem", das angesichts der Signatur von func... und jeder anderen jemals geschriebenen C-Funktion, die NUL-terminierte Zeichenfolgen als Argumente verwendet , unvermeidbar ist . Es ist völlig ahnungslos, die Möglichkeit zu erwähnen, dass der Eingang nicht NUL-terminiert ist.
Jim Balter
"das geht über den Rahmen der Frage hinaus" - was leider für manche Menschen nicht nachvollziehbar ist.
Jim Balter
"Das Problem ist da" - Sie haben Recht, aber Sie vermissen immer noch das Hauptproblem: Der Test ( len >= 100) wurde für einen Wert durchgeführt, aber die Länge der Kopie wurde mit einem anderen Wert versehen ... dies ist eine Verletzung des DRY-Prinzips. Ein einfacher Aufruf strncpy(buffer, str, len)vermeidet die Möglichkeit eines Pufferüberlaufs und macht weniger Arbeit als strncpy(buffer,str,sizeof(buffer) - 1)... obwohl es hier nur ein langsameres Äquivalent zu ist memcpy(buffer, str, len).
Jim Balter
@ JimBalter Es geht über den Rahmen der Frage hinaus, aber ich schweife ab. Ich verstehe, dass die vom Test verwendeten Werte und die in strncpy verwendeten Werte zwei verschiedene sind. Die allgemeine Codierungspraxis besagt jedoch, dass das Kopierlimit sizeof (buffer) - 1 sein sollte, sodass es keine Rolle spielt, wie lang str auf der Kopie ist. strncpy beendet das Kopieren von Bytes, wenn es entweder eine Null trifft oder n Bytes kopiert. Die nächste Zeile garantiert, dass das letzte Byte im Puffer ein Nullzeichen ist. Der Code ist sicher, ich stehe zu meiner vorherigen Aussage.
Daniel Rudy
11

Auch wenn Sie verwenden strncpy, hängt die Länge des Cutoffs immer noch vom übergebenen Zeichenfolgenzeiger ab. Sie haben keine Ahnung, wie lang diese Zeichenfolge ist (dh die Position des Nullterminators relativ zum Zeiger). Wenn Sie strlenalleine anrufen, sind Sie anfällig für Sicherheitslücken. Wenn Sie sicherer sein möchten, verwenden Sie strnlen(str, 100).

Vollständiger Code korrigiert wäre:

int func(char *str) {
   char buffer[100];
   unsigned short len = strnlen(str, 100); // sizeof buffer

   if (len >= 100) {
     return -1;
   }

   strcpy(buffer, str); // this is safe since null terminator is less than 100th index
   return 0;
}
Patrick Roberts
quelle
@ user3386109 Würde dann auch nicht strlenüber das Ende des Puffers hinaus zugreifen?
Patrick Roberts
2
@ user3386109 Was Sie darauf hinweisen, macht die Antwort von orlp genauso ungültig wie meine. Ich verstehe strnlennicht, warum das Problem nicht gelöst wird, wenn das, was orlp vorschlägt, angeblich sowieso richtig ist.
Patrick Roberts
1
"Ich glaube nicht, dass strnlen hier irgendetwas löst" - natürlich; es verhindert ein Überlaufen buffer. "da str auf einen Puffer von 2 Bytes verweisen könnte, von denen keines NUL ist." - das ist irrelevant, da es für jede Implementierung von gilt func. Die Frage hier ist über Pufferüberlauf, nicht UB, weil der Eingang nicht NUL-terminiert ist.
Jim Balter
1
"Der zweite Parameter, der an strnlen übergeben wird, muss die Größe des Objekts sein, auf das der erste Parameter zeigt, oder strnlen ist wertlos" - dies ist vollständiger und völliger Unsinn. Wenn das zweite Argument für strnlen die Länge der Eingabezeichenfolge ist, entspricht strnlen strlen. Wie würden Sie diese Nummer überhaupt erhalten, und wenn Sie sie hätten, warum müssten Sie str [n] len anrufen? Dafür ist strnlen überhaupt nicht da.
Jim Balter
1
+1 Obwohl diese Antwort nicht perfekt sind , weil es mit dem Code des OP nicht gleichwertig ist - Strncpy NUL-Pads und nicht NUL beenden, während strcpy NUL-terminiert und nicht NUL-Pad, es tut das Problem, im Gegensatz zu der Lösung lächerliche, unwissende Kommentare oben.
Jim Balter
4

Die Antwort mit der Verpackung ist richtig. Aber es gibt ein Problem, von dem ich denke, dass es nicht erwähnt wurde, wenn (len> = 100)

Nun, wenn Len 100 wäre, würden wir 100 Elemente kopieren und hätten kein nachfolgendes \ 0. Dies würde eindeutig bedeuten, dass jede andere Funktion in Abhängigkeit von der richtigen Endzeichenfolge weit über das ursprüngliche Array hinausgehen würde.

Die Zeichenfolge problematisch von C ist meiner Meinung nach unlösbar. Sie sollten immer einige Grenzen vor dem Anruf haben, aber selbst das wird nicht helfen. Es gibt keine Überprüfung der Grenzen und so können und werden Pufferüberläufe immer passieren ....

Friedrich
quelle
Das String-Problem ist lösbar: Verwenden Sie einfach die entsprechenden Funktionen. I. e. nicht strncpy() und Freunde, aber die Speicherzuweisung funktioniert wie strdup()und Freunde. Sie sind im POSIX-2008-Standard enthalten, daher sind sie ziemlich portabel, obwohl sie auf einigen proprietären Systemen nicht verfügbar sind.
cmaster - wieder einsetzen Monica
"Jede andere Funktion, die von der richtigen Endzeichenfolge abhängt" - bufferist lokal für diese Funktion und wird an keiner anderen Stelle verwendet. In einem realen Programm müssten wir untersuchen, wie es verwendet wird ... manchmal ist eine NUL-Terminierung nicht korrekt (die ursprüngliche Verwendung von strncpy bestand darin, die 14-Byte-Verzeichniseinträge von UNIX zu erstellen - NUL-aufgefüllt und nicht NUL-terminiert). "Die von C problematische Zeichenfolge ist meiner Meinung nach unlösbar" - während C eine krasse Sprache ist, die von einer weitaus besseren Technologie übertroffen wurde, kann sicherer Code darin geschrieben werden, wenn genügend Disziplin angewendet wird.
Jim Balter
Ihre Beobachtung scheint mir falsch. if (len >= 100)ist die Bedingung, wenn die Prüfung fehlschlägt , nicht wenn sie bestanden wird, was bedeutet, dass es keinen Fall gibt, in dem genau 100 Bytes ohne NUL-Terminator kopiert werden, da diese Länge in der Fehlerbedingung enthalten ist.
Patrick Roberts
@ cmaster. In diesem Fall liegen Sie falsch. Es ist nicht lösbar, weil man immer über die Grenzen hinaus schreiben kann. Ja, es ist ein unversehrtes Verhalten, aber es gibt keine Möglichkeit, es vollständig zu verhindern.
Friedrich
@ Jim Balter. Das ist egal. Ich kann möglicherweise über die Grenzen dieses lokalen Puffers schreiben und es wird immer möglich sein, einige andere Datenstrukturen zu beschädigen.
Friedrich
3

Abgesehen von den Sicherheitsproblemen, die mit dem mehrmaligen Aufrufen verbunden strlensind, sollte man im Allgemeinen keine String-Methoden für Strings verwenden, deren Länge genau bekannt ist [für die meisten String-Funktionen gibt es nur einen wirklich engen Fall, in dem sie verwendet werden sollten - für Strings, für die maximal Länge kann garantiert werden, aber die genaue Länge ist nicht bekannt]. Sobald die Länge der Eingabezeichenfolge bekannt ist und die Länge des Ausgabepuffers bekannt ist, sollte man herausfinden, wie groß eine Region kopiert werden soll, und dann verwenden memcpy(), um die betreffende Kopie tatsächlich durchzuführen. Obwohl es möglich ist, dass beim Kopieren einer Zeichenfolge mit nur etwa 1 bis 3 Byte eine strcpyOutperformance memcpy()erzielt wird, ist dies auf vielen Plattformen memcpy()bei größeren Zeichenfolgen wahrscheinlich mehr als doppelt so schnell.

Obwohl es einige Situationen gibt, in denen Sicherheit zu Lasten der Leistung gehen würde, ist dies eine Situation, in der der sichere Ansatz auch der schnellere ist. In einigen Fällen kann es sinnvoll sein, Code zu schreiben, der nicht gegen Eingaben mit seltsamem Verhalten gesichert ist, wenn der Code, der die Eingaben liefert, sicherstellen kann, dass sie sich gut verhalten, und wenn der Schutz vor Eingaben mit schlechtem Verhalten die Leistung beeinträchtigen würde. Wenn Sie sicherstellen, dass die Zeichenfolgenlängen nur einmal überprüft werden, verbessern Sie sowohl die Leistung als auch die Sicherheit. Sie können jedoch eine zusätzliche Maßnahme ergreifen, um die Sicherheit auch bei manueller Verfolgung der Zeichenfolgenlänge zu gewährleisten: Schreiben Sie für jede Zeichenfolge, für die eine nachfolgende Null erwartet wird, die nachfolgende Null explizit als zu erwarten, dass die Quellzeichenfolge es hat. Wenn man also ein strdupÄquivalent schreibt :

char *strdupe(char const *src)
{
  size_t len = strlen(src);
  char *dest = malloc(len+1);
  // Calculation can't wrap if string is in valid-size memory block
  if (!dest) return (OUT_OF_MEMORY(),(char*)0); 
  // OUT_OF_MEMORY is expected to halt; the return guards if it doesn't
  memcpy(dest, src, len);      
  dest[len]=0;
  return dest;
}

Beachten Sie, dass die letzte Anweisung im Allgemeinen weggelassen werden kann, wenn der Memcpy len+1Bytes verarbeitet hat. Wenn jedoch ein anderer Thread die Quellzeichenfolge ändert, kann das Ergebnis eine nicht NUL-terminierte Zielzeichenfolge sein.

Superkatze
quelle
3
Können Sie bitte die Sicherheitsproblemestrlen erklären, die mit mehr als einmaligen Anrufen verbunden sind ?
Bogdan Alexandru
1
@BogdanAlexandru: Sobald jemand angerufen strlenund eine Aktion basierend auf dem zurückgegebenen Wert ausgeführt hat (was vermutlich der Grund für den Aufruf war), liefert ein wiederholter Aufruf entweder (1) immer die gleiche Antwort wie der erste. In diesem Fall ist es einfach verschwendete Arbeit, oder (2) kann manchmal (weil etwas anderes - vielleicht ein anderer Thread - die Zeichenfolge in der Zwischenzeit geändert hat) eine andere Antwort ergeben. In diesem Fall Code, der einige Dinge mit der Länge macht (z Zuweisen eines Puffers) kann eine andere Größe annehmen als Code, der andere Aufgaben ausführt (Kopieren in den Puffer).
Supercat