O FreeRDP é uma implementação de código aberto do Remote Desktop Protocol (RDP), um protocolo que implementa o controle remoto do computador desenvolvido pela Microsoft. O projeto suporta muitas plataformas, incluindo Windows, Linux, macOS e até iOS com Android. Este projeto foi selecionado como o primeiro de uma série de artigos dedicados à verificação de clientes RDP usando o analisador estático PVS-Studio.
Um pouco de história
O projeto
FreeRDP veio depois que a Microsoft revelou as especificações de seu protocolo RDP proprietário. Naquela época, havia um cliente rdesktop, cuja implementação é baseada nos resultados da Engenharia Reversa.
No processo de implementação do protocolo, ficou mais difícil adicionar novas funcionalidades devido à arquitetura do projeto então existente. As mudanças nele criaram um conflito entre os desenvolvedores, o que levou à criação do fork do rdesktop - FreeRDP. A distribuição posterior do produto foi limitada pela licença GPLv2, como resultado da decisão de licenciar novamente para a Apache License v2. No entanto, nem todos concordaram em alterar a licença de seu código, então os desenvolvedores decidiram reescrever o projeto, como resultado, temos uma aparência moderna da base de código.
Mais detalhes sobre a história do projeto podem ser encontrados na nota oficial do blog: “A história do projeto FreeRDP”.
O PVS-Studio foi usado como uma ferramenta para detectar erros e possíveis vulnerabilidades no código. É um analisador de código estático para C, C ++, C # e Java, disponível no Windows, Linux e macOS.
O artigo apresenta apenas os erros que me pareceram mais interessantes.
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; .... }
Este fragmento foi retirado do subsistema winpr que implementa o wrapper WINAPI para sistemas não Windows, ou seja, É uma contrapartida leve para o vinho. Há um vazamento aqui: a memória alocada pela função
getcwd é liberada apenas em casos especiais. Para corrigir o erro, adicione uma chamada para
liberar após o
memcpy .
Indo além dos limites de uma matriz
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 item é adicionado à lista, mesmo que o número de itens tenha atingido o máximo. Basta substituir o operador
<= por
< para não ultrapassar os limites da matriz.
Outro erro deste tipo foi encontrado:
- A saturação da matriz V557 é possível. O valor do índice 'iBitmapFormat' pode chegar a 8. orders.c 2623
Erros de digitação
Fragmento 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)
Aqui vemos o erro de digitação usual: na segunda condição, a mesma variável é verificada como na primeira. Provavelmente, o erro apareceu como resultado da cópia malsucedida do código.
Fragmento 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: um comentário no código implica que
minorVersion deve vir do fluxo, no entanto, a leitura ocorre em uma variável denominada
majorVersion . No entanto, eu não estou familiarizado com o protocolo, então isso é apenas uma suposição.
Fragmento 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); }
A julgar pelo comentário, a função
trio_index encontra a primeira correspondência de um caractere em uma string quando
trio_index_last é o último. Mas os corpos dessas funções são idênticos! Provavelmente, esse é um erro de digitação e, na função
trio_index_last ,
você precisa usar
strrchr em vez de
strchr . Então o comportamento será esperado.
Fragmento 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; .... }
Parece que eles perderam acidentalmente o operador de negação
! próximo aos
dados . É estranho que isso tenha passado despercebido.
Fragmento 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: aparentemente, alguém esqueceu de verificá-las após a cópia. De acordo com o código, é perceptível que a última parte funciona com valores de quatro bytes, portanto, podemos assumir que a última condição deve ser o
valor <= 0x3FFFFFFF .
Outro erro deste tipo foi encontrado:
- 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
Validação de entrada
Fragmento 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 resultado da função neste exemplo está incorreta. A função
strcat retorna um ponteiro para a versão final da string, ou seja, primeiro parâmetro passado. Neste caso, é
alvo . No entanto, se for
NULL , será tarde demais para verificar, pois na função
strcat será desreferenciada.
Fragmento 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 caso, a variável de
cache recebe o endereço da matriz estática
glyphCache-> glyphCache . Portanto, a verificação
if (cache) pode ser omitida.
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 descritor de arquivo
fp criado chamando a função
CreateFile é fechado por engano pela função
fclose da biblioteca padrão, em vez de
CloseHandle .
Mesmas condições
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); } .... }
Talvez este exemplo não seja um erro. No entanto, ambas as condições contêm a mesma mensagem, uma das quais, provavelmente, pode ser removida.
Apagar 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); .... }
Você pode passar um ponteiro nulo para a função
livre , e o analisador sabe disso. Mas se for revelada uma situação em que o ponteiro sempre passa zero, como neste fragmento, um aviso será emitido.
O ponteiro
mszGroupsA é inicialmente
NULL e não é inicializado em nenhum outro lugar. O único ramo do código em que o ponteiro pode ser inicializado é inacessível.
Havia outros posts como este:
- 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
Muito provavelmente, essas variáveis esquecidas surgem durante a refatoração e podem ser simplesmente excluídas.
Possível estouro
V1028 Possível estouro. Considere lançar operandos, não o resultado. makecert.c 1087
Trazer o resultado a
longo prazo não
é proteção contra o estouro, uma vez que o próprio cálculo é realizado usando o tipo
int .
Desreferenciamento de 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; .... }
Aqui, o ponteiro de
contexto é desreferenciado na inicialização - antes de ser verificado.
Outros erros deste tipo foram encontrados:
- 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 devido à atribuição do valor correspondente anteriormente.
Análise de linha incorreta
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)) { .... } .... }
O analisador para este fragmento emite imediatamente 2 avisos. O especificador
% u espera uma variável do tipo
int sem sinal , mas a variável
sub é do tipo
int . A seguir, vemos uma verificação suspeita: a condição à direita não faz sentido, pois no início há uma comparação com a unidade. Não sei o que o autor deste código tinha em mente, mas há claramente algo errado aqui.
Verificações não ordenadas
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 execução alcançará a segunda condição somente se
status == SEC_E_OK . O código correto pode ser assim:
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
Assim, a verificação do projeto revelou muitos problemas, mas apenas a parte mais interessante foi descrita no artigo. Os desenvolvedores do projeto podem verificar o projeto solicitando uma chave de licença temporária no site do
PVS-Studio . Havia também falsos positivos, trabalhos em que ajudarão a melhorar o analisador. No entanto, a análise estática é importante se você deseja não apenas melhorar a qualidade do código, mas também reduzir o tempo para procurar erros, e o PVS-Studio pode ajudar com isso.
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Sergey Larin.
Verificando o FreeRDP com PVS-Studio