Dlaczego ten kod jest podatny na ataki polegające na przepełnieniu bufora?

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;
}

Ten kod jest podatny na atak przepełnienia bufora i próbuję dowiedzieć się, dlaczego. Myślę, że ma to związek z lenotrzymaniem deklaracji shortzamiast an int, ale nie jestem pewien.

Jakieś pomysły?

Jason
źródło
3
Jest wiele problemów z tym kodem. Przypomnij sobie, że łańcuchy C są zakończone znakiem null.
Dmitri Chubarov
4
@DmitriChubarov, brak wartości null przerywanie ciągu będzie problemem tylko wtedy, gdy ciąg zostanie użyty po wywołaniu funkcji strncpy. W tym przypadku tak nie jest.
R Sahu
43
Problemy w tym kodzie wynikają bezpośrednio z faktu, że strlenjest obliczany, używany do sprawdzania poprawności, a następnie jest ponownie obliczany absurdalnie - to DRY błąd. Gdyby ten drugi strlen(str)został wymieniony na len, nie byłoby możliwości przepełnienia bufora niezależnie od jego typu len. Odpowiedzi nie odnoszą się do tego punktu, po prostu udaje im się tego uniknąć.
Jim Balter
3
@CiaPan: Wenn przekazując do niego ciąg znaków nie zakończony znakiem null, strlen pokaże niezdefiniowane zachowanie.
Kaiserludi
3
@JimBalter Nie, myślę, że je tam zostawię. Może ktoś inny będzie miał to samo głupie błędne przekonanie i wyciągnie z niego wnioski. Możesz ich oflagować, jeśli cię irytują, ktoś może przyjść i je usunąć.
Asad Saeeduddin

Odpowiedzi:

192

W większości kompilatorów maksymalna wartość an unsigned shortto 65535.

Każda wartość powyżej jest zawijana, więc 65536 staje się 0, a 65600 staje się 65.

Oznacza to, że długie łańcuchy o odpowiedniej długości (np. 65600) przejdą kontrolę i przepełnią bufor.


Zastosowanie size_tdo przechowywania wynik strlen(), nie unsigned short, i porównać lendo wyrażenia, które bezpośrednio koduje rozmiar buffer. Na przykład:

char buffer[100];
size_t len = strlen(str);
if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;
memcpy(buffer, str, len + 1);
orlp
źródło
2
@PatrickRoberts Teoretycznie tak. Musisz jednak pamiętać, że 10% kodu odpowiada za 90% czasu wykonania, więc nie powinieneś rezygnować z wydajności przed bezpieczeństwem. I pamiętaj, że z czasem kod się zmienia, co może nagle oznaczać, że poprzednie sprawdzenie zniknęło.
orlp
3
Aby zapobiec przepełnieniu bufora, użyj po prostu lenjako trzeciego argumentu strncpy. W każdym razie ponowne użycie strlen jest głupie.
Jim Balter
15
/ sizeof(buffer[0])- zauważ, że sizeof(char)w C zawsze wynosi 1 (nawet jeśli znak zawiera miliardy bitów), więc jest to zbędne, gdy nie ma możliwości użycia innego typu danych. Mimo to ... brawa za pełną odpowiedź (i dziękuję za odpowiedzi na komentarze).
Jim Balter
3
@ rr-: char[]i char*nie są tym samym. Istnieje wiele sytuacji, w których test char[]zostanie niejawnie przekształcony w plik char*. Na przykład char[]jest dokładnie taki sam, jak char*używany jako typ argumentów funkcji. Jednak konwersja nie następuje w przypadku sizeof().
Dietrich Epp
4
@Controll Ponieważ jeśli zmienisz rozmiar bufferw pewnym momencie, wyrażenie zostanie automatycznie zaktualizowane. Jest to krytyczne dla bezpieczeństwa, ponieważ deklaracja buffermoże znajdować się o kilka wierszy od sprawdzania rzeczywistego kodu. Dlatego łatwo jest zmienić rozmiar bufora, ale zapomnij o aktualizacji w każdym miejscu, w którym rozmiar jest używany.
orlp
28

Problem jest tutaj:

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

Jeśli łańcuch jest większy niż długość bufora docelowego, strncpy nadal go kopiuje. Liczbę znaków łańcucha opierasz na liczbie do skopiowania, a nie na rozmiarze bufora. Prawidłowy sposób na zrobienie tego jest następujący:

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

To powoduje ograniczenie ilości danych kopiowanych do rzeczywistego rozmiaru bufora minus jeden dla znaku kończącego wartość null. Następnie ustawiamy ostatni bajt w buforze na znak null jako dodatkowe zabezpieczenie. Powodem tego jest to, że strncpy skopiuje do n bajtów, w tym kończący null, jeśli strlen (str) <len - 1. Jeśli nie, to wartość null nie jest kopiowana i masz scenariusz awarii, ponieważ teraz twój bufor ma niezakończony strunowy.

Mam nadzieję że to pomoże.

EDYCJA: Po dalszej analizie i wprowadzeniu informacji przez innych, możliwe kodowanie funkcji jest następujące:

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);
  }

Ponieważ znamy już długość ciągu, możemy użyć memcpy do skopiowania ciągu z lokalizacji, do której odwołuje się str, do bufora. Zauważ, że na stronie podręcznika dla strlen (3) (w systemie FreeBSD 9.3) podano, co następuje:

 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.

Co interpretuję jako, że długość łańcucha nie obejmuje wartości null. Dlatego kopiuję len + 1 bajtów, aby uwzględnić wartość null, a test sprawdza, czy długość <rozmiar bufora - 2. Minus jeden, ponieważ bufor zaczyna się na pozycji 0, a minus kolejny, aby upewnić się, że jest miejsce dla null.

EDYCJA: Okazuje się, że rozmiar czegoś zaczyna się od 1, a dostęp zaczyna się od 0, więc -2 przedtem było niepoprawne, ponieważ zwróciłoby błąd dla czegokolwiek> 98 bajtów, ale powinno być> 99 bajtów.

EDYCJA: Chociaż odpowiedź na temat skrótu bez znaku jest ogólnie poprawna, ponieważ maksymalna długość, którą można przedstawić, to 65 535 znaków, nie ma to większego znaczenia, ponieważ jeśli ciąg jest dłuższy, wartość zawinie się. To tak, jakby wziąć 75,231 (co jest 0x000125DF) i zamaskować górne 16 bitów, dając 9695 (0x000025DF). Jedynym problemem, jaki widzę w tym przypadku, jest pierwsze 100 znaków po 65 535, ponieważ sprawdzenie długości pozwoli na kopiowanie, ale we wszystkich przypadkach skopiuje tylko do pierwszych 100 znaków ciągu i zeruje ciąg . Więc nawet w przypadku problemu zawijania, bufor nadal nie zostanie przepełniony.

Może to samo w sobie stanowić zagrożenie bezpieczeństwa, w zależności od zawartości ciągu i tego, do czego go używasz. Jeśli jest to zwykły tekst, który jest czytelny dla człowieka, to generalnie nie ma problemu. Otrzymujesz po prostu obcięty ciąg. Jeśli jednak jest to adres URL lub nawet sekwencja poleceń SQL, możesz mieć problem.

Daniel Rudy
źródło
2
To prawda, ale to jest poza zakresem pytania. Kod wyraźnie pokazuje, że funkcja jest przekazywana jako wskaźnik znaku. Poza zakresem funkcji nas to nie obchodzi.
Daniel Rudy
„bufor, w którym przechowywany jest str” - to nie jest przepełnienie bufora , co jest tutaj problemem. I każda odpowiedź ma ten "problem", który jest nieunikniony, biorąc pod uwagę podpis func... i każdą inną kiedykolwiek napisaną funkcję C, która przyjmuje jako argumenty ciągi zakończone znakiem NUL. Podawanie możliwości, że wejście nie jest zakończone wartością NUL, jest całkowicie bezmyślne.
Jim Balter
„to jest poza zakresem pytania” - co niestety jest poza zdolnością niektórych ludzi do zrozumienia.
Jim Balter
„Problem jest tutaj” - masz rację, ale nadal brakuje Ci kluczowego problemu, którym jest to, że test ( len >= 100) został przeprowadzony na jednej wartości, ale długość kopii miała inną wartość ... to jest naruszeniem zasady DRY. Proste wywołanie strncpy(buffer, str, len)unika możliwości przepełnienia bufora i wykonuje mniej pracy niż strncpy(buffer,str,sizeof(buffer) - 1)... chociaż tutaj jest to po prostu wolniejszy odpowiednik memcpy(buffer, str, len).
Jim Balter
@JimBalter To jest poza zakresem pytania, ale dygresję. Rozumiem, że wartości używane przez test i to, co jest używane w strncpy, to dwie różne. Jednak ogólna praktyka kodowania mówi, że limit kopiowania powinien wynosić sizeof (bufor) - 1, więc nie ma znaczenia, jaka jest długość ciągu na kopii. strncpy przestanie kopiować bajty, gdy osiągnie wartość null lub skopiuje n bajtów. Następna linia gwarantuje, że ostatni bajt w buforze jest znakiem o wartości null. Kod jest bezpieczny, podtrzymuję moje poprzednie oświadczenie.
Daniel Rudy
11

Nawet jeśli używasz strncpy, długość odcięcia jest nadal zależna od przekazanego wskaźnika ciągu. Nie masz pojęcia, jak długi jest ten ciąg (to znaczy lokalizacja terminatora zerowego względem wskaźnika). Więc dzwonienie w strlenpojedynkę otwiera cię na słabość. Jeśli chcesz być bezpieczniejszy, użyj strnlen(str, 100).

Pełny kod poprawiony byłby:

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
źródło
@ user3386109 Czy strlenwtedy również nie uzyskałby dostępu poza końcem bufora?
Patrick Roberts
2
@ user3386109 to, co wskazujesz, sprawia, że ​​odpowiedź orlp jest tak samo nieprawidłowa jak moja. Nie rozumiem, dlaczego strnlennie rozwiązuje problemu, jeśli to, co sugeruje orlp, i tak jest rzekomo poprawne.
Patrick Roberts
1
„Myślę, że strnlen nie rozwiązuje tutaj niczego” - oczywiście, że tak; zapobiega przelewaniu się buffer. „ponieważ str może wskazywać na bufor 2 bajtów, z których żaden nie ma wartości NUL”. - to nie ma znaczenia, tak jak w przypadku każdej implementacji func. Pytanie tutaj dotyczy przepełnienia bufora, a nie UB, ponieważ wejście nie jest zakończone znakiem NUL.
Jim Balter
1
„Drugi parametr przekazany do strnlen musi być rozmiarem obiektu, na który wskazuje pierwszy parametr, lub strnlen jest bezwartościowy” - to kompletny i kompletny nonsens. Jeśli drugim argumentem strnlen jest długość ciągu wejściowego, to strnlen jest równoważne strlen. Jak w ogóle uzyskasz ten numer, a gdybyś go miał, dlaczego miałbyś dzwonić do str [n] len? W ogóle nie do tego służy strnlen.
Jim Balter
1
+1 Chociaż ta odpowiedź jest niedoskonała, ponieważ nie jest równoznaczne z kodem OP - strncpy NUL poduszki i nie NUL wypowiedzenia, natomiast StrCpy nul wygaśnięciem i nie NUL-pad, to nie rozwiąże problemu, w przeciwieństwie do śmieszne, ignoranckie komentarze powyżej.
Jim Balter
4

Odpowiedź z opakowaniem jest prawidłowa. Ale jest problem, o którym myślę, że nie został wspomniany, jeśli (len> = 100)

Cóż, gdyby Len miał 100, skopiowalibyśmy 100 elementów i nie mielibyśmy końcowych \ 0. To oczywiście oznaczałoby, że każda inna funkcja zależna od prawidłowo zakończonego łańcucha wyszłaby daleko poza oryginalną tablicę.

Ciąg problematyczny z C jest nierozwiązywalny IMHO. Zawsze lepiej jest mieć pewne ograniczenia przed rozmową, ale nawet to nie pomoże. Nie ma sprawdzania granic, więc przepełnienia bufora zawsze mogą i niestety się zdarzają ....

Friedrich
źródło
Problem ze stringami można rozwiązać: po prostu użyj odpowiednich funkcji. I. e. nie strncpy() i przyjaciele, ale funkcje przydzielania pamięci, takie jak strdup()i przyjaciele. Są w standardzie POSIX-2008, więc są dość przenośne, chociaż nie są dostępne w niektórych zastrzeżonych systemach.
cmaster
„dowolna inna funkcja zależna od prawidłowego zakończenia” - bufferjest lokalna dla tej funkcji i nie jest używana nigdzie indziej. W prawdziwym programie musielibyśmy zbadać, jak jest używany ... czasami zakończenie nieprzekraczające wartości NUL jest poprawne (pierwotnym zastosowaniem strncpy było utworzenie 14-bajtowych wpisów katalogu UNIX-a - uzupełnionych o NUL i nie zakończonych znakiem NUL). „Ciąg znaków problematyczny z C jest nierozwiązywalny w IMHO” - podczas gdy C jest wrednym językiem, przewyższającym znacznie lepszą technologię, można w nim napisać bezpieczny kod, jeśli zastosuje się odpowiednią dyscyplinę.
Jim Balter
Twoja obserwacja wydaje mi się błędna. if (len >= 100)jest warunkiem, kiedy sprawdzenie kończy się niepowodzeniem , a nie, gdy kończy się pomyślnie, co oznacza, że ​​nie ma przypadku, w którym dokładnie 100 bajtów bez terminatora NUL jest kopiowanych na drugą stronę, ponieważ ta długość jest uwzględniona w warunku niepowodzenia.
Patrick Roberts
@ cmaster. W tym przypadku się mylisz. Nie da się tego rozwiązać, bo zawsze można pisać poza granicami. Tak, jest to nieskrępowane zachowanie, ale nie ma sposobu, aby całkowicie temu zapobiec.
Friedrich
@Jim Balter. Nie ważne. Potencjalnie mogę pisać poza granicami tego lokalnego bufora, więc zawsze będzie możliwe uszkodzenie innych struktur danych.
Friedrich
3

Poza kwestiami bezpieczeństwa związanymi z wywołaniem strlenwięcej niż jeden raz, generalnie nie należy używać metod łańcuchowych na łańcuchach, których długość jest dokładnie znana [w przypadku większości funkcji łańcuchowych istnieje tylko bardzo wąski przypadek, w którym należy ich używać - na łańcuchach, dla których maksymalna długość można zagwarantować, ale dokładna długość nie jest znana]. Gdy znana jest długość ciągu wejściowego i znana jest długość bufora wyjściowego, należy dowiedzieć się, jak duży region powinien zostać skopiowany, a następnie użyć go memcpy()do faktycznego wykonania danej kopii. Chociaż możliwe jest, że strcpymoże memcpy()to osiągnąć lepsze wyniki podczas kopiowania ciągu o wielkości zaledwie 1-3 bajtów, na wielu platformach memcpy()może być ponad dwukrotnie szybszy w przypadku większych ciągów.

Chociaż istnieją sytuacje, w których bezpieczeństwo kosztowałoby wydajność, jest to sytuacja, w której bezpieczne podejście jest również szybsze. W niektórych przypadkach rozsądne może być napisanie kodu, który nie jest zabezpieczony przed dziwnie zachowującymi się danymi wejściowymi, jeśli kod dostarczający dane wejściowe może zapewnić, że będą one dobrze zachowane i jeśli ochrona przed niewłaściwymi danymi wejściowymi zmniejszy wydajność. Zapewnienie, że długości łańcuchów są sprawdzane tylko raz, poprawia zarówno wydajność, jak i bezpieczeństwo, chociaż można zrobić jedną dodatkową rzecz, aby pomóc chronić bezpieczeństwo nawet podczas ręcznego śledzenia długości ciągu: dla każdego łańcucha, który ma mieć końcowe null, zapisz końcowe null jawnie, a nie niż oczekiwanie, że łańcuch źródłowy go będzie miał. Tak więc, gdyby ktoś pisał strdupodpowiednik:

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;
}

Zauważ, że ostatnia instrukcja mogłaby zostać pominięta, jeśli memcpy przetworzyłby len+1bajty, ale jeśli inny wątek miałby zmodyfikować łańcuch źródłowy, wynikiem może być ciąg docelowy niezakończony NUL.

superkat
źródło
3
Czy możesz wyjaśnić kwestie bezpieczeństwa związane z dzwonieniem strlenwięcej niż raz ?
Bogdan Alexandru
1
@BogdanAlexandru: Kiedy ktoś zadzwoni strleni podejmie jakąś akcję w oparciu o zwróconą wartość (co prawdopodobnie było powodem wywołania jej w pierwszej kolejności), wówczas powtórzone połączenie (1) zawsze da taką samą odpowiedź jak pierwsza, w takim przypadku jest to po prostu marnowana praca lub (2) może czasami (ponieważ coś innego - być może inny wątek - zmodyfikował w międzyczasie ciąg) daje inną odpowiedź, w takim przypadku kod, który robi pewne rzeczy z długością (np. alokacja bufora) może przybierać inny rozmiar niż kod wykonujący inne czynności (kopiowanie do bufora).
supercat