FreeRDP – uma implementação aberta do Remote Desktop Protocol (RDP), um protocolo que permite o controle remoto de um computador, desenvolvido pela Microsoft. O projeto suporta várias plataformas, incluindo Windows, Linux, macOS e até mesmo iOS e Android. Este projeto foi o escolhido como o primeiro em uma série de artigos dedicados à análise de clientes RDP usando o analisador estático PVS-Studio.
Um pouco de história
Projeto surgiu após a Microsoft abrir as especificações de seu protocolo proprietário RDP. Naquela época, existia o cliente rdesktop, cuja implementação é baseada em resultados de engenharia reversa.
Durante a implementação do protocolo, tornou-se cada vez mais difícil adicionar novas funcionalidades devido à arquitetura existente do projeto. Mudanças nela geraram conflitos entre os desenvolvedores, levando à criação do fork rdesktop – FreeRDP. A disseminação posterior do produto foi limitada pela licença GPLv2, motivo pelo qual foi tomada a decisão de relicenciar sob a Apache License v2. No entanto, nem todos concordaram em mudar a licença de seu código, então os desenvolvedores decidiram reescrever o projeto, resultando na base de código moderna que temos hoje.
Para mais detalhes sobre a história do projeto, você pode ler a nota no blog oficial: "A história do projeto FreeRDP".
Como ferramenta para identificar erros e potenciais vulnerabilidades no código, foi utilizado . Este é um analisador estático de código para as linguagens C, C++, C# e Java, disponível nas plataformas Windows, Linux e macOS.
Neste artigo, são apresentados apenas os erros que me pareceram mais interessantes.
Vazamento de memória
A função foi encerrada sem liberar o ponteiro ‘cwd’. Um vazamento de memória é possível. 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 trecho foi retirado do subsistema winpr, que implementa uma camada de abstração WINAPI para sistemas não-Windows, ou seja, é uma leve analogia ao Wine. Aqui, podemos notar o vazamento: a memória alocada pela função getcwd, é liberada apenas ao lidar com casos especiais. Para corrigir o erro, é necessário adicionar uma chamada de free após memcpy.
Acesso fora dos limites do array
O acesso ao array pode estar fora dos limites. 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 é adicionado à lista, mesmo que o número de elementos tenha atingido o máximo. Basta substituir o operador <= para <, para não ultrapassar os limites da matriz.
Foi encontrada outra ocorrência desse tipo de erro:
- V557 O estouro do array é possível. O valor do índice ‘iBitmapFormat’ pode chegar a 8. orders.c 2623
Erros de digitação
Fragmento 1
A expressão ‘!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) // <=
goto error_out;
....
}Aqui vemos um erro de digitação comum: na segunda condição, está sendo feita a verificação da mesma variável que na primeira. Provavelmente, o erro foi causado por uma cópia de código mal sucedida.
Fragmento 2
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;
....
/* MajorVersion (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* MinorVersion (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Mais um erro de digitação: o comentário no código sugere que deveria chegar do fluxo minorVersion, porém a leitura está acontecendo na variável com o nome majorVersion. No entanto, não estou familiarizado com o protocolo, então isso é apenas uma suposição.
Fragmento 3
É estranho que o corpo da função ‘trio_index_last’ seja totalmente equivalente ao corpo da função ‘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);
}De acordo com o comentário, a função trio_index encontra a primeira ocorrência de um caractere na string, enquanto trio_index_last encontra a última. Mas os corpos dessas funções são idênticos! Provavelmente, isso é um erro de digitação, e a função trio_index_last deveria usar strrchr em vez de strchr. Então, o comportamento será o esperado.
Fragmento 4
O ponteiro ‘data’ na expressão é igual a nullptr. O valor resultante das operações aritméticas sobre esse ponteiro é sem 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 aqui o operador de negação foi acidentalmente esquecido ! perto de data. Estranho que isso não tenha sido notado.
Fragmento 5
O padrão ‘if (A) {…} else if (A) {…}’ foi detectado. Há 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 > 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);
}
....
}As duas últimas condições são idênticas: aparentemente, alguém esqueceu de verificá-las após a cópia. Pelo código, é evidente que a última parte trabalha com valores de quatro bytes, então pode-se supor que a última condição deveria ser value <= 0x3FFFFFFF.
Foi encontrada outra ocorrência desse tipo de erro:
- V517 O padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de erro lógico presente. Verifique as linhas: 169, 173. file.c 169
Verificação de dados de entrada
Fragmento 1
A expressão 'strcat(target, source) != 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 execução da função neste exemplo é incorreta. A função strcat retorna um ponteiro para a versão final da string, ou seja, o primeiro parâmetro passado. Neste caso, é alvo. No entanto, se ele for igual a NULL, então a verificação é tardia, pois na função strcat ocorrerá a desreferenciação.
Fragmento 2
A expressão '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)
{
....
}
....
}Neste caso, a variável cache recebe o endereço de um array estático glyphCache->glyphCache. Portanto, a verificação if (cache) pode ser descartada.
Erro de gerenciamento de recursos
O recurso foi adquirido usando a função 'CreateFileA' mas foi liberado utilizando a função incompatível '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;
}
....
}O descritor de arquivo fp, criado pela chamada da função CreateFile, foi erroneamente fechado pela função fclose da biblioteca padrão, e não CloseHandle..
Condições idênticas
As expressões condicionais das instruções 'if' situadas lado a lado 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, "aviso: NdrComplexStructBufferSize array_type: "
"0xX não 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, "aviso: NdrComplexStructBufferSize array_type: "
"0xX não implementado", array_type);
}
....
}Talvez este exemplo não seja um erro. No entanto, ambas as condições contêm mensagens idênticas, uma das quais provavelmente pode ser removida.
Limpeza de ponteiros nulos
O ponteiro nulo é passado para a função 'free'. 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; /* mszGroups não é suportado pelo 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);
....
}Na função free é possível passar um ponteiro nulo e o analisador sabe disso. Mas se uma situação se revela onde o ponteiro é sempre passado como nulo, como neste fragmento, um aviso será emitido.
Ponteiro mszGroupsA inicialmente é igual a NULL e não é inicializado em nenhum outro lugar. O único ramo de código onde o ponteiro poderia ser inicializado é inatingível.
Houve outras mensagens desse tipo:
- V575 O ponteiro nulo é passado para a função 'free'. Inspecione o primeiro argumento. license.c 790
- V575 O ponteiro nulo é passado para a função 'free'. Inspecione o primeiro argumento. rdpsnd_alsa.c 575
É provável que essas variáveis esquecidas surjam durante o processo de refatoração e podem ser simplesmente removidas.
Possível estouro
Possível estouro. Considere fazer o cast dos operandos, e não do 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));
....
}Fazer o cast do resultado para long não é uma proteção contra estouro, uma vez que o próprio cálculo ocorre usando o tipo int.
Desreferenciamento de ponteiro na inicialização
O ponteiro 'context' foi utilizado antes de ser verificado contra 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 está o ponteiro contexto é desreferenciado na inicialização — antes de sua verificação.
Foram encontrados outros erros desse tipo:
- V595 O ponteiro ‘ntlm’ foi utilizado antes de ser verificado contra nullptr. Verifique as linhas: 236, 255. ntlm.c 236
- V595 O ponteiro ‘contexto’ foi utilizado antes de ser verificado contra nullptr. Verifique as linhas: 1003, 1007. rfx.c 1003
- V595 O ponteiro ‘rdpei’ foi utilizado antes de ser verificado contra nullptr. Verifique as linhas: 176, 180. rdpei_main.c 176
- V595 O ponteiro ‘gdi’ foi utilizado antes de ser verificado contra nullptr. Verifique as linhas: 121, 123. xf_gfx.c 121
Condição sem sentido
A expressão ‘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; // state >= CONNECTION_STATE_ACTIVE) // Activate, client->activated, client);
if (!client->activated)
return -1;
}
....
}
....
}É fácil notar que a primeira condição não faz sentido devido à atribuição do valor correspondente anteriormente.
Análise incorreta da string
Formato incorreto. Considere verificar o terceiro argumento real da função ‘sscanf’. Um ponteiro para o tipo unsigned int é esperado. proxy.c 220
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 gera imediatamente 2 advertências. O especificador %u espera uma variável do tipo unsigned int, mas a variável sub tem tipo int. Vemos a verificação suspeita: a condição à direita não faz sentido, pois no início é uma comparação com um.
Verificações desordenadas
A expressão ‘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) // <=
status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
status = SEC_I_CONTINUE_NEEDED;
....
}As condições marcadas sempre serão falsas, pois a execução chegará à segunda condição apenas quando 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 identificou vários problemas, mas apenas a parte mais interessante deles foi descrita no artigo. Os desenvolvedores do projeto podem verificar o projeto por conta própria, solicitando uma chave de licença temporária no site. . Também houve falsos positivos, trabalhar nos quais ajudará a melhorar o analisador. No entanto, a análise estática é importante se você deseja não apenas aumentar a qualidade do código, mas também reduzir o tempo de busca por erros, e o PVS-Studio pode ajudar nisso.
Se você quiser compartilhar este artigo com um público de língua inglesa, por favor, use o link para a tradução: Sergey Larin.
Fonte: habr.com
