FreeRDP – otwarta implementacja protokołu zdalnego pulpitu (RDP), protokołu, który umożliwia zdalne zarządzanie komputerem, opracowanego przez firmę Microsoft. Projekt wspiera wiele platform, w tym Windows, Linux, macOS, a nawet iOS i Android. Ten projekt został wybrany jako pierwszy w cyklu artykułów poświęconych testowaniu klientów RDP przy użyciu statycznego analizy PVS-Studio.
Historia
Projekt pojawił się po tym, jak Microsoft udostępnił specyfikacje swojego zastrzeżonego protokołu RDP. W tamtym czasie istniał klient rdesktop, którego implementacja opierała się na wynikach reverse engineering.
W miarę realizacji protokołu stawało się coraz trudniejsze dodawanie nowych funkcji z powodu istniejącej architektury projektu. Zmiany w niej spowodowały konflikt między programistami, co doprowadziło do powstania forka rdesktop – FreeRDP. Dalsze rozwijanie produktu zostało ograniczone licencją GPLv2, w wyniku czego podjęto decyzję o zmianie licencji na Apache License v2. Jednak nie wszyscy zgadzali się na zmianę licencji swojego kodu, w związku z czym programiści postanowili przebudować projekt, co zaowocowało nowoczesną bazą kodu.
Więcej informacji na temat historii projektu można znaleźć w notce oficjalnego bloga: „Historia projektu FreeRDP”.
Jako narzędzie do wykrywania błędów i potencjalnych luk w kodzie użyto . To statyczny analizator kodu dla języków C, C++, C# i Java, dostępny na platformach Windows, Linux i macOS.
W artykule przedstawione są tylko te błędy, które wydawały mi się najbardziej interesujące.
Wycieki pamięci
Funkcja została zakończona bez zwolnienia wskaźnika ‘cwd’. Możliwe jest wystąpienie wycieku pamięci. environment.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 systemów nie-Windows, tzn. jest to lekki odpowiednik Wine. Tutaj można zauważyć wyciek: pamięć przydzielona przez funkcję getcwd, jest zwalniana tylko w przypadku obsługi specjalnych sytuacji. Aby usunąć błąd, należy dodać wywołanie free po memcpy.
Przekroczenie granic tablicy
Możliwe jest przekroczenie granic 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 maksymalny limit. Wystarczy zastąpić operator <= na <, aby nie przekroczyć granic tablicy.
Znaleziono również inny błąd tego typu:
- V557 Możliwe przepełnienie tablicy. Wartość indeksu ‘iBitmapFormat’ może osiągnąć 8. orders.c 2623
Błędy pisowni
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 typowy błąd pisowni: w drugim warunku sprawdzana jest ta sama zmienna, co w pierwszym. Prawdopodobnie błąd powstał wskutek nieudanej kopii kodu.
Fragment 2
Znaleziono dwa identyczne bloki 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);
....
}Jeszcze jeden błąd pisowni: komentarz kodu sugeruje, że z potoku powinna nadejść minorVersion, jednak odczyt odbywa się w zmiennej o nazwie majorVersion. Niemniej jednak, nie znam się na protokole, więc to tylko przypuszczenie.
Fragment 3
Dziwne, że ciało funkcji ‘trio_index_last’ jest w pełni równoważne z ciałem 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);
}Z komentarza wynika, że funkcja trio_index znajduje pierwsze wystąpienie symbolu w ciągu, podczas gdy trio_index_last — to ostatnie. Ale ciała tych funkcji są identyczne! Prawdopodobnie to błąd pisowni, a funkcja trio_index_last trzeba użyć strrchr zamiast strchr. Wtedy zachowanie będzie zgodne z oczekiwaniami.
Fragment 4
Wskaźnik ‘data’ w wyrażeniu jest równy nullptr. Wynikowe wartości operacji arytmetycznych na tym wskaźniku są bezsensowne i nie powinny być używane. 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 operator negacji ! w pobliżu data. Dziwne, że to nie zostało zauważone.
Fragment 5
Wykryto wzór użycia ‘if (A) {…} else if (A) {…}’. Istnieje prawdopodobieństwo obecności błędu logicznego. Sprawdź linie: 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 > 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 > 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ą identyczne: najwyraźniej ktoś zapomniał ich sprawdzić po skopiowaniu. Z kodu widać, że ostatnia część obsługuje wartości czterobajtowe, więc można przypuszczać, że ostatni warunek powinien być value <= 0x3FFFFFFF.
Znaleziono również inny błąd tego typu:
- V517 Wykryto użycie wzoru ‘if (A) {…} else if (A) {…}’. Istnieje prawdopodobieństwo wystąpienia błędu logicznego. Sprawdź linie: 169, 173. file.c 169
Sprawdzanie 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);
}Weryfikacja wyniku funkcji w tym przykładzie jest niepoprawna. Funkcja strcat zwraca wskaźnik na końcową wersję ciągu, tzn. pierwszy przekazany parametr. W tym przypadku to target. Ale jeśli jest równy NULL, to sprawdzanie go jest spóźnione, ponieważ w funkcji strcat dojdzie do jego dereferencji.
Fragment 2
Wyrażenie ‘cache’ jest zawsze prawdziwe. glyph.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 takim przypadku zmiennej pamięć podręczna przypisywana jest adres statycznej tablicy glyphCache->glyphCache. Dlatego sprawdzenie if (cache) można pominąć.
Błąd zarządzania zasobami
Zasób został pozyskany za pomocą funkcji ‘CreateFileA’, ale został zwolniony za pomocą niekompatybilnej funkcji ‘fclose’. certificate.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;
}
....
}Deskryptor pliku fp, utworzony wywołaniem funkcji CreateFile, przez pomyłkę został zamknięty funkcją fclose z biblioteki standardowej, a nie CloseHandle.
Identyczne warunki
Wyrażenia warunkowe w instrukcjach ‘if’, które znajdują się obok siebie, są identyczne. Sprawdź linie: 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, "ostrzeżenie: NdrComplexStructBufferSize typ_tablicy: "
"0xX niezaimplementowane", 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, "ostrzeżenie: NdrComplexStructBufferSize typ_tablicy: "
"0xX niezaimplementowane", array_type);
}
....
}Możliwe, że ten przykład nie jest błędem. Jednak oba warunki zawierają te same komunikaty, z których jeden najprawdopodobniej można usunąć.
Czyszczenie wskaźników null
Wskaźnik null jest przekazywany do funkcji 'free'. Sprawdź pierwszy argument. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups nie jest obsługiwane przez 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);
....
}Do funkcji free można przekazać wskaźnik null, a analizator o tym wie. Ale jeśli wykryta zostanie sytuacja, w której wskaźnik zawsze jest przekazywany jako null, jak w tym fragmencie, wyświetli się ostrzeżenie.
Wskaźnik mszGroupsA początkowo równy NULL i nigdzie indziej nie jest inicjalizowany. Jedyną gałęzią kodu, w której wskaźnik mógł zostać zainicjowany, jest nieosiągalna.
Były i inne komunikaty tego typu:
- V575 Wskaźnik null jest przekazywany do funkcji 'free'. Sprawdź pierwszy argument. license.c 790
- V575 Wskaźnik null jest przekazywany do funkcji 'free'. Sprawdź pierwszy argument. rdpsnd_alsa.c 575
Najprawdopodobniej takie zapomniane zmienne powstają w trakcie refaktoryzacji i można je po prostu usunąć.
Możliwe przepełnienie
Możliwe przepełnienie. Rozważ rzutowanie operandów, a nie wyniku. 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));
....
}Rzutowanie wyniku na long nie jest ochroną przed przepełnieniem, ponieważ samo obliczenie odbywa się przy użyciu typu int.
Dereferencja wskaźnika w inicjalizacji
Wskaźnik 'context' był wykorzystywany, zanim został zweryfikowany w stosunku do 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;
....
}Wskaźnik tutaj context jest dereferencjonowany w inicjalizacji — zanim zostanie sprawdzony.
Znaleziono inne błędy tego typu:
- V595 Wskaźnik ‘ntlm’ został użyty przed jego weryfikacją względem nullptr. Sprawdź linie: 236, 255. ntlm.c 236
- V595 Wskaźnik ‘context’ został użyty przed jego weryfikacją względem nullptr. Sprawdź linie: 1003, 1007. rfx.c 1003
- V595 Wskaźnik ‘rdpei’ został użyty przed jego weryfikacją względem nullptr. Sprawdź linie: 176, 180. rdpei_main.c 176
- V595 Wskaźnik ‘gdi’ został użyty przed jego weryfikacją względem nullptr. Sprawdź linie: 121, 123. xf_gfx.c 121
Bezsensowne warunki
Wyrażenie ‘rdp->state >= CONNECTION_STATE_ACTIVE’ jest zawsze prawdziwe. connection.c 1489
int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
....
switch (state)
{
....
case CONNECTION_STATE_ACTIVE:
rdp->state = CONNECTION_STATE_ACTIVE; // state >= CONNECTION_STATE_ACTIVE) // Activate, client->activated, client);
if (!client->activated)
return -1;
}
....
}
....
}Łatwo zauważyć, że pierwsze warunek nie ma sensu z powodu przypisania odpowiedniej wartości wcześniej.
Niepoprawna analiza ciągu
Niepoprawny format. Sprawdź trzeci rzeczywisty argument 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 dla tego fragmentu zwraca od razu 2 ostrzeżenia. Specyfikator %u oczekuje zmiennej typu unsigned int, ale zmienna sub ma typ int. Następnie widzimy podejrzane sprawdzenie: warunek z prawej strony nie ma sensu, ponieważ na początku następuje porównanie z jedynką. Nie wiem, co miał na myśli autor tego kodu, ale coś jest zdecydowanie nie tak.
Nieuporządkowane kontrole
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 dojdzie do drugiego warunku tylko wtedy, gdy status == SEC_E_OK. Prawidłowy kod może wyglądać tak:
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;
}Podsumowanie
W związku z tym, przegląd projektu ujawnił wiele problemów, lecz tylko najciekawsza ich część została opisana w artykule. Programiści projektu mogą sami sprawdzić projekt, wnioskując tymczasowy klucz licencji na stronie . Były także fałszywe alarmy, nad którymi prace pomogą ulepszyć analityka. Niemniej jednak, analiza statyczna jest ważna, jeśli chcesz nie tylko poprawić jakość kodu, ale również skrócić czas poszukiwania błędów, a PVS-Studio może w tym pomóc.
Jeśli chcesz podzielić się tym artykułem z anglojęzyczną publicznością, proszę użyć linku do tłumaczenia: Sergey Larin.
Źródło: habr.com
