NSA, Ghidra, dan Unicorn

NSA, Ghidra, dan Unicorn

Kali ini, tim PVS-Studio tertarik oleh Ghidra, kerangka kerja besar dan jahat untuk rekayasa terbalik yang dengannya Anda dapat menganalisis berbagai file biner dan melakukan segala macam hal menyeramkan bersama mereka. Hal yang paling menarik tentang itu bukanlah bahwa itu gratis untuk digunakan atau diperluas dengan baik dengan plugin, tetapi itu ditulis dalam NSA dan diposting di GitHub untuk semua orang. Di satu sisi, NSA tampaknya memiliki sumber daya yang cukup untuk menjaga basis kode tetap bersih. Dan di sisi lain, kontributor baru yang tidak terlalu mengenalnya bisa saja secara tidak sengaja menambahkan bug yang tidak terdeteksi belakangan ini. Oleh karena itu, berbekal analisis statis, kami memutuskan untuk mencari kelemahan dalam proyek ini.

Pendahuluan


Secara total, analisa statis PVS-Studio mengeluarkan 651 peringatan tinggi, 904 sedang, dan rendah 909 di bagian Jawa dari proyek Ghidra ( rilis 9.1.2, komit 687ce7f ). Di antara mereka, sekitar setengah dari respons tinggi dan sedang dipicu oleh diagnostik V6022 .Parameter tidak digunakan di dalam tubuh metode ", yang biasanya muncul setelah refactoring, ketika beberapa parameter tidak lagi diperlukan atau beberapa fungsi dinonaktifkan untuk sementara waktu oleh komentar. Lihat sekilas peringatan ini (ada terlalu banyak dari mereka untuk melihat masing-masing sebagai pengamat luar) ) dalam proyek ini tidak mengungkapkan sesuatu yang mencurigakan. Mungkin diperbolehkan untuk proyek ini untuk sementara menonaktifkan diagnostik ini dalam pengaturan analisa sehingga tidak terganggu olehnya. Dalam prakteknya, Anda sering dapat melihat kesalahan ketik atas nama setter atau parameter konstruktor dan, secara umum, seharusnya tidak Saya yakin sebagian besar pembaca setidaknya sekali menemukan pola yang tidak menyenangkan serupa:

public class A {
  private String value;
  public A(String val) { // V6022
    this.value = value;
  }
  public int hashCode() {
    return value.hashCode(); // NullPointerException
  }
}

Lebih dari setengah dari peringatan rendah dibuat oleh diagnostik " V6008 Potential null dereference 'variable'" - misalnya, nilai File.getParentFile () sering digunakan tanpa memeriksa nol . Jika objek file tempat metode ini dipanggil dibangun tanpa jalur absolut, null akan dikembalikan dan tidak adanya verifikasi dapat membatalkan aplikasi.

Secara tradisi, kami hanya akan menganalisis peringatan tingkat tinggi dan menengah, karena sebagian besar kesalahan nyata terkandung di dalamnya. Saat bekerja dengan laporan penganalisa, kami selalu merekomendasikan untuk menganalisis peringatan dengan urutan menurun keandalannya.

Selanjutnya, kami mempertimbangkan beberapa fragmen yang ditunjukkan oleh penganalisa yang tampak mencurigakan atau menarik bagi saya. Basis kode proyek ternyata berukuran sangat mengesankan, dan hampir mustahil untuk menemukan tempat-tempat seperti itu secara manual.

Fragmen 1: validasi rusak


private boolean parseDataTypeTextEntry()
throws InvalidDataTypeException {
  ...
  try {
    newDataType = parser.parse(selectionField.getText(),
                               getDataTypeRootForCurrentText());
  }
  catch (CancelledException e) {
    return false;
  }
  if (newDataType != null) {
    if (maxSize >= 0
        && newDataType.getLength() > newDataType.getLength()) { // <=
      throw new InvalidDataTypeException("data-type larger than "
                                         + maxSize + " bytes");
    }
    selectionField.setSelectedValue(newDataType);
    return true;
  }
  return false;
}

Peringatan PVS-Studio: V6001 Ada sub-ekspresi yang identik 'newDataType.getLength ()' di sebelah kiri dan di sebelah kanan operator '>'. DataTypeSelectionEditor.java data66

Kelas ini menyediakan komponen grafis untuk memilih tipe data yang mendukung pelengkapan otomatis. Pengembang yang menggunakan komponen ini dapat mengatur ukuran maksimum yang diizinkan dari tipe data yang dipilih (melalui bidang maxSize ) atau menjadikannya tidak terbatas dengan menetapkan nilai negatif. Diasumsikan bahwa ketika memvalidasi data yang dimasukkan, melebihi batas melempar pengecualian, yang kemudian menangkap tumpukan panggilan dan pengguna ditampilkan pesan.

Tampaknya pengarang komponen itu terganggu tepat pada saat menulis tes ini, atau mungkin dia memikirkan arti hidup, tetapi pada akhirnya, validasi tidak dilakukan, karena jumlahnya tidak pernah bisa lebih besar dari dirinya sendiri dan, karenanya, kita mengabaikan kondisi ini. Ini berarti bahwa komponen ini dapat memberikan data yang tidak valid.

Kesalahan serupa lainnya ditemukan di dua kelas lagi: GuidUtil dan NewGuid .

public class GuidUtil {
  ...
  public static GuidInfo parseLine(...) {
    ...
    long[] data = new long[4];
    ...
    if (isOK(data)) {
      if (!hasVersion) {
        return new GuidInfo(guidString, name, guidType);
      }
      return new VersionedGuidInfo(guidString, version, name, guidType);
    }
    return null;
  }
  ...
  private static boolean isOK(long[] data) {
    for (int i = 0; i < data.length; i++) {
      if ((data[i] != 0) || (data[i] != 0xFFFFFFFFL)) { // <=
        return true;
      }
    }
    return false;
  }
  ...
}

Peringatan PVS-Studio: Data V6007 Ekspresi '[i]! = 0xFFFFFFFFL' selalu benar. GuidUtil.java:200

The untuk loop yang isOK metode pemeriksaan bahwa nilai yang sama tidak sama untuk dua nomor yang berbeda pada waktu yang sama. Jika demikian, maka GUID segera diakui sebagai valid. Artinya, GUID akan menjadi tidak valid hanya jika array data kosong, dan ini tidak pernah terjadi, karena nilai variabel yang sesuai ditugaskan hanya sekali - pada awal metode parseLine . Badan

metode IsOKdi kedua kelas itu benar-benar bertepatan, yang menunjukkan gagasan lain copy-paste kode yang salah. Apa sebenarnya yang ingin diperiksa penulis, saya tidak yakin, tetapi saya dapat berasumsi bahwa metode ini harus diperbaiki sebagai berikut:

private static boolean isOK(long[] data) {
  for (int i = 0; i < data.length; i++) {
    if ((data[i] == 0) || (data[i] == 0xFFFFFFFFL)) {
      return false;
    }
  }
  return true;
}

Fragmen 2: menyembunyikan pengecualian


public void putByte(long offsetInMemBlock, byte b)
throws MemoryAccessException, IOException {
  long offsetInSubBlock = offsetInMemBlock - subBlockOffset;
  try {
    if (ioPending) {
      new MemoryAccessException("Cyclic Access"); // <=
    }
    ioPending = true;
    doPutByte(mappedAddress.addNoWrap(offsetInSubBlock / 8),
              (int) (offsetInSubBlock % 8), b);
  }
  catch (AddressOverflowException e) {
    new MemoryAccessException("No memory at address"); // <=
  }
  finally {
    ioPending = false;
  }
}

Peringatan PVS-Studio: V6006 Objek telah dibuat tetapi tidak digunakan. Kata kunci 'melempar' mungkin hilang: 'MemoryAccessException baru ("Akses Siklik")'. BitMappedSubMemoryBlock.java:99

Pengecualian objek sendiri, seperti yang Anda tahu, tidak melakukan apa-apa (atau setidaknya tidak boleh melakukan apa-apa). Hampir selalu, contoh baru mereka dilemparkan melalui lemparan , dalam beberapa kasus yang jarang - ditransfer ke suatu tempat atau mungkin ditempatkan dalam koleksi.

Kelas yang berisi metode ini adalah pembungkus di atas blok memori yang memungkinkan membaca dan menulis data. Di sini, karena fakta bahwa pengecualian tidak dilemparkan, pembatasan akses yang dikenakan ke blok memori saat ini dengan ioPending flag dapat dilanggardan, di samping itu, AddressOverflowException diabaikan . Dengan demikian, data dapat rusak secara diam-diam, dan bukannya secara eksplisit menunjukkan kesalahan di tempat tertentu, pengembang akan menerima artefak aneh yang harus dianalisis oleh debugger.

Ada delapan pengecualian yang hilang ini:

  • BitMappedSubMemoryBlock.java: lines 77, 99, 106, 122
  • ByteMappedSubMemoryBlock.java: lines 52, 73, 92, 114

Merupakan karakteristik bahwa dalam file yang sama ada metode yang sangat mirip di mana melempar hadir. Kemungkinan besar, satu metode awalnya ditulis mirip dengan fragmen di atas, setelah itu disalin beberapa kali, entah bagaimana menemukan kesalahan dan memperbaikinya di tempat-tempat yang mereka ingat.

Fragmen 3: ladang ranjau


private void processSelection(OptionsTreeNode selectedNode) {
  if (selectedNode == null) {
    setViewPanel(defaultPanel, selectedNode); // <=
    return;
  }
  ...
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
  ...
  setHelpLocation(component, selectedNode);
  ...
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
  Options options = node.getOptions();
  ...
}

PVS-Studio warning : V6008 Null dereference dari 'selectedNode' dalam fungsi 'setViewPanel'. OptionsPanel.java:66

Penganalisis berbohong sedikit - saat ini, memanggil metode processSelection tidak mengarah ke NullPointerException , karena metode ini dipanggil hanya dua kali, dan sebelum dipanggil, terpilihNode secara eksplisit diperiksa untuk null . Ini tidak boleh dilakukan, karena pengembang lain dapat melihat bahwa metode ini secara eksplisit menangani case selectedNode == null , dan memutuskan bahwa ini adalah nilai yang valid, yang kemudian akan mengakibatkan aplikasi mogok. Kejutan seperti itu sangat berbahaya hanya di proyek-proyek terbuka, karena orang-orang yang tidak mengetahui basis kode berpartisipasi secara menyeluruh di dalamnya.

Secara umum, saya harus mengatakan bahwa seluruh metode pemilihan proses terlihat agak aneh. Kemungkinan ini adalah kesalahan salin-tempel, karena dalam metode yang sama blok if dengan tubuh yang sama ditemukan dua kali lebih banyak, walaupun dengan kondisi yang berbeda. Namun, pada titik ini, yang dipilihNode tidak akan lagi menjadi nol , dan rantai panggilan setViewPanel-setHelpLocation tidak akan menghasilkan NullPointerException .

Fragmen 4: pelengkapan otomatis untuk kejahatan


public static final int[] UNSUPPORTED_OPCODES_LIST = { ... };
public static final Set<Integer> UNSUPPORTED_OPCODES = new HashSet<>();

static {
  for (int opcode : UNSUPPORTED_OPCODES) {
    UNSUPPORTED_OPCODES.add(opcode);
  }
}

Peringatan PVS-Studio: V6053 Koleksi ini dimodifikasi saat iterasi sedang berlangsung. ConcurrentModificationException dapat terjadi. DWARFExpressionOpCodes.java:205

Dalam kasus ini, penganalisa lagi berbohong sedikit - pengecualian tidak akan dibuang, karena koleksi UNSUPPORTED_OPCODES selalu kosong dan loop tidak akan dieksekusi. Selain itu, kebetulan bahwa koleksi adalah banyak, dan menambahkan elemen yang sudah ada tidak akan mengubahnya. Kemungkinan besar, penulis memasukkan masing-masingnama koleksi melalui pelengkapan otomatis dan tidak melihat bahwa bidang yang salah telah diusulkan. Modifikasi koleksi selama iterasi tidak mungkin, tetapi dalam keadaan baik, seperti dalam kasus ini, aplikasi mungkin tidak jatuh. Di sini, kesalahan ketik ini memiliki efek tidak langsung: mesin yang mem-parsing file DWARF bergantung pada koleksi ini untuk menghentikan analisis ketika menemukan opcode yang tidak didukung.

Dimulai dengan Java 9, perlu menggunakan metode pabrik dari perpustakaan standar untuk koleksi konstan: misalnya, Set.of (T ... elemen) tidak hanya jauh lebih nyaman, tetapi juga segera membuat koleksi yang dibuat tidak dapat diubah, yang meningkatkan keandalan kode.

Fragmen 5: ada segalanya


public void setValueAt(Object aValue, int row, int column) {
  ...
  int index = indexOf(newName);
  if (index >= 0) {                  // <=
    Window window = tool.getActiveWindow();
    Msg.showInfo(getClass(), window, "Duplicate Name",
                 "Name already exists: " + newName);
    return;
  }

  ExternalPath path = paths.get(row); // <=
  ...
}
private int indexOf(String name) {
  for (int i = 0; i < paths.size(); i++) {
    ExternalPath path = paths.get(i);
    if (path.getName().equals(name)) {
      return i;
    }
  }
  return 0;
}

Peringatan PVS-Studio:

  • Ekspresi V6007 'indeks> = 0' selalu benar. ExternalNamesTableModel.java:105
  • V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

Penulis memikirkannya dan dalam metode indexOf alih-alih "index" -1 untuk nilai yang tidak terdeteksi mengembalikan 0 - indeks elemen pertama dari koleksi path . Bahkan jika koleksinya kosong. Atau mungkin metode yang dihasilkan, tetapi lupa untuk mengubah nilai pengembalian default. Akibatnya, metode setValueAt akan membuang nilai apa pun yang diteruskan ke sana dan menampilkan pengguna dengan kesalahan "Nama sudah ada", bahkan jika tidak ada nama yang ada.

By the way, indexOf tidak digunakan di tempat lain, dan nilainya diperlukan hanya untuk menentukan apakah elemen yang Anda cari ada. Mungkin, bukannya metode terpisah, write untuk-masing langsung dalam setValueAt dan make kembalipada item yang cocok alih-alih game dengan indeks.

Catatan: Saya tidak dapat mereproduksi kesalahan yang dituduhkan. Metode setValueAt mungkin tidak lagi digunakan atau dipanggil hanya dalam kondisi tertentu.

Fragmen 6: tetap diam


final static Map<Character, String> DELIMITER_NAME_MAP = new HashMap<>(20);
// Any non-alphanumeric char can be used as a delimiter.
static {
  DELIMITER_NAME_MAP.put(' ', "Space");
  DELIMITER_NAME_MAP.put('~', "Tilde");
  DELIMITER_NAME_MAP.put('`', "Back quote");
  DELIMITER_NAME_MAP.put('@', "Exclamation point");
  DELIMITER_NAME_MAP.put('@', "At sign");
  DELIMITER_NAME_MAP.put('#', "Pound sign");
  DELIMITER_NAME_MAP.put('$', "Dollar sign");
  DELIMITER_NAME_MAP.put('%', "Percent sign");
  ...
}

PVS-Studio Warning: V6033 Item dengan kunci yang sama '@' telah ditambahkan. FilterOptions.java:45

Ghidra mendukung pemfilteran data dalam berbagai konteks: misalnya, Anda dapat memfilter daftar file proyek berdasarkan nama. Selain itu, pemfilteran dengan beberapa kata kunci sekaligus diterapkan: '.java, .c' ketika mode 'OR' aktif, menampilkan semua file yang namanya mengandung '.java' atau '.c'. Dapat dipahami bahwa setiap karakter khusus dapat digunakan sebagai pemisah kata (pemisah tertentu dipilih dalam pengaturan filter), tetapi pada kenyataannya tanda seru tidak tersedia.

Dalam lembar inisialisasi seperti itu, sangat mudah untuk disegel, karena mereka sering ditulis menggunakan copy-paste, dan ketika Anda melihat kode tersebut, mata Anda dengan cepat kabur. Dan jika kesalahan ketik tidak pada dua garis yang berdekatan, maka dengan tangan hampir pasti tidak ada yang melihat.

Fragmen 7: sisa pembagian selalu 0


void setFactorys(FieldFactory[] fieldFactorys,
                 DataFormatModel dataModel, int margin) {
  factorys = new FieldFactory[fieldFactorys.length];

  int x = margin;
  int defaultGroupSizeSpace = 1;
  for (int i = 0; i < factorys.length; i++) {
    factorys[i] = fieldFactorys[i];
    factorys[i].setStartX(x);
    x += factorys[i].getWidth();
    // add in space between groups
    if (((i + 1) % defaultGroupSizeSpace) == 0) { // <=
      x += margin * dataModel.getUnitDelimiterSize();
    }
  }
  width = x - margin * dataModel.getUnitDelimiterSize() + margin;
  layoutChanged();
}

Peringatan PVS-Studio:

  • Ekspresi V6007 '((i +1)% defaultGroupSizeSpace) == 0' selalu benar. ByteViewerLayoutModel.java:66
  • V6048 Ungkapan ini bisa disederhanakan. Operand 'defaultGroupSizeSpace' dalam operasi sama dengan 1. ByteViewerLayoutModel.java:66

Penampil hex byte mendukung pilihan ukuran grup yang ditampilkan: misalnya, Anda dapat mengonfigurasi output dalam format 'ffff ffff' atau 'ff ff ff ff'. Metode setFactorys bertanggung jawab atas lokasi grup ini di antarmuka pengguna . Terlepas dari kenyataan bahwa kustomisasi dan tampilan berfungsi dengan benar, siklus dalam metode ini terlihat sangat mencurigakan: sisa pembagian dengan satu selalu nol, yang berarti bahwa koordinat x akan meningkat pada setiap iterasi. Kecurigaan menambahkan properti dan ketersediaan groupSize dalam pengaturan DataModel .

Sisa sampah setelah refactoring? Atau mungkin perhitungan variabel defaultGroupSizeSpace hilang? Bagaimanapun, upaya untuk mengganti nilainya dengan dataModel.getGroupSize () memecah tata letak, dan mungkin hanya pembuat kode ini yang dapat memberikan jawaban yang jelas.

Fragmen 8: validasi rusak, bagian 2


private String parseArrayDimensions(String datatype,
                                    List<Integer> arrayDimensions) {
  String dataTypeName = datatype;
  boolean zeroLengthArray = false;
  while (dataTypeName.endsWith("]")) {
    if (zeroLengthArray) {                   // <=
      return null; // only last dimension may be 0
    }
    int rBracketPos = dataTypeName.lastIndexOf(']');
    int lBracketPos = dataTypeName.lastIndexOf('[');
    if (lBracketPos < 0) {
      return null;
    }
    int dimension;
    try {
      dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                          rBracketPos));
      if (dimension < 0) {
        return null; // invalid dimension
      }
    }
    catch (NumberFormatException e) {
      return null;
    }
    dataTypeName = dataTypeName.substring(0, lBracketPos).trim();
    arrayDimensions.add(dimension);
  }
  return dataTypeName;
}

PVS-Studio Peringatan: V6007 Expression 'zeroLengthArray' selalu salah. PdbDataTypeParser.java:78

Metode ini mem-parsing dimensi array multidimensi dan mengembalikan teks yang tersisa setelah parsing atau null untuk data yang tidak valid. Komentar di sebelah salah satu pemeriksaan validasi menyatakan bahwa hanya ukuran baca terakhir yang bisa nol. Analisis bergerak dari kanan ke kiri, sehingga dipahami bahwa '[0] [1] [2]' adalah teks input yang valid, dan '[2] [1] [0]' tidak.

Tapi, masalah: tidak ada yang menambahkan cek bahwa ukuran berikutnya adalah nol, dan parser memakan data yang tidak valid tanpa pertanyaan yang tidak perlu. Anda mungkin harus memperbaiki blok try sebagai berikut:

try {
  dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                      rBracketPos));
  if (dimension < 0) {
    return null; // invalid dimension
  } else if (dimension == 0) {
    zeroLengthArray = true;
  }
}
catch (NumberFormatException e) {
  return null;
}

Secara alami, ada kemungkinan bahwa kriteria validitas ini dari waktu ke waktu ternyata tidak diperlukan, atau komentar penulis memiliki arti yang berbeda dan perlu untuk memeriksa pembacaan dimensi pertama. Dalam kasus apa pun, validasi data adalah bagian penting dari aplikasi apa pun, yang harus diambil dengan tanggung jawab penuh. Kesalahan di dalamnya dapat menyebabkan crash aplikasi yang tidak berbahaya, serta lubang keamanan, kebocoran data, korupsi atau kehilangan data (misalnya, jika Anda melewatkan injeksi SQL selama validasi permintaan).

Sedikit tentang sisa peringatan


Pembaca mungkin memperhatikan bahwa banyak peringatan dikeluarkan, tetapi sedikit yang dipertimbangkan. Tidak terlalu disetel cloc dalam proyek menghitung sekitar 1,25 juta baris kode Java (tidak kosong dan tidak komentar). Faktanya adalah bahwa hampir semua peringatan sangat mirip: di sini mereka lupa untuk memeriksa nol , mereka tidak menghapus kode warisan yang tidak digunakan di sana. Saya benar-benar tidak ingin membuat pembaca bosan dengan membuat daftar hal yang sama, dan saya menyebutkan sebagian dari kasus tersebut di awal artikel.

Contoh lain adalah peringatan lima puluh " Fungsi V6009 menerima argumen aneh" dalam konteks penggunaan metode substring yang tidak akurat.(CParserUtils.java:80, ComplexName.java:48 dan lainnya) untuk mendapatkan sisa string setelah pemisah apa pun. Pengembang sering berharap bahwa pemisah ini akan hadir dalam string dan lupa bahwa jika tidak indexOf akan mengembalikan -1, yang merupakan nilai yang salah untuk substring . Secara alami, jika data divalidasi atau diterima bukan dari luar, maka kemungkinan aplikasi crash secara signifikan berkurang. Namun, secara umum, ini adalah tempat-tempat yang berpotensi berbahaya yang ingin kami bantu singkirkan.

Kesimpulan


Secara umum, Ghidra senang dengan kualitas kode - tidak ada mimpi buruk yang jelas. Kode diformat dengan baik dan memiliki gaya yang sangat konsisten: dalam kebanyakan kasus, variabel, metode, dan yang lainnya diberi nama yang jelas, komentar penjelasan ditemukan, sejumlah besar tes hadir.

Secara alami, tidak ada masalah, di antaranya:

  • Kode mati, yang, kemungkinan besar, tetap setelah banyak refactoring;
  • Banyak javadocs sudah usang dan, misalnya, menunjukkan parameter yang tidak ada;
  • Tidak ada kemungkinan pengembangan yang mudah saat menggunakan IntelliJ IDEA ;
  • Sistem modular yang dibangun di sekitar refleksi membuat navigasi proyek dan menemukan ketergantungan antar komponen menjadi lebih sulit.

Tolong jangan mengabaikan alat pengembang. Analisis statis, seperti sabuk pengaman, bukanlah obat mujarab, tetapi akan membantu mencegah beberapa bencana sebelum dilepaskan. Dan tidak ada yang suka menggunakan perangkat lunak yang masuk.

Anda dapat membaca tentang proyek lain yang terbukti di blog kami . Dan kami juga memiliki lisensi uji coba dan berbagai opsi untuk menggunakan alat analisis tanpa perlu membayarnya.



Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Nikita Lazeba. NSA, Ghidra, dan Unicorn .

All Articles