Comprobación de FreeRDP con el analizador PVS-Studio

FreeRDP es una implementación de código abierto del Protocolo de escritorio remoto (RDP), un protocolo que implementa el control remoto de la computadora desarrollado por Microsoft. El proyecto es compatible con muchas plataformas, incluidas Windows, Linux, macOS e incluso iOS con Android. Este proyecto fue seleccionado como el primero de una serie de artículos dedicados a verificar clientes RDP utilizando el analizador estático PVS-Studio.

Un poco de historia

El proyecto FreeRDP se produjo después de que Microsoft dio a conocer las especificaciones de su protocolo RDP patentado. En ese momento, había un cliente rdesktop, cuya implementación se basa en los resultados de la ingeniería inversa.

En el proceso de implementación del protocolo, se hizo más difícil agregar nueva funcionalidad debido a la arquitectura del proyecto existente en ese momento. Los cambios en él crearon un conflicto entre los desarrolladores, lo que condujo a la creación de la bifurcación rdesktop: FreeRDP. La distribución adicional del producto estaba limitada por la licencia GPLv2, como resultado de lo cual se tomó la decisión de volver a otorgar la licencia a Apache License v2. Sin embargo, no todos acordaron cambiar la licencia de su código, por lo que los desarrolladores decidieron reescribir el proyecto, por lo que tenemos una apariencia moderna de la base del código.

Se pueden encontrar más detalles sobre la historia del proyecto en la nota oficial del blog: "La historia del proyecto FreeRDP".

PVS-Studio se utilizó como herramienta para detectar errores y vulnerabilidades potenciales en el código. Es un analizador de código estático para C, C ++, C # y Java, disponible en Windows, Linux y macOS.

El artículo presenta solo aquellos errores que me parecieron los más interesantes.

Pérdida de memoria

V773 Se salió de la función sin soltar el puntero 'cwd'. Una pérdida de memoria es posible. 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; .... } 

Este fragmento fue tomado del subsistema winpr que implementa el contenedor WINAPI para sistemas que no son Windows, es decir. Es una contraparte ligera del vino. Aquí hay una fuga: la memoria asignada por la función getcwd se libera solo en casos especiales. Para corregir el error, agregue una llamada gratuita después de memcpy .

Ir más allá de los límites de una matriz.

V557 Array overrun es posible. El valor del índice 'event-> EventHandlerCount' podría alcanzar 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++; } .... } 

En este ejemplo, se agrega un nuevo elemento a la lista, incluso si el número de elementos ha alcanzado el máximo. Es suficiente reemplazar el operador <= con < para no ir más allá de los límites de la matriz.

Se encontró otro error de este tipo:

• V557 Array overrun es posible. El valor del índice 'iBitmapFormat' podría llegar a 8. orders.c 2623

Errores tipográficos

Fragmento 1

V547 La expresión '! Pipe-> In' siempre es falsa. 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; .... } 

Aquí vemos el error tipográfico habitual: en la segunda condición, se verifica la misma variable que en la primera. Lo más probable es que el error apareció como resultado de una copia fallida del código.

Fragmento 2

V760 Se encontraron dos bloques de texto idénticos. El segundo bloque comienza desde la línea 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); .... } 

Otro error tipográfico: un comentario sobre el código implica que minorVersion debe provenir de la transmisión, sin embargo, la lectura se lleva a cabo en una variable llamada majorVersion . Sin embargo, no estoy familiarizado con el protocolo, así que esto es solo una suposición.

Fragmento 3

V524 Es extraño que el cuerpo de la función 'trio_index_last' sea completamente equivalente al cuerpo de la función '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); } 

A juzgar por el comentario, la función trio_index encuentra la primera coincidencia de un carácter en una cadena cuando trio_index_last es la última. ¡Pero los cuerpos de estas funciones son idénticos! Lo más probable es que sea un error tipográfico, y en la función trio_index_last necesita usar strrchr en lugar de strchr . Entonces se esperará el comportamiento.

Fragmento 4

V769 El puntero de 'datos' en la expresión es igual a nullptr. El valor resultante de las operaciones aritméticas en este puntero no tiene sentido y no debe usarse. 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; .... } 

¡Parece que accidentalmente perdieron al operador de negación ! al lado de los datos . Es extraño que esto haya pasado desapercibido.

Fragmento 5

V517 El uso del patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 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); } .... } 

Las dos últimas condiciones son las mismas: aparentemente, alguien olvidó verificarlas después de copiar. De acuerdo con el código, es notable que la última parte funciona con valores de cuatro bytes, por lo que podemos suponer que la última condición debe ser el valor <= 0x3FFFFFFF .

Se encontró otro error de este tipo:

• V517 El uso del patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 169, 173. file.c 169

Validación de entrada

Fragmento 1

V547 La expresión 'strcat (target, source)! = NULL' siempre es verdadera. 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); } 

Verificar el resultado de la función en este ejemplo es incorrecto. La función strcat devuelve un puntero a la versión final de la cadena, es decir. primer parámetro pasado En este caso, es el objetivo . Sin embargo, si es NULL , entonces es demasiado tarde para verificar, ya que en la función strcat se desreferenciará.

Fragmento 2

La expresión 'caché' V547 siempre es verdadera. 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) { .... } .... } 

En este caso, a la variable de caché se le asigna la dirección de la matriz estática glyphCache-> glyphCache . Por lo tanto, la verificación if (caché) puede omitirse.

Error de gestión de recursos

V1005 El recurso se adquirió utilizando la función 'CreateFileA' pero se lanzó utilizando la función incompatible 'fclose'. certificado.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; } .... } 

El descriptor de archivo fp creado al llamar a la función CreateFile se cierra por error por la función fclose de la biblioteca estándar, no por CloseHandle .

Mismas condiciones

V581 Las expresiones condicionales de las declaraciones 'if' ubicadas una al lado de la otra son idénticas. Líneas de verificación: 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); } .... } 

Quizás este ejemplo no sea un error. Sin embargo, ambas condiciones contienen el mismo mensaje, uno de los cuales, muy probablemente, se puede eliminar.

Despeje de punteros nulos

V575 El puntero nulo se pasa a la función 'libre'. Inspecciona el primer argumento. 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); .... } 

Puede pasar un puntero nulo a la función libre , y el analizador lo sabe. Pero si se revela una situación en la que el puntero siempre se pasa a cero, como en este fragmento, se emitirá una advertencia.

El puntero mszGroupsA es inicialmente NULL y no se inicializa en ningún otro lugar. La única rama de código donde se puede inicializar el puntero es inalcanzable.

Hubo otras publicaciones como esta:

• V575 El puntero nulo se pasa a la función 'libre'. Inspecciona el primer argumento. licencia.c 790
• V575 El puntero nulo se pasa a la función 'libre'. Inspecciona el primer argumento. rdpsnd_alsa.c 575

Lo más probable es que tales variables olvidadas surjan durante la refactorización y simplemente se puedan eliminar.

Posible desbordamiento

V1028 Posible desbordamiento. Considere lanzar operandos, no el resultado. 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)); .... } 

Traer el resultado a largo no es protección contra el desbordamiento, ya que el cálculo en sí se realiza utilizando el tipo int .

Desreferencia de puntero en inicialización

V595 El puntero de 'contexto' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 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; .... } 

Aquí, el puntero de contexto se desreferencia en la inicialización, antes de que se verifique.

Se encontraron otros errores de este tipo:

• V595 El puntero 'ntlm' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 236, 255. ntlm.c 236
• V595 El puntero de 'contexto' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 1003, 1007. rfx.c 1003
• V595 El puntero 'rdpei' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 176, 180. rdpei_main.c 176
• V595 El puntero 'gdi' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 121, 123. xf_gfx.c 121

Condición sin sentido

V547 La expresión 'rdp-> state> = CONNECTION_STATE_ACTIVE' siempre es verdadera. 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; } .... } .... } 

Es fácil ver que la primera condición no tiene sentido debido a la asignación del valor correspondiente anteriormente.

Análisis de línea incorrecto

V576 Formato incorrecto. Considere verificar el tercer argumento real de la función 'sscanf'. Se espera un puntero al tipo int sin signo. proxy.c 220

V560 Una parte de la expresión condicional siempre es verdadera: (rc> = 0). proxy.c 222

 static BOOL check_no_proxy(....) { .... int sub; int rc = sscanf(range, "%u", &sub); if ((rc == 1) && (rc >= 0)) { .... } .... } 

El analizador de este fragmento da inmediatamente 2 advertencias. El especificador % u espera una variable de tipo unsigned int , pero la variable sub es de tipo int . A continuación, vemos una verificación sospechosa: la condición de la derecha no tiene sentido, ya que al principio hay una comparación con la unidad. No sé qué tenía en mente el autor de este código, pero claramente hay algo mal aquí.

Cheques sin ordenar

V547 La expresión 'estado == 0x00090314' siempre es falsa. 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; .... } 

Las condiciones marcadas siempre serán falsas, ya que la ejecución alcanzará la segunda condición solo si status == SEC_E_OK . El código correcto podría verse así:

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

Conclusión

Por lo tanto, la verificación del proyecto reveló muchos problemas, pero solo la parte más interesante se describió en el artículo. Los desarrolladores de proyectos pueden verificar el proyecto ellos mismos solicitando una clave de licencia temporal en el sitio web de PVS-Studio . También hubo falsos positivos, cuyo trabajo ayudará a mejorar el analizador. Sin embargo, el análisis estático es importante si desea no solo mejorar la calidad del código, sino también reducir el tiempo de búsqueda de errores, y PVS-Studio puede ayudarlo.

Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Larin. Comprobación de FreeRDP con PVS-Studio