Mengapa kode ini rentan terhadap serangan buffer overflow?

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

Kode ini rentan terhadap serangan buffer overflow, dan saya mencoba mencari tahu alasannya. Saya pikir ini ada hubungannya dengan lendinyatakan sebagai shortpengganti int, tapi saya tidak begitu yakin.

Ada ide?

Jason
sumber
3
Ada beberapa masalah dengan kode ini. Ingat bahwa string C diakhiri null.
Dmitri Chubarov
4
@ DmitriChubarov, bukan nol mengakhiri string akan menjadi masalah hanya jika string digunakan setelah panggilan ke strncpy. Dalam hal ini tidak.
R Sahu
43
Masalah dalam kode ini mengalir langsung dari fakta yang strlendihitung, digunakan untuk pemeriksaan validitas, dan kemudian dihitung secara absurd lagi - ini merupakan kegagalan KERING. Jika yang kedua strlen(str)diganti dengan len, tidak akan ada kemungkinan buffer overflow, terlepas dari jenis len. Jawabannya tidak membahas poin ini, mereka hanya berhasil menghindarinya.
Jim Balter
3
@CiaPan: Jika melewati string yang tidak diakhiri null untuk itu, strlen akan menunjukkan perilaku yang tidak terdefinisi.
Kaiserludi
3
@ JimBalter Nah, saya pikir saya akan meninggalkan mereka di sana. Mungkin orang lain akan memiliki kesalahpahaman bodoh yang sama dan belajar darinya. Jangan ragu untuk menandai mereka jika mereka mengganggu Anda, seseorang mungkin akan datang dan menghapusnya.
Asad Saeeduddin

Jawaban:

192

Pada kebanyakan penyusun nilai maksimum suatu unsigned shortadalah 65535.

Nilai apa pun di atas yang dililit, jadi 65536 menjadi 0, dan 65600 menjadi 65.

Ini berarti bahwa string panjang dengan panjang yang tepat (misalnya 65600) akan melewati pemeriksaan, dan meluap buffer.


Gunakan size_tuntuk menyimpan hasil strlen(), bukan unsigned short, dan membandingkan lendengan ekspresi yang secara langsung menyandikan ukuran buffer. Jadi misalnya:

char buffer[100];
size_t len = strlen(str);
if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;
memcpy(buffer, str, len + 1);
orlp
sumber
2
@ PatrickRoberts Secara teoritis, ya. Tetapi Anda harus ingat bahwa 10% dari kode bertanggung jawab atas 90% dari runtime, jadi Anda tidak boleh membiarkan kinerja berjalan sebelum keamanan. Dan perlu diingat bahwa seiring waktu kode berubah, yang dapat tiba-tiba berarti bahwa pemeriksaan sebelumnya hilang.
orlp
3
Untuk mencegah buffer overflow, cukup gunakan lensebagai argumen ketiga strncpy. Menggunakan strlen lagi bodoh dalam hal apapun.
Jim Balter
15
/ sizeof(buffer[0])- perhatikan bahwa sizeof(char)di C selalu 1 (bahkan jika char berisi banyak trilyun bit) jadi itu berlebihan ketika tidak ada kemungkinan menggunakan tipe data yang berbeda. Tetap ... pujian untuk jawaban yang lengkap (dan terima kasih sudah responsif terhadap komentar).
Jim Balter
3
@rr-: char[]dan char*bukan hal yang sama. Ada banyak situasi di mana a char[]akan secara implisit dikonversi menjadi char*. Misalnya, char[]persis sama dengan char*ketika digunakan sebagai tipe untuk argumen fungsi. Namun, konversi tidak terjadi sizeof().
Dietrich Epp
4
@Kontrol Karena jika Anda mengubah ukuran bufferpada suatu titik, ekspresi secara otomatis akan diperbarui. Ini sangat penting untuk keamanan, karena deklarasi buffermungkin beberapa baris jauh dari pemeriksaan kode aktual. Jadi mudah untuk mengubah ukuran buffer, tetapi lupa memperbarui di setiap lokasi di mana ukuran digunakan.
orlp
28

Masalahnya ada di sini:

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

Jika string lebih besar dari panjang buffer target, strncpy akan tetap menyalinnya. Anda mendasarkan jumlah karakter string sebagai angka yang akan disalin, bukan ukuran buffer. Cara yang benar untuk melakukan ini adalah sebagai berikut:

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

Apa yang dilakukan adalah membatasi jumlah data yang disalin ke ukuran aktual buffer dikurangi satu untuk karakter terminasi nol. Kemudian kita mengatur byte terakhir dalam buffer ke karakter nol sebagai perlindungan tambahan. Alasan untuk ini adalah karena strncpy akan menyalin hingga n byte, termasuk null terminating, jika strlen (str) <len - 1. Jika tidak, maka null tidak disalin dan Anda memiliki skenario macet karena sekarang buffer Anda memiliki underminated tali.

Semoga ini membantu.

EDIT: Setelah pemeriksaan lebih lanjut dan masukan dari orang lain, kemungkinan pengkodean untuk fungsi ini adalah sebagai berikut:

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

Karena kita sudah mengetahui panjang string, kita bisa menggunakan memcpy untuk menyalin string dari lokasi yang dirujuk oleh str ke buffer. Perhatikan bahwa per halaman manual untuk strlen (3) (pada sistem FreeBSD 9.3), berikut ini dinyatakan:

 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.

Yang saya menafsirkan bahwa panjang string tidak termasuk nol. Itu sebabnya saya menyalin len +1 byte untuk memasukkan nol, dan tes memeriksa untuk memastikan bahwa panjang <ukuran buffer - 2. Minus satu karena buffer dimulai pada posisi 0, dan minus satu lagi untuk memastikan ada ruang untuk nol.

EDIT: Ternyata, ukuran sesuatu dimulai dengan 1 sementara akses dimulai dengan 0, jadi -2 sebelumnya salah karena akan mengembalikan kesalahan untuk apa pun> 98 byte tetapi seharusnya> 99 byte.

EDIT: Meskipun jawaban tentang unsigned short umumnya benar karena panjang maksimum yang dapat diwakili adalah 65.535 karakter, itu tidak terlalu penting karena jika string lebih panjang dari itu, nilainya akan membungkus. Ini seperti mengambil 75.231 (yang merupakan 0x000125DF) dan menutupi 16 bit teratas memberi Anda 9695 (0x000025DD). Satu-satunya masalah yang saya lihat dengan ini adalah 100 karakter pertama melewati 65.535 karena pemeriksaan panjang akan memperbolehkan penyalinan, tetapi hanya akan menyalin hingga 100 karakter pertama dari string dalam semua kasus dan null mengakhiri string . Begitu pun dengan masalah sampulnya, buffer masih tidak akan meluap.

Ini mungkin atau tidak dengan sendirinya menimbulkan risiko keamanan tergantung pada konten string dan untuk apa Anda menggunakannya. Jika hanya teks lurus yang dapat dibaca manusia, maka umumnya tidak ada masalah. Anda hanya mendapatkan string terpotong. Namun, jika itu sesuatu seperti URL atau bahkan urutan perintah SQL, Anda bisa memiliki masalah.

Daniel Rudy
sumber
2
Benar, tapi itu di luar ruang lingkup pertanyaan. Kode tersebut dengan jelas menunjukkan fungsi yang dilewatkan sebuah pointer char. Di luar lingkup fungsi, kami tidak peduli.
Daniel Rudy
"buffer di mana str disimpan" - itu bukan buffer overflow , yang merupakan masalah di sini. Dan setiap jawaban memiliki "masalah" itu, yang tidak dapat dihindari dengan tanda tangan func... dan setiap fungsi C lainnya yang pernah ditulis yang menggunakan string yang diakhiri NUL sebagai argumen. Memunculkan kemungkinan input yang tidak diakhiri NUL sama sekali tidak mengerti.
Jim Balter
"Itu di luar ruang lingkup pertanyaan" - yang sayangnya di luar kemampuan beberapa orang untuk memahaminya.
Jim Balter
"Masalahnya ada di sini" - Anda benar, tetapi Anda masih melewatkan masalah utama, yaitu bahwa pengujian ( len >= 100) dilakukan terhadap satu nilai tetapi panjang salinan diberikan nilai yang berbeda ... ini merupakan pelanggaran terhadap prinsip KERING. Cukup menelepon strncpy(buffer, str, len)menghindari kemungkinan buffer overflow, dan bekerja lebih sedikit daripada strncpy(buffer,str,sizeof(buffer) - 1)... meskipun di sini hanya setara dengan lebih lambat memcpy(buffer, str, len).
Jim Balter
@ JimBalter Ini di luar ruang lingkup pertanyaan, tapi saya ngelantur. Saya mengerti bahwa nilai yang digunakan oleh tes dan apa yang digunakan dalam strncpy adalah dua yang berbeda. Namun, praktik pengkodean umum mengatakan bahwa batas salinan harus sizeof (buffer) - 1 sehingga tidak masalah berapa panjang str pada salinan. strncpy akan berhenti menyalin byte ketika hit nol atau salinan n byte. Baris berikutnya menjamin bahwa byte terakhir dalam buffer adalah null char. Kode aman, saya mendukung pernyataan saya sebelumnya.
Daniel Rudy
11

Meskipun Anda menggunakan strncpy, panjang cutoff masih tergantung pada pointer string yang lewat. Anda tidak tahu berapa lama string itu (lokasi dari terminator nol relatif terhadap pointer, yaitu). Jadi menelepon strlenseorang diri membuat Anda rentan. Jika Anda ingin lebih aman, gunakan strnlen(str, 100).

Kode lengkap yang diperbaiki adalah:

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
sumber
@ user3386109 Bukankah strlenkemudian akses melewati akhir buffer?
Patrick Roberts
2
@ user3386109 apa yang Anda tunjukkan membuat jawaban orlp sama tidak validnya dengan milik saya. Saya gagal melihat mengapa strnlentidak menyelesaikan masalah jika apa yang disarankan orlp seharusnya benar.
Patrick Roberts
1
"Saya tidak berpikir strnlen menyelesaikan apa pun di sini" - tentu saja; itu mencegah meluap buffer. "Karena str dapat menunjuk ke buffer 2 byte, yang keduanya tidak NUL." - itu tidak relevan, karena itu benar dari setiap pelaksanaan func. Pertanyaannya di sini adalah tentang buffer overflow, bukan UB karena inputnya tidak diakhiri dengan NUL.
Jim Balter
1
"Parameter kedua yang diteruskan ke strnlen haruslah ukuran objek yang ditunjuk parameter pertama, atau strnlen tidak berharga" - ini lengkap dan benar-benar omong kosong. Jika argumen kedua untuk strnlen adalah panjang dari string input, maka strnlen setara dengan strlen. Bagaimana Anda mendapatkan nomor itu, dan jika Anda memilikinya, mengapa Anda perlu menelepon str [n] len? Itu bukan untuk apa strnlen sama sekali.
Jim Balter
1
1 Meskipun jawaban ini tidak sempurna karena itu tidak setara dengan kode OP - strncpy NUL-bantalan dan tidak NUL mengakhiri, sedangkan strcpy NUL-Menghentikan dan tidak NUL-pad, itu tidak memecahkan masalah, bertentangan dengan komentar konyol di atas.
Jim Balter
4

Jawaban dengan pembungkusnya benar. Tapi ada masalah yang saya pikir tidak disebutkan jika (len> = 100)

Jika Len 100, kita akan menyalin 100 elemen dan kita tidak akan tertinggal \ 0. Itu jelas akan berarti fungsi lain tergantung pada string akhir yang tepat akan berjalan jauh melampaui array asli.

String yang bermasalah dari C adalah IMHO yang tidak dapat diselesaikan. Anda sebaiknya memiliki beberapa batasan sebelum panggilan, tetapi itu pun tidak akan membantu. Tidak ada batas memeriksa dan buffer overflows selalu dapat dan sayangnya akan terjadi ....

Friedrich
sumber
String bermasalah dapat dipecahkan: cukup gunakan fungsi yang sesuai. Saya. E. bukan strncpy() dan teman, tapi fungsi pengalokasian memori suka strdup()dan teman. Mereka berada dalam standar POSIX-2008, sehingga mereka cukup portabel, meskipun tidak tersedia pada beberapa sistem berpemilik.
cmaster - mengembalikan monica
"fungsi lain apa pun tergantung pada string akhir yang benar" - bufferbersifat lokal untuk fungsi ini dan tidak digunakan di tempat lain. Dalam sebuah program nyata kita harus memeriksa bagaimana ini digunakan ... kadang-kadang tidak mengakhiri NUL benar (penggunaan asli strncpy adalah untuk membuat entri direktori 14 byte UNIX - NUL-empuk dan bukan NUL-dihentikan). "String yang bermasalah dari C adalah IMHO yang tidak dapat diselesaikan" - sementara C adalah bahasa yang sulit dipahami yang telah dilampaui oleh teknologi yang jauh lebih baik, kode aman dapat ditulis di dalamnya jika cukup disiplin digunakan.
Jim Balter
Pengamatan Anda bagi saya keliru. if (len >= 100)adalah kondisi ketika cek gagal , bukan ketika melewati, yang berarti tidak ada kasus di mana tepatnya 100 byte tanpa terminator NUL disalin, karena panjangnya termasuk dalam kondisi gagal.
Patrick Roberts
@ cmaster. Dalam hal ini Anda salah. Ini tidak bisa dipecahkan, karena orang selalu bisa menulis diotak batas. Ya itu perilaku yang tidak didefinisikan tetapi tidak ada cara untuk mencegahnya sepenuhnya.
Friedrich
@ Jim Balter. Tidak masalah. Saya berpotensi dapat menulis di atas batas buffer lokal ini dan karenanya akan selalu mungkin untuk merusak beberapa struktur data lainnya.
Friedrich
3

Di luar masalah keamanan yang terlibat dengan pemanggilan strlenlebih dari satu kali, seseorang umumnya tidak boleh menggunakan metode string pada string yang panjangnya diketahui dengan tepat [untuk sebagian besar fungsi string, hanya ada kasus yang sangat sempit di mana mereka harus digunakan - pada string yang maksimum panjang bisa dijamin, tetapi panjang pastinya tidak diketahui]. Setelah panjang string input diketahui dan panjang buffer output diketahui, orang harus mencari tahu seberapa besar suatu wilayah harus disalin dan kemudian digunakan memcpy()untuk benar-benar melakukan salinan yang dimaksud. Meskipun mungkin yang strcpymungkin mengungguli memcpy()ketika menyalin string hanya 1-3 byte atau lebih, pada banyak platform memcpy()cenderung lebih dari dua kali lebih cepat ketika berhadapan dengan string yang lebih besar.

Meskipun ada beberapa situasi di mana keamanan akan mengorbankan kinerja, ini adalah situasi di mana pendekatan yang aman juga lebih cepat. Dalam beberapa kasus, mungkin masuk akal untuk menulis kode yang tidak aman terhadap input berperilaku aneh, jika kode yang memasok input dapat memastikan mereka akan berperilaku baik, dan jika menjaga terhadap input berperilaku buruk akan menghambat kinerja. Memastikan bahwa panjang string hanya diperiksa sekali meningkatkan baik kinerja dan keamanan, meskipun satu hal tambahan yang dapat dilakukan untuk membantu penjaga keamanan bahkan ketika pelacakan panjang string secara manual: untuk setiap string yang yang diharapkan memiliki nol miring, menulis nol Trailing eksplisit lebih daripada mengharapkan string sumber untuk memilikinya. Jadi, jika seseorang menulis yang strdupsetara:

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

Perhatikan bahwa pernyataan terakhir secara umum dapat dihilangkan jika memcpy telah memproses len+1byte, tetapi utas lainnya adalah untuk memodifikasi string sumber, hasilnya bisa menjadi string tujuan yang tidak diakhiri dengan NUL.

supercat
sumber
3
Bisakah Anda jelaskan masalah keamanan yang terlibat dengan panggilan strlenlebih dari satu kali ?
Bogdan Alexandru
1
@BogdanAlexandru: Setelah seseorang memanggil strlendan mengambil tindakan berdasarkan nilai yang dikembalikan (yang mungkin merupakan alasan untuk memanggilnya di tempat pertama), maka panggilan berulang baik (1) akan selalu menghasilkan jawaban yang sama seperti yang pertama, dalam hal ini hanya buang-buang pekerjaan, atau (2) kadang-kadang (karena sesuatu yang lain - mungkin utas lainnya - memodifikasi string sementara itu) menghasilkan jawaban yang berbeda, di mana kode kasus yang melakukan beberapa hal dengan panjang (misalnya mengalokasikan buffer) dapat mengasumsikan ukuran yang berbeda dari kode yang melakukan hal-hal lain (menyalin ke buffer).
supercat