FreeRDP es una implementación abierta del Protocolo de Escritorio Remoto (RDP), un protocolo que permite el control remoto de un ordenador, desarrollado por Microsoft. El proyecto es compatible con múltiples plataformas, incluidas Windows, Linux, macOS e incluso iOS y Android. Este proyecto ha sido elegido como el primero en una serie de artículos dedicados a la evaluación de clientes RDP utilizando el analizador estático PVS-Studio.
Un poco de historia
Proyecto apareció después de que Microsoft abriera las especificaciones de su protocolo propietario RDP. En ese momento existía el cliente rdesktop, cuya implementación se basa en los resultados de la ingeniería inversa.
Durante la implementación del protocolo, se volvió más difícil agregar nuevas funcionalidades debido a la arquitectura existente del proyecto. Los cambios en esta generaron un conflicto entre los desarrolladores, lo que llevó a la creación del fork rdesktop: FreeRDP. La posterior distribución del producto se vio limitada por la licencia GPLv2, lo que llevó a la decisión de relicenciarlo bajo la Licencia Apache v2. Sin embargo, no todos estaban de acuerdo en cambiar la licencia de su código, por lo que los desarrolladores decidieron reescribir el proyecto, lo que nos llevó a la forma moderna de la base de código.
Más información sobre la historia del proyecto se puede leer en la entrada del blog oficial: 'La historia del proyecto FreeRDP'.
Como herramienta para identificar errores y posibles vulnerabilidades en el código, se utilizó . Este es un analizador estático de código para lenguajes C, C++, C# y Java, disponible en plataformas Windows, Linux y macOS.
En este artículo se presentan solo aquellos errores que me parecieron más interesantes.
Fuga de memoria
La función se salió sin liberar el puntero 'cwd'. Es posible una fuga de memoria. 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 extraído del subsistema winpr, que implementa un wrapper de WINAPI para sistemas no Windows, es decir, es un equivalente ligero de Wine. Aquí se puede notar una fuga: la memoria asignada por la función getcwd, solo se libera en el manejo de casos especiales. Para corregir el error, es necesario agregar una llamada free después de memcpy.
Desbordamiento de matriz
Es posible un desbordamiento de matriz. El índice del valor '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 añade un nuevo elemento a la lista, incluso si se ha alcanzado el número máximo de elementos. Solo es necesario reemplazar el operador <= en <, para no salir de los límites del arreglo.
Se encontró otro error de este tipo:
- V557 Puede haber un desbordamiento de arreglo. El valor del índice 'iBitmapFormat' podría alcanzar 8. orders.c 2623
Errores tipográficos
Fragmento 1
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 un error tipográfico común: en la segunda condición se verifica la misma variable que en la primera. Lo más probable es que el error se haya producido por una copia fallida del código.
Fragmento 2
Se encontraron dos bloques idénticos de texto. El segundo bloque comienza en 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 del código sugiere que debe llegar del flujo minorVersion, sin embargo, la lectura se realiza en la variable llamada majorVersion. Sin embargo, no estoy familiarizado con el protocolo, así que esto es solo una suposición.
Fragmento 3
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);
}Según el comentario, la función trio_index encuentra la primera coincidencia de un carácter en una cadena, mientras que trio_index_last es la última. ¡Pero los cuerpos de estas funciones son idénticos! Lo más probable es que se trate de un error tipográfico, y en la función trio_index_last debería utilizarse strrchr en lugar de strchr. Entonces el comportamiento será el esperado.
Fragmento 4
El puntero ‘data’ en la expresión es igual a nullptr. El valor resultante de las operaciones aritméticas en este puntero es sin sentido y no debe utilizarse. 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 aquí se omitió accidentalmente el operador de negación ! cerca de data. Es extraño que esto no haya sido notado.
Fragmento 5
Se detectó el patrón ‘if (A) {…} else if (A) {…}’. Existe la probabilidad de que haya un error lógico. Verifique las líneas: 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);
}
....
}Las últimas dos condiciones son iguales: parece que alguien olvidó verificarlas después de copiarlas. Por el código, se nota que la última parte trabaja con valores de cuatro bytes, por lo que se puede suponer que la última condición debería ser value <= 0x3FFFFFFF.
Se encontró otro error de este tipo:
- V517 Se detectó el uso del patrón ‘if (A) {...} else if (A) {...}’. Existe una probabilidad de presencia de error lógico. Verifique las líneas: 169, 173. file.c 169
Verificación de datos de entrada
Fragmento 1
La expresión ‘strcat(target, source) != NULL’ es siempre 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 verificación del resultado de la función en este caso es incorrecta. La función strcat devuelve un puntero a la versión final de la cadena, es decir, el primer parámetro pasado. En este caso, es target. Sin embargo, si es igual a NULL, comprobarlo es tarde, ya que en la función strcat se producirá su desreferenciación.
Fragmento 2
La expresión ‘cache’ es siempre 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 cache se le asigna la dirección de un arreglo estático glyphCache->glyphCache. Así, la comprobación if (cache) se puede omitir.
Error de gestión de recursos
El recurso fue adquirido utilizando la función ‘CreateFileA’ pero se liberó utilizando la función incompatible ‘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;
}
....
}El descriptor de archivo fp, creado por la llamada a la función CreateFile, se cerró por error con la función fclose de la biblioteca estándar, y no CloseHandle.
Condiciones idénticas
Las expresiones condicionales de las declaraciones ‘if’ situadas una al lado de la otra son idénticas. Verifique las líneas: 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, "advertencia: NdrComplexStructBufferSize array_type: "
"0xX no implementado", 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, "advertencia: NdrComplexStructBufferSize array_type: "
"0xX no implementado", array_type);
}
....
}Puede que este ejemplo no sea un error. Sin embargo, ambas condiciones contienen mensajes idénticos, de los cuales uno probablemente podría ser eliminado.
Limpieza de punteros nulos
Se pasa un puntero nulo a la función 'free'. Inspeccione 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 no es compatible con 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);
....
}En la función free se puede pasar un puntero nulo y el analizador es consciente de ello. Pero si se identifica una situación en la que el puntero siempre se pasa como nulo, como en este fragmento, se emitirá una advertencia.
Puntero mszGroupsA inicialmente es igual a NULL y no se inicializa en ninguna otra parte. La única rama del código donde el puntero podría haberse inicializado es inalcanzable.
Hubo otros mensajes de este tipo:
- V575 Se pasa un puntero nulo a la función 'free'. Inspeccione el primer argumento. license.c 790
- V575 Se pasa un puntero nulo a la función 'free'. Inspeccione el primer argumento. rdpsnd_alsa.c 575
Probablemente, estas variables olvidadas ocurren durante el proceso de refactorización y simplemente se pueden eliminar.
Posible desbordamiento
Posible desbordamiento. Considere el tipo de los 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));
....
}El casting del resultado a long no es una protección contra desbordamiento, ya que el cálculo en sí se realiza utilizando el tipo int.
Desreferenciación del puntero en la inicialización
El puntero 'context' fue utilizado antes de que se verificara contra nullptr. Verifique las líneas: 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í está el puntero 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 ser verificado contra nullptr. Verifique las líneas: 236, 255. ntlm.c 236
- V595 El puntero 'contexto' se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 1003, 1007. rfx.c 1003
- V595 El puntero 'rdpei' se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 176, 180. rdpei_main.c 176
- V595 El puntero 'gdi' se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 121, 123. xf_gfx.c 121
Condición sin sentido
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; // state >= CONNECTION_STATE_ACTIVE) // Activate, client->activated, client);
if (!client->activated)
return -1;
}
....
}
....
}Es fácil notar que la primera condición no tiene sentido debido a la asignación del valor correspondiente previamente.
Análisis incorrecto de la cadena
Formato incorrecto. Considere verificar el tercer argumento real de la función 'sscanf'. Se espera un puntero del tipo unsigned int. proxy.c 220
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 para este fragmento genera inmediatamente 2 advertencias. El especificador %u espera una variable del tipo unsigned int, pero la variable sub tiene el tipo int. Luego vemos una verificación sospechosa: la condición a la derecha no tiene sentido, ya que al principio se realiza una comparación con uno. No sé qué quiso decir el autor de este código, pero aquí claramente algo no está bien.
Comprobaciones desordenadas
La expresión 'status == 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 solo llegará a la segunda condición cuando 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
Así, la revisión del proyecto reveló múltiples problemas, pero solo la parte más interesante de ellos se describió en el artículo. Los desarrolladores del proyecto pueden verificar el proyecto ellos mismos solicitando una clave de licencia temporal en el sitio web. . También hubo falsos positivos, y trabajar en ellos 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 ayudar en esto.
Si desea compartir este artículo con una audiencia de habla inglesa, le pido que use el enlace a la traducción: Sergey Larin.
Fuente: habr.com
