FreeRDP to implementacja protokołu RDP (Remote Desktop Protocol) o otwartym kodzie źródłowym, opracowana przez firmę Microsoft do zdalnego sterowania komputerem. Projekt obsługuje wiele platform, w tym: Windows, Linux, macOS i nawet iOS z AndroidProjekt ten został wybrany jako pierwszy z serii artykułów poświęconych testowaniu klientów RDP przy użyciu statycznego analizatora PVS-Studio.
Trochę historii
Projekt pojawiło się po tym, jak Microsoft udostępnił specyfikacje swojego zastrzeżonego protokołu RDP. Istniał wówczas klient rdesktop, którego implementacja opierała się na wynikach inżynierii odwrotnej.
W miarę wdrażania protokołu dodawanie nowych funkcjonalności stawało się coraz trudniejsze ze względu na istniejącą wówczas architekturę projektu. Zmiany w nim wprowadzone spowodowały konflikt między programistami, co doprowadziło do stworzenia forka rdesktop - FreeRDP. Dalsza dystrybucja produktu była ograniczona licencją GPLv2, w związku z czym podjęto decyzję o zmianie licencji na Apache License v2. Jednak nie wszyscy zgodzili się na zmianę licencji swojego kodu, więc programiści postanowili napisać projekt od nowa, w wyniku czego mamy współczesną postać bazy kodu.
Więcej na temat historii projektu można przeczytać w oficjalnym wpisie na blogu: „Historia projektu FreeRDP”.
Jako narzędzie do identyfikacji błędów i potencjalnych luk w kodzie, Jest to statyczny analizator kodu dla języków C, C++, C# i Java, dostępny na platformach Windows, Linux и macOS.
W artykule przedstawiono tylko te błędy, które wydały mi się najciekawsze.
Wyciek pamięci
Funkcja została zakończona bez zwalniania wskaźnika „cwd”. Możliwy jest wyciek pamięci. środowisko.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;
....
}Ten fragment pochodzi z podsystemu winpr, który implementuje opakowanie WINAPI dla nie-Windows systems, czyli lekki odpowiednik Wine. Tutaj widać wyciek: pamięć przydzielona przez funkcję dostaćcwd, jest zwalniany tylko w przypadku obsługi szczególnych przypadków. Aby naprawić błąd, musisz dodać wywołanie za darmo później memcpy.
Tablica poza zakresem
Możliwe jest przepełnienie tablicy. Wartość indeksu 'event->EventHandlerCount' może osiągnąć 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++;
}
....
}W tym przykładzie nowy element jest dodawany do listy, nawet jeśli liczba elementów osiągnęła maksimum. Tutaj wystarczy wymienić operatora <= na <, aby nie wyjść poza granice tablicy.
Znaleziono kolejny błąd tego typu:
- V557 Możliwe jest przepełnienie tablicy. Wartość indeksu „iBitmapFormat” może osiągnąć 8. orders.c 2623
Literówki
Fragment 1
Wyrażenie '!pipe->In' jest zawsze fałszywe. MessagePipe.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;
....
}Tutaj widzimy częsty błąd literowy: drugi warunek sprawdza tę samą zmienną, co pierwszy. Najprawdopodobniej błąd pojawił się na skutek nieudanego skopiowania kodu.
Fragment 2
Znaleziono dwa bloki identycznego tekstu. Drugi blok zaczyna się od linii 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);
....
}Kolejna literówka: komentarz w kodzie sugeruje, że strumień powinien pochodzić z minorVersionjednak odczyt odbywa się do zmiennej o nazwie Wersja główna. Nie znam jednak tego protokołu, więc to tylko przypuszczenie.
Fragment 3
Dziwne jest to, że treść funkcji 'trio_index_last' jest całkowicie równoważna treści funkcji '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);
}Sądząc po komentarzu, funkcja indeks_trio znajduje pierwsze dopasowanie znaku w ciągu, gdy trio_index_ostatni — ostatni. Ale ciała tych funkcji są identyczne! Najprawdopodobniej jest to literówka i w funkcji trio_index_ostatni potrzebuję użyć strrrchr zamiast strchr. Wtedy zachowanie będzie zgodne z oczekiwaniami.
Fragment 4
Wskaźnik 'data' w wyrażeniu jest równy nullptr. Wynik operacji arytmetycznych na tym wskaźniku jest pozbawiony sensu i nie należy go używać. 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;
....
}Wygląda na to, że przypadkowo pominięto tu operator negacji. ! obok dane. Dziwne, że nikt tego nie zauważył.
Fragment 5
Wykryto użycie wzorca 'if (A) {…} else if (A) {…}'. Istnieje prawdopodobieństwo występowania błędu logicznego. Sprawdź wiersze: 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);
}
....
}Ostatnie dwa warunki są takie same: najwyraźniej ktoś zapomniał je sprawdzić po skopiowaniu. Z kodu widać, że ostatnia część działa z wartościami czterobajtowymi, więc można założyć, że ostatni warunek powinien być wartość <= 0x3FFFFFFFF.
Znaleziono kolejny błąd tego typu:
- V517 Wykryto użycie wzorca 'if (A) {…} else if (A) {…}'. Istnieje prawdopodobieństwo występowania błędu logicznego. Sprawdź linie: 169, 173. file.c 169
Walidacja danych wejściowych
Fragment 1
Wyrażenie 'strcat(target, source) != NULL' jest zawsze prawdziwe. 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);
}Sprawdzenie wyniku funkcji w tym przykładzie jest nieprawidłowe. Funkcjonować strcat zwraca wskaźnik do ostatecznej wersji ciągu, tj. pierwszego przekazanego parametru. W tym przypadku tak jest cel. Jednakże, jeśli jest to równe NULL, to jest już za późno, żeby to sprawdzić, ponieważ w funkcji strcat zostanie odwołany.
Fragment 2
Wyrażenie „cache” jest zawsze prawdziwe. glif.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)
{
....
}
....
}W tym przypadku zmienna Pamięć podręczna adres tablicy statycznej jest przypisany glyphCache->glyphCache. Tak więc kontrola jeśli (pamięć podręczna) można pominąć.
Błąd zarządzania zasobami
Zasób został pobrany za pomocą funkcji „CreateFileA”, ale zwolniony za pomocą niekompatybilnej funkcji „fclose”. certyfikat.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;
}
....
}Opis pliku fp, utworzone przez wywołanie funkcji UtwórzPlik, zamknięte z powodu błędu przez funkcję fzamknij ze standardowej biblioteki, nie Zamknij uchwyt.
Te same warunki
Wyrażenia warunkowe instrukcji „if” znajdujące się obok siebie są identyczne. Sprawdź wiersze: 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);
}
....
}Ten przykład może nie być błędny. Jednak oba warunki zawierają te same komunikaty, z których jeden najprawdopodobniej można usunąć.
Czyszczenie wskaźników null
Wskaźnik zerowy jest przekazywany do funkcji 'free'. Przyjrzyj się pierwszemu argumentowi. karta inteligentna_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);
....
}W funkcji za darmo Możesz przekazać wskaźnik null i analizator będzie o tym wiedział. Jeśli jednak wystąpi sytuacja, w której wskaźnik jest zawsze przekazywany jako null, jak w tym fragmencie, wyświetlone zostanie ostrzeżenie.
Wskaźnik mszGrupyA początkowo równy NULL i nie jest nigdzie indziej inicjalizowany. Jedyny kod, w którym można zainicjować wskaźnik, jest niedostępny.
Były też inne wiadomości tego typu:
- V575 Wskaźnik zerowy jest przekazywany do funkcji 'free'. Przyjrzyj się pierwszemu argumentowi. licencja.c 790
- V575 Wskaźnik zerowy jest przekazywany do funkcji 'free'. Przyjrzyj się pierwszemu argumentowi. rdpsnd_alsa.c 575
Najprawdopodobniej takie zapomniane zmienne powstają podczas refaktoryzacji i można je po prostu usunąć.
Możliwe przepełnienie
Możliwe przepełnienie. Rozważ rzutowanie operandów, a nie wyniku. zróbcert.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));
....
}Przyniesienie wyniku do długie nie jest zabezpieczony przed przepełnieniem, ponieważ samo obliczenie odbywa się przy użyciu typu int.
Dereferencja wskaźnika podczas inicjalizacji
Wskaźnik „kontekstu” został wykorzystany przed sprawdzeniem go względem nullptr. Sprawdź linie: 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;
....
}Oto wskaźnik kontekst jest dereferencjonowany podczas inicjalizacji - przed sprawdzeniem.
Znaleziono inne błędy tego typu:
- V595 Wskaźnik „ntlm” został wykorzystany przed sprawdzeniem go względem nullptr. Sprawdź wiersze: 236, 255. ntlm.c 236
- V595 Wskaźnik „kontekstu” został wykorzystany przed sprawdzeniem go względem nullptr. Sprawdź linie: 1003, 1007. rfx.c 1003
- V595 Wskaźnik „rdpei” został wykorzystany przed sprawdzeniem go względem nullptr. Sprawdź wiersze: 176, 180. rdpei_main.c 176
- V595 Wskaźnik „gdi” został wykorzystany przed sprawdzeniem go względem nullptr. Sprawdź wiersze: 121, 123. xf_gfx.c 121
Stan bez sensu
Wyrażenie 'rdp->state >= CONNECTION_STATE_ACTIVE' jest zawsze prawdziwe. połączenie.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;
}
....
}
....
}Łatwo zauważyć, że pierwszy warunek nie ma sensu ze względu na wcześniejsze przypisanie odpowiedniej wartości.
Nieprawidłowa analiza ciągu
Nieprawidłowy format. Rozważ sprawdzenie trzeciego rzeczywistego argumentu funkcji „sscanf”. Oczekiwany jest wskaźnik do typu unsigned int. proxy.c 220
Część wyrażenia warunkowego jest zawsze prawdziwa: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizator wydaje 2 ostrzeżenia dla tego fragmentu. Specyfikator %u oczekuje zmiennej typu niepodpisany, ale zmienne poniżej ma typ int. Następnie widzimy podejrzaną kontrolę: warunek po prawej stronie nie ma sensu, ponieważ zaczyna się od porównania z jedynką. Nie wiem, co autor tego kodu miał na myśli, ale coś tu jest ewidentnie nie tak.
Nieuporządkowane czeki
Wyrażenie 'status == 0x00090314' jest zawsze fałszywe. 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;
....
}Zaznaczone warunki będą zawsze fałszywe, ponieważ wykonanie osiągnie drugi warunek tylko wtedy, gdy stan == SEC_E_OK. Prawidłowy kod może wyglądać następująco:
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;
}wniosek
Przegląd projektu ujawnił więc wiele problemów, ale w artykule opisano tylko najciekawszą ich część. Twórcy projektu mogą sami sprawdzić projekt, prosząc na stronie internetowej o tymczasowy klucz licencyjny . Wystąpiły także fałszywe alarmy, których rozwiązanie pomoże udoskonalić analizator. Jednak analiza statyczna jest istotna, jeśli chcesz nie tylko poprawić jakość swojego kodu, ale także skrócić czas poświęcany na wyszukiwanie błędów. PVS-Studio może Ci w tym pomóc.
Jeśli chcesz udostępnić ten artykuł anglojęzycznej publiczności, skorzystaj z linku do tłumaczenia: Sergey Larin.
Źródło: www.habr.com
