FreeRDP – an open implementation of the Remote Desktop Protocol (RDP) developed by Microsoft for remote computer management. The project supports multiple platforms, including Windows, Linux, macOS, and even iOS with Android. This project has been chosen first in a series of articles dedicated to testing RDP clients using the static analyzer PVS-Studio.
A Bit of History
Project emerged after Microsoft opened its proprietary RDP protocol specifications. At that time, there was the rdesktop client, whose implementation was based on results from Reverse Engineering.
During the protocol implementation, it became increasingly difficult to add new functionality due to the existing project architecture. Changes in it led to conflict among developers, resulting in the creation of a fork from rdesktop — FreeRDP. Further distribution of the product was limited by the GPLv2 license, leading to the decision to relicense it under the Apache License v2. However, not everyone agreed to change their code's license, so developers decided to rewrite the project, resulting in what we have today as a modern codebase.
For more details about the project's history, you can read in the official blog note: "The history of the FreeRDP project."
As a tool for identifying bugs and potential vulnerabilities in the code, the static code analyzer for C, C++, C#, and Java, available on Windows, Linux, and macOS.
The article presents only those errors that I found to be most interesting.
Memory leak
The function was exited without releasing the 'cwd' pointer. A memory leak is possible. 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;
....
}This fragment was taken from the winpr subsystem, which implements a WINAPI wrapper for non-Windows systems, i.e., it is a lightweight alternative to Wine. Here, a leak can be noticed: memory allocated by the function getcwd, is freed only when handling special cases. To fix the error, a call to free after memcpy.
Array out of bounds
Array overrun is possible. The value of 'event->EventHandlerCount' index could reach 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++;
}
....
}In this example, a new element is added to the list, even if the number of elements has reached the maximum. Here, it is enough to replace the operator <= to <, to avoid going out of bounds of the array.
Another error of this type was found:
- V557 Array overrun is possible. The value of ‘iBitmapFormat’ index could reach 8. orders.c 2623
Typos
Fragment 1
Expression ‘!pipe->In’ is always 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;
....
}Here we see a typical typo: in the second condition, the same variable is checked as in the first. Most likely, the error occurred due to an unsuccessful code copy.
Fragment 2
Two identical blocks of text were found. The second block begins from line 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);
....
}Another typo: the comment indicates that data should come from the stream minorVersion, however, the reading occurs into a variable named majorVersion. Nevertheless, I am not familiar with the protocol, so this is just a guess.
Fragment 3
It is odd that the body of ‘trio_index_last’ function is fully equivalent to the body of ‘trio_index’ function. 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);
}Judging by the comment, the function trio_index finds the first occurrence of a character in a string, while trio_index_last is supposed to find the last one. But the bodies of these functions are identical! Most likely, it's a typo, and the function trio_index_last needs to use strrchr instead of strchr. Then the behavior would be as expected.
Fragment 4
The ‘data’ pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. 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;
....
}It seems that the negation operator was accidentally missed ! next to data. It is strange that this went unnoticed.
Fragment 5
The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 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);
}
....
}The last two conditions are the same: evidently, someone forgot to check them after copying. The code indicates that the last part deals with four-byte values, so it can be assumed that the last condition should be value <= 0x3FFFFFFF.
Another error of this type was found:
- V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 169, 173. file.c 169
Input data check
Fragment 1
Expression ‘strcat(target, source) != NULL’ is always true. 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);
}The function's result check in this example is incorrect. The function strcat returns a pointer to the final version of the string, i.e., the first parameter passed. In this case, it is target. However, if it is equal to NULL, checking it is too late since it will be dereferenced in the function strcat .
Fragment 2
Expression ‘cache’ is always true. 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)
{
....
}
....
}In this case, the variable cache is assigned the address of a static array glyphCache->glyphCache. Therefore, the check if (cache) can be omitted.
Resource management error
The resource was acquired using ‘CreateFileA’ function but was released using incompatible ‘fclose’ function. 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;
}
....
}File descriptor fp, created by the call to function CreateFile, was mistakenly closed by the function fclose from the standard library, instead of CloseHandle.
Identical conditions
The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 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: "
"0xX 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: "
"0xX unimplemented", array_type);
}
....
}This example may not be an error. However, both conditions contain identical messages, one of which can likely be removed.
Clearing null pointers
The null pointer is passed into ‘free’ function. Inspect the first argument. smartcard_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);
....
}In the function free It is possible to pass a null pointer, and the analyzer is aware of it. However, if a scenario arises where the pointer is consistently passed as null, as in this fragment, a warning will be issued.
Pointer mszGroupsA initially equals NULL and is not initialized anywhere else. The only code branch where the pointer could be initialized is unreachable.
There were other messages of this type:
- V575 The null pointer is passed into ‘free’ function. Inspect the first argument. license.c 790
- V575 The null pointer is passed into ‘free’ function. Inspect the first argument. rdpsnd_alsa.c 575
Such forgotten variables likely arise during refactoring and can often be simply removed.
Possible overflow
Possible overflow. Consider casting operands, not the result. 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));
....
}Casting the result to long is not a safeguard against overflow, as the computation itself uses the type int.
Dereferencing the pointer upon initialization
The ‘context’ pointer was utilized before it was verified against nullptr. Check lines: 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;
....
}Here the pointer context is dereferenced during initialization — before its validity is checked.
Other errors of this type were found:
- V595 The ‘ntlm’ pointer was utilized before it was verified against nullptr. Check lines: 236, 255. ntlm.c 236
- V595 The ‘context’ pointer was utilized before it was verified against nullptr. Check lines: 1003, 1007. rfx.c 1003
- V595 The ‘rdpei’ pointer was utilized before it was verified against nullptr. Check lines: 176, 180. rdpei_main.c 176
- V595 The ‘gdi’ pointer was utilized before it was verified against nullptr. Check lines: 121, 123. xf_gfx.c 121
Meaningless condition
Expression ‘rdp->state >= CONNECTION_STATE_ACTIVE’ is always true. 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;
}
....
}
....
}It is easy to notice that the first condition is nonsense due to the assignment of the corresponding value earlier.
Incorrect string parsing
Incorrect format. Consider checking the third actual argument of the ‘sscanf’ function. A pointer to the unsigned int type is expected. proxy.c 220
A part of the conditional expression is always true: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}The analyzer for this fragment gives two warnings immediately. The specifier %u expects a variable of type unsigned int, but the variable sub is of type int. Next, we see a suspicious check: the condition on the right makes no sense as it starts with a comparison to one. I don't know what the author of this code meant, but something is clearly off here.
Unordered checks
Expression ‘status == 0x00090314’ is always false. 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;
....
}The marked conditions will always be false, as execution will reach the second condition only when status == SEC_E_OK. The correct code might look like this:
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;
}Conclusion
Thus, the project review revealed many issues, but only the most interesting ones were described in the article. Project developers can check the project themselves by requesting a temporary license key on the website. There were also false positives, and working on them will help improve the analyzer. Nevertheless, static analysis is important if you want not only to enhance code quality but also to reduce debugging time, and PVS-Studio can assist with this.
If you want to share this article with an English-speaking audience, please use the link to the translation: Sergey Larin.
Source: habr.com
