Comprobación de FreeRDP con PVS-Studio

FreeRDP es una implementación de código abierto del Protocolo de escritorio remoto (RDP), un protocolo patentado por Microsoft. El proyecto admite múltiples plataformas, incluidas Windows, Linux, macOS e incluso iOS y Android. Elegimos que fuera el primer proyecto analizado con el analizador de código estático PVS-Studio para una serie de artículos sobre las comprobaciones de clientes RDP.

Algo de historia

El proyecto FreeRDP se inició después de que Microsoft abrió las especificaciones para su protocolo propietario RDP. En ese momento, un cliente llamado rdesktop ya estaba en uso, basado principalmente en el trabajo de ingeniería inversa.

Mientras implementaban el protocolo, a los desarrolladores les resultó difícil agregar nueva funcionalidad debido a problemas de arquitectura. Los cambios en la arquitectura implicaron un conflicto entre los desarrolladores y llevaron a la creación de una bifurcación de rdesktop conocida como FreeRDP. La distribución adicional estaba limitada por la licencia GPLv2, y los autores decidieron volver a licenciar a Apache License v2. Sin embargo, algunos no estaban dispuestos a cambiar la licencia, por lo que los desarrolladores decidieron reescribir la base del código desde cero y así es como surgió el proyecto tal como lo conocemos hoy.

La historia completa del proyecto está disponible en el blog oficial: " La historia del proyecto FreeRDP ".

Utilicé PVS-Studio para escanear el proyecto en busca de errores y vulnerabilidades potenciales. PVS-Studio es un analizador estático para código escrito en C, C ++, C # y Java y se ejecuta en Windows, Linux y macOS.

Tenga en cuenta que solo hablaré de los errores que me parecieron 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 proviene del subsistema winpr, que implementa un contenedor WINAPI para sistemas que no son Windows, es decir, actúa como un equivalente más ligero de Wine. El código anterior contiene una pérdida de memoria: la memoria asignada por la función getcwd se libera solo en ramas de casos especiales. Para solucionar esto, los autores deben agregar una llamada gratuita después de la llamada a memcpy .

Índice de matriz fuera de límites

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 agregará un nuevo elemento a la lista incluso cuando este último haya alcanzado el número máximo de elementos. Este error se puede solucionar simplemente reemplazando el operador <= con < .

El analizador 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; .... 

}

Lo que vemos aquí es un error tipográfico ordinario: tanto la primera como la segunda condición verifican la misma variable. Se parece mucho a un producto de mal pegado.

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: el comentario dice que deberíamos esperar que la variable minorVersion se lea de la secuencia, mientras que el valor se lee en la variable majorVersion . Sin embargo, no estoy familiarizado con el proyecto lo suficiente como para asegurarlo.

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

Como sugiere el comentario, la función trio_index encuentra la aparición del primer carácter en la cadena, mientras que la función trio_index_last encuentra la última aparición. ¡Sin embargo, los cuerpos de ambas funciones son exactamente iguales! Esto debe ser un error tipográfico y la función trio_index_last probablemente debería devolver strrchr en lugar de strchr ; en ese caso, el programa se comportaría como se esperaba.

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

¡El desarrollador debe haber omitido accidentalmente al operador de negación ! antes de los datos . Me pregunto por qué nadie lo notó antes.

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: el programador debe haber olvidado cambiar la copia. A juzgar por la lógica del código, la última parte maneja valores de cuatro bytes, por lo que podríamos suponer que la última condición debería verificar si el valor <= 0x3FFFFFFF .

Un error más 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

Comprobación de datos 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); } 

La comprobación del valor de retorno de la función es defectuosa. La función strcat devuelve un puntero a la cadena de destino, es decir, el primer parámetro, que en este caso es el destino . Pero si es igual a NULL, es demasiado tarde para verificarlo, ya que habrá sido desreferenciado en la función strcat .

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 fragmento, a la variable de caché se le asigna la dirección de la matriz estática glyphCache-> glyphCache . La comprobación si (caché) puede, por lo tanto, eliminarse.

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 identificador fp del archivo creado por la función CreateFile se cerró por error llamando a la función fclose desde la biblioteca estándar en lugar de la función CloseHandle .

Condiciones idénticas

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

Este fragmento puede ser correcto, pero es sospechoso que ambas condiciones contengan mensajes idénticos; uno de ellos probablemente sea innecesario.

Liberación 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); .... } 

La función libre se puede invocar con un puntero nulo, y PVS-Studio lo sabe. Pero si se encuentra que el puntero siempre es nulo, como en este fragmento, el analizador emitirá una advertencia.

El puntero mszGroupsA se establece inicialmente en NULL y no se inicializa en ningún otro lugar. La única rama donde podría inicializarse es inalcanzable.

Un par de otras advertencias de este tipo:

• 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

Las variables abandonadas como esa parecen ser residuos que quedan después de la refactorización y pueden eliminarse.

Desbordamiento potencial

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

Lanzar el resultado de la expresión a largo no evitará un desbordamiento ya que la evaluación se realiza en el valor mientras todavía es de tipo int .

Puntero de referencia en la 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; .... } 

El puntero de contexto se desreferencia durante su inicialización, es decir, antes de la verificación.

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 porque el valor en cuestión ya se asignó anteriormente.

Manejo de cadena 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)) { .... } .... } 

Este código activa dos advertencias a la vez. El marcador de posición % u se usa para variables de tipo unsigned int , mientras que la subvariable es de tipo int . La segunda advertencia señala una verificación sospechosa: la parte derecha de la expresión condicional no tiene sentido ya que la variable ya se verificó para 1 en la parte izquierda. No estoy seguro de las intenciones del autor, pero obviamente hay algo mal con este código.

Cheques en el orden incorrecto

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 segunda condición solo se puede ejecutar si status == SEC_E_OK . Así es como se vería la versión correcta:

 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

La verificación reveló muchos errores, y los discutidos anteriormente son solo los más interesantes. Los desarrolladores del proyecto pueden enviar un formulario para una clave de licencia temporal en el sitio web de PVS-Studio para hacer su propia verificación. El analizador también produjo una serie de falsos positivos, que corregiremos para mejorar su rendimiento. Tenga en cuenta que el análisis estático es indispensable si su objetivo no es solo mejorar la calidad del código, sino también hacer que la búsqueda de errores sea menos lenta, y ahí es donde PVS-Studio será útil.