FreeRDP – отворена реализация Remote Desktop Protocol (RDP), протокол, реализиращ отдалено управление на компютър, разработен от Microsoft. Проектът поддържа множество платформи, среди които Windows, Linux, macOS и дори iOS с Android. Този проект е избран първи в рамките на цикъл статии, посветени на проверката на RDP клиенти с помощта на статичен анализатор PVS-Studio.
Малко история
Проект появи се след като Microsoft отвори спецификациите на собствения си проприетарен протокол RDP. В онзи момент съществуваше клиент rdesktop, чиято реализация е базирана на резултатите от объркано инженерство.
В процеса на реализация на протокола ставаше все по-трудно да се добавя нов функционал поради съществуващата тогава архитектура на проекта. Промените в нея породиха конфликт между разработчиците, което доведе до създаването на форк rdesktop – FreeRDP. По-нататъшното разпространение на продукта беше ограничено от лицензия GPLv2, в резултат на което беше взето решение за прелицензиране на Apache License v2. Въпреки това не всички бяха съгласни да променят лиценза на кода си, затова разработчиците решиха да пренапишат проекта, в резултат на което имаме съвременен вид на кодовата база.
По-подробно за историята на проекта можете да прочетете в бележката на официалния блог: „Историята на проекта FreeRDP“.
Като инструмент за откриване на грешки и потенциални уязвимости в кода беше използван . Това е статичен анализатор на кода за езици C, C++, C# и Java, достъпен на платформите Windows, Linux и macOS.
В статията са представени само тези грешки, които ми се струваха най-интелигентни.
Изтичане на памет
Функцията е напусната без освобождаване на указателя 'cwd'. Възможно е изтичане на памет. 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;
....
}Този фрагмент е взет от подсистемата winpr, която реализира обвивка на WINAPI за не-Windows системи, т.е. това е лек аналог на Wine. Тук можем да забележим изтичане: паметта, заделена от функцията getcwd, се освобождава само при обработка на специални случаи. За да се отстрани грешката, трябва да добавим извикване на free след memcpy.
Излизане извън границите на масива
Възможно е да се случи излизане извън границите на масива. Стойността на индекса 'event->EventHandlerCount' може да достигне 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++;
}
....
}В този пример новият елемент се добавя в списъка, дори и когато броят на елементите е достигнал максимума. Достатъчно е да замените оператора <= на <, за да не излезе извън границите на масива.
Намерена е и друга грешка от същия тип:
- V557 Възможно е препълване на масива. Стойността на индекса ‘iBitmapFormat’ може да достигне 8. orders.c 2623
Типографски грешки
Фрагмент 1
Изразът ‘!pipe->In’ винаги е false. 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;
....
}Тук виждаме обикновена типографска грешка: във второто условие се проверява същата променлива, както и в първото. Най-вероятно грешката е резултат на неуспешно копиране на кода.
Фрагмент 2
Намерени са два идентични текстови блока. Вторият блок започва от ред 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);
....
}Още една типографска грешка: коментарът към кода предполага, че от потока трябва да дойде minorVersion, но четенето става в променлива с името majorVersion. Все пак, не съм запознат с протокола, така че това е само предположение.
Фрагмент 3
Странно е, че тялото на функцията ‘trio_index_last’ е напълно еквивалентно на тялото на функцията ‘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);
}Съдейки по коментара, функцията trio_index намеря първото съвпадение на символ в низ, докато trio_index_last — последното. Но телата на тези функции са идентични! Най-вероятно това е типографска грешка и в функцията trio_index_last трябва да се използва strrchr вместо strchr. Тогава поведението ще бъде очаквано.
Фрагмент 4
Указателят ‘data’ в израза е равен на nullptr. Получената стойност от аритметичните операции върху този указател е безсмислена и не трябва да се използва. 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;
....
}Изглежда, че тук случайно е пропуснат операторът за отрицание ! близо до data. Странно е, че това е останало незабелязано.
Фрагмент 5
Открит е модел за употреба ‘if (A) {…} else if (A) {…}’. Съществува вероятност за наличие на логическа грешка. Проверете редове: 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);
}
....
}Последните две условия са идентични: очевидно, някой е забравил да ги провери след копирането. От кода е видно, че последната част работи с четирибайтови стойности, така че може да се предположи, че последното условие трябва да бъде value <= 0x3FFFFFFF.
Намерена е и друга грешка от същия тип:
- V517 Открит е шаблон с ‘if (A) {...} else if (A) {...}’. Съществува вероятност за логическа грешка. Провери редове: 169, 173. file.c 169
Проверка на входните данни
Фрагмент 1
Изразът ‘strcat(target, source) != NULL’ винаги е верен. 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);
}Проверката за резултата от изпълнението на функцията в този пример е некоректна. Функцията strcat връща указател към крайния вариант на стринга, т.е. първия предаден параметър. В този случай това е target. Обаче, ако той е равен на NULL, то проверката му е късно, тъй като в функцията strcat ще се случи разименуването му.
Фрагмент 2
Изразът ‘cache’ винаги е верен. 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)
{
....
}
....
}В този случай на променливата cache се присвоява адрес на статичен масив glyphCache->glyphCache. Така проверката if (cache) може да бъде пропусната.
Грешка в управлението на ресурсите
Ресурсът е получен чрез функцията ‘CreateFileA’, но е освободен с несъвместимата функция ‘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;
}
....
}Дескрипторът на файла fp, създаден чрез извикването на функцията CreateFile, по грешка е затворен с функцията fclose от стандартната библиотека, а не CloseHandle.
Идентични условия
Условните изрази на операторите ‘if’, разположени един до друг, са идентични. Провери редове: 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, "внимание: NdrComplexStructBufferSize array_type: "
"0xX не е реализирано", 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, "внимание: NdrComplexStructBufferSize array_type: "
"0xX не е реализирано", array_type);
}
....
}Възможно е този пример да не е грешка. Въпреки това и двете условия съдържат идентични съобщения, едното от които вероятно може да бъде премахнато.
Премахване на нулеви указатели
Нулевият указател е предаден на функцията ‘free’. Проверете първия аргумент. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL;
/* mszGroups не се поддържа от 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);
....
}В функция free може да се предаде нулев указател и анализаторът знае за това. Но ако се установи ситуация, при която указателят винаги се предава като нулев, както в този фрагмент, ще бъде генерирано предупреждение.
Указател mszGroupsA първоначално е равен на NULL и не се инициализира никъде другаде. Единствената клонова част от кода, където указателят може да бъде инициализиран, е недостижима.
Имаше и други съобщения от този тип:
- V575 Нулевият указател е предаден на функцията ‘free’. Проверете първия аргумент. license.c 790
- V575 Нулевият указател е предаден на функцията ‘free’. Проверете първия аргумент. rdpsnd_alsa.c 575
Вероятно подобни забравени променливи възникват по време на рефакториране и могат просто да бъдат премахнати.
Възможно препълване
Възможно препълване. Обмислете преобразуването на операндите, а не на резултата. 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));
....
}Преобразуването на резултата в long не е защита срещу препълване, тъй като самото изчисление се извършва с тип int.
Разименуване на указателя в инициализация
Указателят ‘context’ беше използван преди да бъде проверен срещу nullptr. Проверете редове: 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;
....
}Тук указателят контекст разыменова се в инициализацията — преди проверката му.
Намерени са и други подобни грешки:
- V595 Указателят ‘ntlm’ е използван преди да бъде проверен за nullptr. Проверете редове: 236, 255. ntlm.c 236
- V595 Указателят ‘context’ е използван преди да бъде проверен за nullptr. Проверете редове: 1003, 1007. rfx.c 1003
- V595 Указателят ‘rdpei’ е използван преди да бъде проверен за nullptr. Проверете редове: 176, 180. rdpei_main.c 176
- V595 Указателят ‘gdi’ е използван преди да бъде проверен за nullptr. Проверете редове: 121, 123. xf_gfx.c 121
Безсмислено условие
Изразът ‘rdp->state >= CONNECTION_STATE_ACTIVE’ винаги е истинен. 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;
}
....
}
....
}Лесно може да се забележи, че първото условие няма смисъл, заради присвояването на съответната стойност по-рано.
Некоректен анализ на стринг
Некоректен формат. Проверьте третия актуален аргумент на функцията ‘sscanf’. Очаква се указател към unsigned int. proxy.c 220
Част от условното изразяване винаги е истина: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Анализаторът за този фрагмент дава веднага 2 предупреждения. Спецификатор %u очаква променлива от тип unsigned int, но променливата sub има тип int. След това виждаме съмнителна проверка: условието отдясно няма смисъл, тъй като в началото е сравнение с единицата. Не знам какво е имал предвид авторът на този код, но явно нещо не е наред.
Несортирани проверки
Изразът ‘status == 0x00090314’ винаги е невярен. 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;
....
}Отбелязаните условия винаги ще бъдат неверни, тъй като изпълнението ще достигне второто условие само когато status == SEC_E_OK. Правилният код може да изглежда така:
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;
}Заключение
По този начин проверката на проекта разкри множество проблем, но само най-интригуващата им част е описана в статията. Разработчиците на проекта могат сами да проверят проекта, като поискат временно лицензионно ключе от сайта. . Имаше и фалшиви сработвания, работата по които ще помогне за подобряване на анализатора. Въпреки това, статичният анализ е важен, ако искате не само да увеличите качеството на кода, но и да намалите времето за търсене на грешки, а PVS-Studio може да помогне в това.
Ако искате да споделите тази статия с англоезичната аудитория, моля, използвайте линка към превода: Сергей Ларин.
Източник: habr.com
