O FreeRDP é uma implementação de código aberto do Remote Desktop Protocol (RDP), um protocolo proprietário da Microsoft. O projeto suporta várias plataformas, incluindo Windows, Linux, macOS e até iOS e Android. Optamos por ser o primeiro projeto analisado com o analisador de código estático PVS-Studio para uma série de artigos sobre as verificações de clientes RDP.
Alguma história
O projeto
FreeRDP foi iniciado depois que a Microsoft abriu as especificações para o protocolo proprietário RDP. Naquele momento, um cliente chamado rdesktop já estava em uso, com base principalmente no trabalho de engenharia reversa.
Enquanto implementavam o protocolo, os desenvolvedores acharam difícil adicionar novas funcionalidades devido a problemas de arquitetura. Alterações na arquitetura envolveram um conflito entre os desenvolvedores e levaram à criação de um fork do rdesktop conhecido como FreeRDP. A distribuição adicional foi limitada pela licença GPLv2, e os autores decidiram relicenciar a licença Apache v2. No entanto, alguns não estavam dispostos a alterar a licença, então os desenvolvedores decidiram reescrever a base de código do zero e foi assim que o projeto, como o conhecemos hoje, surgiu.
O histórico completo do projeto está disponível no blog oficial: "
O histórico do projeto FreeRDP ".
Usei o
PVS-Studio para verificar o projeto quanto a bugs e possíveis vulnerabilidades. O PVS-Studio é um analisador estático para código escrito em C, C ++, C # e Java e é executado no Windows, Linux e macOS.
Observe que discutirei apenas os bugs que pareciam mais interessantes para mim.
Vazamento de memória
V773 A função foi encerrada sem liberar o ponteiro 'cwd'. É possível um vazamento de memória. 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; .... }
Esse trecho vem do subsistema winpr, que implementa um wrapper WINAPI para sistemas não Windows, ou seja, atua como um equivalente mais leve do Wine. O código acima contém um vazamento de memória: a memória alocada pela função
getcwd é liberada apenas em ramificações de casos especiais. Para corrigir isso, os autores devem adicionar uma chamada para
liberar após a chamada para
memcpy .
Índice de matriz fora dos limites
A saturação da matriz V557 é possível. O valor do índice 'event-> EventHandlerCount' pode chegar a 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++; } .... }
Neste exemplo, um novo elemento será adicionado à lista mesmo quando este já tiver atingido o número máximo de elementos. Esse bug pode ser corrigido simplesmente substituindo o operador
<= por
< .
O analisador encontrou outro bug desse tipo:
- A saturação da matriz V557 é possível. O valor do índice 'iBitmapFormat' pode chegar a 8. orders.c 2623
Erros de digitação
Snippet 1
A expressão
V547 '! Pipe-> In' é sempre 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)
}
O que vemos aqui é um erro de digitação comum: a primeira e a segunda condições verificam a mesma variável. Parece muito com um produto de copiar e colar incorreto.
Snippet 2
V760 Foram
encontrados dois blocos de texto idênticos. O segundo bloco começa na linha 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; .... Stream_Read_UINT16(pdu->s, versionCaps->majorVersion); Stream_Read_UINT16(pdu->s, versionCaps->majorVersion); .... }
Outro erro de digitação: o comentário diz que devemos esperar que a variável
minorVersion seja lida no fluxo, enquanto o valor é lido na variável
majorVersion . Eu não estou familiarizado com o projeto o suficiente para ter certeza, no entanto.
Snippet 3
V524 É estranho que o corpo da função 'trio_index_last' seja totalmente equivalente ao corpo da função 'trio_index'. triostr.c 933
TRIO_PUBLIC_STRING char * trio_index TRIO_ARGS2((string, character), TRIO_CONST char *string, int character) { assert(string); return strchr(string, character); } TRIO_PUBLIC_STRING char * trio_index_last TRIO_ARGS2((string, character), TRIO_CONST char *string, int character) { assert(string); return strchr(string, character); }
Como o comentário sugere, a função
trio_index localiza a ocorrência do primeiro caractere na string, enquanto a função
trio_index_last localiza a última ocorrência. No entanto, os corpos dessas duas funções são exatamente os mesmos! Isso deve ser um erro de digitação e a função
trio_index_last provavelmente deve retornar
strrchr em vez de
strchr - nesse caso, o programa se comportaria conforme o esperado.
Snippet 4
V769 O ponteiro 'data' na expressão é igual a nullptr. O valor resultante das operações aritméticas nesse ponteiro não faz sentido e não deve ser usado. 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; .... }
O desenvolvedor deve ter deixado acidentalmente o operador de negação
! antes dos
dados . Eu me pergunto por que ninguém percebeu isso antes.
Snippet 5
V517 O uso do
padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 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); } .... }
As duas últimas condições são as mesmas: o programador deve ter esquecido de alterar a cópia. A julgar pela lógica do código, a última parte lida com valores de quatro bytes, para que possamos assumir que a última condição deve verificar se o
valor <= 0x3FFFFFFF .
Mais um bug deste tipo:
- V517 O uso do padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 169, 173. file.c 169
Verificando dados de entrada
Snippet 1
A expressão
V547 'strcat (destino, origem)! = NULL' sempre é verdadeira. 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); }
A verificação do valor de retorno da função está com defeito. A função
strcat retorna um ponteiro para a sequência de destino, ou seja, o primeiro parâmetro, que neste caso é o
destino . Mas se for igual a NULL, é tarde demais para checá-lo, pois ele já terá sido desreferenciado na função
strcat .
Snippet 2
A expressão
V547 'cache' é sempre verdadeira. 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) { .... } .... }
Nesse snippet, a variável de
cache recebe o endereço da matriz estática
glyphCache-> glyphCache . A verificação
se (cache) pode, portanto, ser removida.
Erro de gerenciamento de recursos
V1005 O recurso foi adquirido usando a função 'CreateFileA', mas foi liberado usando a função 'fclose' incompatível. 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; } .... }
O identificador
fp para o arquivo criado pela função
CreateFile foi fechado por engano chamando a função
fclose da biblioteca padrão em vez da função
CloseHandle .
Condições idênticas
V581 As expressões condicionais das instruções 'if' localizadas uma ao lado da outra são idênticas. Verifique as linhas: 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); } .... }
Esse trecho pode estar correto, mas é suspeito que ambas as condições contenham mensagens idênticas - uma delas é provavelmente desnecessária.
Liberando ponteiros nulos
V575 O ponteiro nulo é passado para a função 'livre'. Inspecione o primeiro argumento. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW( SCARDCONTEXT hContext, LPCWSTR mszGroups, LPWSTR mszReaders, LPDWORD pcchReaders) { LPSTR mszGroupsA = NULL; .... mszGroups = NULL; 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); .... }
A função
livre pode ser chamada em um ponteiro nulo, e o PVS-Studio sabe disso. Mas se o ponteiro for sempre nulo, como neste trecho, o analisador emitirá um aviso.
O ponteiro
mszGroupsA é inicialmente definido como
NULL e não é inicializado em nenhum outro lugar. O único ramo em que poderia ser inicializado é inacessível.
Alguns outros avisos desse tipo:
- V575 O ponteiro nulo é passado para a função 'livre'. Inspecione o primeiro argumento. license.c 790
- V575 O ponteiro nulo é passado para a função 'livre'. Inspecione o primeiro argumento. rdpsnd_alsa.c 575
Variáveis abandonadas como essa parecem ser resíduos deixados após a refatoração e podem ser removidas.
Excesso potencial
V1028 Possível estouro. Considere lançar operandos, não o resultado. makecert.c 1087
A conversão
longa do resultado da expressão não impedirá um estouro, pois a avaliação é feita no valor enquanto ele ainda é do tipo
int .
Desreferenciando o ponteiro na inicialização
V595 O ponteiro 'context' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 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; .... }
O ponteiro de
contexto é desreferenciado durante sua inicialização, ou seja, antes da verificação.
Outros erros deste tipo:
- V595 O ponteiro 'ntlm' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 236, 255. ntlm.c 236
- V595 O ponteiro 'context' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 1003, 1007. rfx.c 1003
- V595 O ponteiro 'rdpei' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 176, 180. rdpei_main.c 176
- V595 O ponteiro 'gdi' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 121, 123. xf_gfx.c 121
Condição sem sentido
A expressão
V547 'rdp-> state> = CONNECTION_STATE_ACTIVE' sempre é verdadeira. connection.c 1489
int rdp_server_transition_to_state(rdpRdp* rdp, int state) { .... switch (state) { .... case CONNECTION_STATE_ACTIVE: rdp->state = CONNECTION_STATE_ACTIVE;
É fácil ver que a primeira condição não faz sentido porque o valor em questão já foi atribuído antes.
Manuseio incorreto de strings
V576 Formato incorreto. Considere verificar o terceiro argumento real da função 'sscanf'. Um ponteiro para o tipo int não assinado é esperado. proxy.c 220
V560 Uma parte da expressão condicional é sempre verdadeira: (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 dispara dois avisos ao mesmo tempo. O espaço reservado
% u é usado para variáveis do tipo
int sem sinal , enquanto a
sub- variável é do tipo
int . O segundo aviso indica uma verificação suspeita: a parte direita da expressão condicional não faz sentido, pois a variável já foi verificada para 1 na parte esquerda. Não tenho certeza das intenções do autor, mas algo está obviamente errado com esse código.
Verifica na ordem errada
A expressão
V547 'status == 0x00090314' é sempre falsa. ntlm.c 299
BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded) { .... if (status != SEC_E_OK) { .... return FALSE; } if (status == SEC_I_COMPLETE_NEEDED)
As condições marcadas sempre serão falsas, pois a segunda condição pode ser executada apenas se
status == SEC_E_OK . É assim que a versão correta pode ser:
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; }
Conclusão
A verificação revelou muitos erros, e os discutidos acima são apenas os mais interessantes. Os desenvolvedores do projeto podem enviar um formulário para uma chave de licença temporária no site do
PVS-Studio para fazer sua própria verificação. O analisador também produziu vários falsos positivos, que serão corrigidos para melhorar seu desempenho. Observe que a análise estática é indispensável se seu objetivo não é apenas melhorar a qualidade do código, mas também tornar a caça a bugs menos demorada - e é aí que o PVS-Studio será útil.