Geekly Articles each Day
⬆️ ⬇️

Checking FreeRDP using PVS-Studio analyzer

Picture 2


FreeRDP is an open source implementation of the Remote Desktop Protocol (RDP), a protocol that implements remote computer control developed by Microsoft. The project supports many platforms, including Windows, Linux, macOS, and even iOS with Android. This project is the first to be selected as part of a series of articles on testing RDP clients using the PVS-Studio static analyzer.



A bit of history



The FreeRDP project appeared after Microsoft opened the specifications of its proprietary RDP protocol. At that time, there was an rdesktop client, the implementation of which is based on the results of Reverse Engineering.



In the process of implementing the protocol, it became more difficult to add new functionality due to the existing project architecture. Changes in it caused a conflict between developers, which led to the creation of the fork rdesktop - FreeRDP. Further distribution of the product was limited to the GPLv2 license, as a result of which it was decided to relicensing against Apache License v2. However, not everyone agreed to change the license of their code, so the developers decided to rewrite the project, with the result that we have a modern look of the code base.



You can read more about the history of the project in the official blog note: “The history of the FreeRDP project”.

')

PVS-Studio was used as a tool to detect errors and potential vulnerabilities in the code. This is a static code analyzer for C, C ++, C # and Java languages, available on Windows, Linux and macOS platforms.



The article presents only those errors that seemed to me the most interesting.



Memory leak



V773 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 that implements the WINAPI wrapper for non-Windows systems, i.e. This is an easy analogue of Wine. Here you can see a leak: the memory allocated by the getcwd function is released only when handling special cases. To eliminate the error, you need to add a free call after memcpy .



Array bounds



V557 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 item is added to the list, even if the number of items has reached its maximum. Here it is enough to replace the operator <= by < , in order not to go beyond the bounds of the array.



Another error of this type was found:





Typos



Fragment 1



V547 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 the usual typo: in the second condition, the same variable is checked as in the first one. Most likely, the error appeared as a result of unsuccessful copying of the code.



Fragment 2



V760 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 code comment implies that a minorVersion should come from the stream, but the readout takes place in a variable named majorVersion . However, I am not familiar with the protocol, so this is only an assumption.



Fragment 3



V524 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 trio_index function finds the first match of a character in a string, when trio_index_last is the last. But the bodies of these functions are identical! Most likely, this is a typo, and in the trio_index_last function you need to use strrchr instead of strchr . Then the behavior will be expected.



Fragment 4



V769 The 'data' pointer in the expression equals nullptr. This should be the case. 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 here ! next to data . It is strange that it went unnoticed.



Fragment 5



V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 <= 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); } .... }


The last two conditions are the same: apparently, someone forgot to check them after copying. By code, it is noticeable that the last part works with four-byte values, so we can assume that the last condition must be value <= 0x3FFFFFFF .



Another error of this type was found:





Input Validation



Fragment 1



V547 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); }


Checking the result of the function in this example is incorrect. The strcat function returns a pointer to the final string, i.e. first passed parameter In this case, this is the target . However, if it is NULL , then it is too late to check it, since its dereferencing will occur in the strcat function.



Fragment 2



V547 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 cache variable is assigned the address of the static array glyphCache-> glyphCache . Thus, if (cache) check can be omitted.



Resource Management Error



V1005 The resource was acquired using the 'CreateFileA' function but it 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; } .... }


The file descriptor fp , created by a call to the CreateFile function , is mistakenly closed by the fclose function from the standard library, not CloseHandle .



Same conditions



V581 The conditional expressions of the statement "if" statements followed 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: " "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); } .... }


Perhaps this example is not an error. However, both conditions contain the same message, one of which, most likely, can be removed.



Cleaning null pointers



V575 The null pointer is passed to the '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 free function, you can pass a null pointer and the analyzer knows about it. But if there is a situation in which the pointer is always transmitted as null, as in this fragment, a warning will be issued.



The mszGroupsA pointer is initially NULL and is not initialized anywhere else. The only branch of the code where the pointer could be initialized is unreachable.



There were other messages like this:





Most likely, such forgotten variables arise during the refactoring process and can be simply deleted.



Possible overflow



V1028 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)); .... }


Reduction of the result to a long is not an overflow protection, since the calculation itself takes place using the int type.



Pointer dereferencing in initialization



V595 context context pointer context against 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 context pointer is dereferenced in initialization — before it is checked.



Other errors of this type were found:





Meaningless condition



V547 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; // <= .... if (rdp->state >= CONNECTION_STATE_ACTIVE) // <= { IFCALLRET(client->Activate, client->activated, client); if (!client->activated) return -1; } .... } .... }


It is easy to see that the first condition does not make sense due to the assignment of the corresponding value earlier.



Invalid line parsing



V576 Incorrect format. Consider checking the sscanf function. A pointer to the unsigned int type is expected. proxy.c 220



V560 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 immediately gives 2 warnings. The % u specifier 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 does not make sense, since at the beginning there is a comparison with the unit. I do not know what the author of this code meant, but there is clearly something wrong here.



Unordered checks



V547 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; .... }


These conditions will always be false, since the fulfillment 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 verification of the project revealed many problems, but only the most interesting part of them was described in the article. Project developers can check the project themselves by requesting a temporary license key on the PVS-Studio website. There were also false positives, working on which will help improve the analyzer. Nevertheless, static analysis is important if you want to not only improve the quality of the code, but also reduce the time for finding errors, and PVS-Studio can help with this.







If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Larin. Checking FreeRDP with PVS-Studio

Source: https://habr.com/ru/post/444242/

More articles:



All Articles