ProHoster > Blog > administrasi > Memeriksa FreeRDP dengan penganalisis PVS-Studio

Memeriksa FreeRDP dengan penganalisis PVS-Studio

Memeriksa FreeRDP menggunakan penganalisa PVS-Studio
FreeRDP adalah implementasi sumber terbuka dari Remote Desktop Protocol (RDP), sebuah protokol yang dikembangkan oleh Microsoft untuk kontrol komputer jarak jauh. Proyek ini mendukung berbagai platform, termasuk Windows, Linux, macOS dan bahkan iOS dengan AndroidProyek ini dipilih sebagai yang pertama dalam serangkaian artikel yang membahas pengujian klien RDP menggunakan penganalisis statis PVS-Studio.

Sedikit sejarah

Proyek RDP gratis muncul setelah Microsoft membuka spesifikasi untuk protokol RDP miliknya. Saat itu ada klien rdesktop yang implementasinya berdasarkan hasil Reverse Engineering.

Ketika protokol diterapkan, penambahan fungsionalitas baru menjadi lebih sulit karena arsitektur proyek yang sudah ada. Perubahan di dalamnya menimbulkan konflik antar pengembang, yang menyebabkan terciptanya cabang rdesktop - FreeRDP. Distribusi lebih lanjut dari produk ini dibatasi oleh lisensi GPLv2, sebagai akibatnya diambil keputusan untuk melisensikannya kembali ke Lisensi Apache v2. Namun, tidak semua orang setuju untuk mengubah lisensi kode mereka, sehingga pengembang memutuskan untuk menulis ulang proyek tersebut, sehingga menghasilkan basis kode yang modern.

Anda dapat membaca lebih lanjut tentang sejarah proyek di postingan blog resmi: “Sejarah proyek FreeRDP”.

Digunakan sebagai alat untuk mengidentifikasi kesalahan dan potensi kerentanan dalam kode. PVS-StudioIni adalah penganalisis kode statis untuk C, C++, C# dan Java, tersedia di berbagai platform. Windows, Linux и macOS.

Artikel ini hanya menyajikan kesalahan-kesalahan yang menurut saya paling menarik.

Kebocoran memori

V773 Fungsi tersebut keluar tanpa melepaskan penunjuk 'cwd'. Kebocoran memori mungkin terjadi. lingkungan.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }

  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }

  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

Fragmen ini diambil dari subsistem winpr, yang mengimplementasikan pembungkus WINAPI untuk non-Windows sistem, yaitu, ini adalah analog ringan dari Wine. Di sini Anda dapat melihat kebocoran: memori yang dialokasikan oleh fungsi tersebut. dapatkan cwd, dirilis hanya ketika menangani kasus-kasus khusus. Untuk memperbaiki kesalahan Anda perlu menambahkan panggilan gratis setelah memcpy.

Susunan di luar batas

V557 Pembebanan array mungkin terjadi. Nilai indeks 'event->EventHandlerCount' bisa mencapai 32. PubSub.c 117

#define MAX_EVENT_HANDLERS  32

struct _wEventType
{
  ....
  int EventHandlerCount;
  pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};

int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
      pEventHandler EventHandler)
{
  ....
  if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
  {
    event->EventHandlers[event->EventHandlerCount] = EventHandler;
    event->EventHandlerCount++;
  }
  ....
}

Contoh ini menambahkan elemen baru ke daftar meskipun jumlah elemen telah mencapai maksimum. Di sini cukup mengganti operatornya <= pada <, agar tidak melampaui batas array.

Kesalahan lain dari jenis ini ditemukan:

  • V557 Array yang dibanjiri mungkin terjadi. Nilai indeks 'iBitmapFormat' bisa mencapai 8.orders.c 2623

salah ketik

Fragmen 1

V547 Ekspresi '!pipe->In' selalu salah. PesanPipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;

  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....
}

Di sini kita melihat kesalahan ketik yang umum: kondisi kedua memeriksa variabel yang sama dengan yang pertama. Kemungkinan besar, kesalahan muncul karena penyalinan kode yang gagal.

Fragmen 2

V760 Dua blok teks identik ditemukan. Blok kedua dimulai dari baris 771.tsg.c 770

typedef struct _TSG_PACKET_VERSIONCAPS
{
  ....
  UINT16 majorVersion;
  UINT16 minorVersion;
  ....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;

static BOOL TsProxyCreateTunnelReadResponse(....)
{
  ....
  PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Kesalahan ketik lainnya: komentar kode menyiratkan bahwa utas harus datang Versi kecil, namun, pembacaan terjadi pada variabel bernama Versi utama. Namun, saya tidak paham dengan protokolnya, jadi ini hanya tebakan.

Fragmen 3

V524 Anehnya, isi fungsi 'trio_index_last' sepenuhnya setara dengan isi fungsi 'trio_index'. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Dilihat dari komentarnya, fungsinya trio_index menemukan karakter pertama yang cocok dalam sebuah string kapan trio_index_last - hal terakhir. Namun fungsi-fungsi ini identik! Kemungkinan besar ini salah ketik, dan pada fungsinya trio_index_last perlu digunakan strrr daripada strchr. Maka perilaku tersebut akan diharapkan.

Fragmen 4

V769 Penunjuk 'data' dalam ekspresi sama dengan nullptr. Nilai yang dihasilkan dari operasi aritmatika pada penunjuk ini tidak masuk akal dan tidak boleh digunakan. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Sepertinya operator negasi tidak sengaja terlewat di sini ! Dekat data. Sungguh aneh bahwa hal ini luput dari perhatian.

Fragmen 5

V517 Penggunaan pola 'if (A) {…} else if (A) {…}' terdeteksi. Ada kemungkinan adanya kesalahan logis. Periksa baris: 213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;

  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

Dua syarat terakhir sama: ternyata ada yang lupa mengeceknya setelah disalin. Terlihat dari kode bahwa bagian terakhir bekerja dengan nilai empat byte, jadi kita dapat berasumsi bahwa kondisi terakhir seharusnya demikian nilai <= 0x3FFFFFFFF.

Kesalahan lain dari jenis ini ditemukan:

  • V517 Penggunaan pola 'if (A) {…} else if (A) {…}' terdeteksi. Ada kemungkinan adanya kesalahan logis. Periksa baris: 169, 173.file.c 169

Validasi data masukan

Fragmen 1

V547 Ekspresi 'strcat(target, source) != NULL' selalu benar. triostr.c 425

TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
     char *target,
     TRIO_CONST char *source)
{
  assert(target);
  assert(source);
  
  return (strcat(target, source) != NULL);
}

Pengecekan hasil fungsi pada contoh ini salah. Fungsi strcat mengembalikan pointer ke versi final string, mis. parameter pertama lolos. Dalam hal ini memang demikian target. Namun jika setara NULL, maka sudah terlambat untuk memeriksanya, karena dalam fungsinya strcat itu akan direferensikan.

Fragmen 2

V547 Ekspresi 'cache' selalu benar. mesin terbang.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;

struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};

void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;

  if (cache)
  {
    ....
  }
  ....
}

Dalam hal ini variabel Cache alamat array statis ditetapkan glyphCache->glyphCache. Jadi, periksa jika (cache) dapat dihilangkan.

Kesalahan manajemen sumber daya

V1005 Sumber daya diperoleh menggunakan fungsi 'CreateFileA' tetapi dirilis menggunakan fungsi 'fclose' yang tidak kompatibel. sertifikat.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

Deskriptor file fp, dibuat oleh panggilan fungsi Buat File ditutup karena kesalahan fungsi tutup dari perpustakaan standar, tidak TutupHandle.

Kondisi yang sama

V581 Ekspresi kondisional dari pernyataan 'jika' yang terletak bersebelahan adalah identik. Periksa baris: 269, 283.ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }

  if (conformant_array_description)
  {
    unsigned char array_type;
    array_type = conformant_array_description[0];
    pStubMsg->MaxCount = MaxCount;
    pStubMsg->ActualCount = ActualCount;
    pStubMsg->Offset = Offset;
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
  }
  ....
}

Contoh ini mungkin bukan bug. Namun, kedua kondisi tersebut berisi pesan yang sama, salah satunya kemungkinan besar dapat dihapus.

Membersihkan pointer nol

V575 Pointer nol diteruskan ke fungsi 'gratis'. Periksa argumen pertama. kartu pintar_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */

  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);

  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);

  if (status == SCARD_S_SUCCESS)
  {
    ....
  }

  free(mszGroupsA);
  ....
}

Dalam fungsi gratis Anda dapat memberikan pointer nol dan penganalisis mengetahuinya. Namun jika terdeteksi situasi di mana penunjuk selalu memberikan nilai nol, seperti dalam cuplikan ini, peringatan akan dikeluarkan.

penunjuk mszGroupA awalnya sama NULL dan tidak diinisialisasi di tempat lain. Satu-satunya cabang kode tempat penunjuk dapat diinisialisasi tidak dapat dijangkau.

Ada pesan lain seperti:

  • V575 Pointer null diteruskan ke fungsi 'gratis'. Periksa argumen pertama. lisensi.c 790
  • V575 Pointer null diteruskan ke fungsi 'gratis'. Periksa argumen pertama. rdpsnd_alsa.c 575

Kemungkinan besar, variabel yang terlupakan tersebut muncul selama proses pemfaktoran ulang dan dapat dihilangkan begitu saja.

Kemungkinan meluap

V1028 Kemungkinan meluap. Pertimbangkan untuk mentransmisikan operan, bukan hasilnya. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);

struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};

typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;

int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Membawa hasilnya ke panjang bukan proteksi overflow, karena penghitungannya sendiri dilakukan menggunakan tipe int.

Dereferensi penunjuk dalam inisialisasi

V595 Penunjuk 'konteks' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 746, 748.gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;

  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

Inilah penunjuknya konteks didereferensi selama inisialisasi - sebelum diperiksa.

Kesalahan lain dari jenis ini ditemukan:

  • V595 Pointer 'ntlm' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 236, 255.ntlm.c 236
  • V595 Penunjuk 'konteks' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 1003, 1007. rfx.c 1003
  • V595 Penunjuk 'rdpei' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 176, 180.rdpei_main.c 176
  • V595 Penunjuk 'gdi' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 121, 123.xf_gfx.c 121

Kondisi tidak berarti

V547 Ekspresi 'rdp->state >= CONNECTION_STATE_ACTIVE' selalu benar. koneksi.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // <=
      ....
      if (rdp->state >= CONNECTION_STATE_ACTIVE)     // <=
      {
        IFCALLRET(client->Activate, client->activated, client);

        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

Sangat mudah untuk melihat bahwa kondisi pertama tidak ada artinya karena nilai terkait telah diberikan sebelumnya.

Penguraian string salah

V576 Formatnya salah. Pertimbangkan untuk memeriksa argumen ketiga sebenarnya dari fungsi 'sscanf'. Diharapkan ada penunjuk ke tipe int unsigned. proxy.c 220

V560 Bagian dari ekspresi kondisional selalu benar: (rc >= 0). proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);

  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

Penganalisis segera mengeluarkan 2 peringatan untuk fragmen ini. Penentu %u mengharapkan variabel bertipe tidak ditandatangani, tetapi bervariasi di bawah memiliki tipe int. Selanjutnya kita melihat pemeriksaan yang mencurigakan: kondisi di sebelah kanan tidak masuk akal, karena di awal ada perbandingan dengan yang satu. Saya tidak tahu apa maksud pembuat kode ini, tetapi jelas ada sesuatu yang salah di sini.

Pemeriksaan yang tidak sesuai pesanan

V547 Ekspresi 'status == 0x00090314' selalu salah. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }

  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

Kondisi yang diperiksa akan selalu salah, karena eksekusi hanya akan mencapai kondisi kedua jika status == SEC_E_OK. Kode yang benar mungkin terlihat seperti ini:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

Kesimpulan

Jadi, pemeriksaan proyek mengungkapkan banyak masalah, tetapi hanya bagian paling menarik yang dijelaskan dalam artikel. Pengembang proyek dapat memeriksa sendiri proyek tersebut dengan meminta kunci lisensi sementara di situs web PVS-Studio. Ada juga kesalahan positif, pekerjaan yang akan membantu meningkatkan penganalisis. Namun, analisis statis penting jika Anda tidak hanya ingin meningkatkan kualitas kode Anda, tetapi juga mengurangi waktu yang dihabiskan untuk menemukan kesalahan, dan PVS-Studio dapat membantu dalam hal ini.

Memeriksa FreeRDP menggunakan penganalisa PVS-Studio

Jika Anda ingin membagikan artikel ini kepada audiens berbahasa Inggris, silakan gunakan tautan terjemahan: Sergey Larin. Memeriksa FreeRDP dengan PVS-Studio

Sumber: www.habr.com

Beli hosting yang andal untuk situs dengan perlindungan DDoS, server VPS VDS 🔥 Beli hosting website andal dengan perlindungan DDoS, server VPS VDS | ProHoster