Este é o segundo post da nossa série de artigos sobre os resultados da verificação de software de código aberto que trabalha com o protocolo RDP. Hoje vamos dar uma olhada no cliente rdesktop e no servidor xrdp.
A análise foi realizada pelo
PVS-Studio . Este é um analisador estático para código escrito em C, C ++, C # e Java e é executado no Windows, Linux e macOS.
Vou discutir apenas os bugs que me pareciam mais interessantes. Por outro lado, como os projetos são bem pequenos, não há muitos bugs neles :).
Nota O artigo anterior sobre a verificação do FreeRDP está disponível
aqui .
rdesktop
O rdesktop é um cliente RDP gratuito para sistemas baseados em UNIX. Também pode ser executado no Windows, se construído com o Cygwin. O rdesktop é lançado sob a GPLv3.
Este é um cliente muito popular. Ele é usado como um cliente padrão no ReactOS e você também pode encontrar front-ends gráficos de terceiros para acompanhá-lo. No entanto, o projeto é bastante antigo: foi lançado pela primeira vez em 4 de abril de 2001 e tem 17 anos, até o momento.
Como eu já disse, o projeto é muito pequeno - cerca de 30 KLOC, o que é um pouco estranho, considerando sua idade. Compare isso com o FreeRDP com seu 320 KLOC. Aqui está a saída de Cloc:
Código inacessível
V779 Código inacessível detectado. É possível que haja um erro. rdesktop.c 1502
int main(int argc, char *argv[]) { .... return handle_disconnect_reason(deactivated, ext_disc_reason); if (g_redirect_username) xfree(g_redirect_username); xfree(g_username); }
O primeiro erro é encontrado imediatamente na função
principal : o código após a declaração de
retorno foi criado para liberar a memória alocada anteriormente. Mas esse defeito não é perigoso, porque toda a memória alocada anteriormente será liberada pelo sistema operacional assim que o programa terminar.
Sem manipulação de erros
V557 Array underrun é possível. O valor do índice 'n' pode chegar a -1. rdesktop.c 1872
RD_BOOL subprocess(char *const argv[], str_handle_lines_t linehandler, void *data) { int n = 1; char output[256]; .... while (n > 0) { n = read(fd[0], output, 255); output[n] = '\0';
O conteúdo do arquivo é lido no buffer até que o EOF seja alcançado. Ao mesmo tempo, esse código não possui um mecanismo de tratamento de erros e, se algo der errado, a
leitura retornará -1 e a execução começará a ler além dos limites da matriz de
saída .
Usando EOF em char
O V739 EOF não deve ser comparado com um valor do tipo 'char'. O '(c = fgetc (fp))' deve ser do tipo 'int'. ctrl.c 500
int ctrl_send_command(const char *cmd, const char *arg) { char result[CTRL_RESULT_SIZE], c, *escaped; .... while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != '\n') { result[index] = c; index++; } .... }
Esse código implementa manipulação incorreta de
EOF : se
fgetc retornar um caractere cujo código é 0xFF, ele será interpretado como o final do arquivo (
EOF ).
EOF é uma constante normalmente definida como -1. Por exemplo, na codificação CP1251, a última letra do alfabeto russo é codificada como 0xFF, que corresponde ao número -1 no tipo
char . Isso significa que o caractere 0xFF, assim como o
EOF (-1), será interpretado como o final do arquivo. Para evitar erros como esse, o resultado retornado pela função
fgetc deve ser armazenado em uma variável do tipo
int .
Erros de digitação
Snippet 1A expressão
V547 'write_time' é sempre falsa. disk.c 805
RD_NTSTATUS disk_set_information(....) { time_t write_time, change_time, access_time, mod_time; .... if (write_time || change_time) mod_time = MIN(write_time, change_time); else mod_time = write_time ? write_time : change_time;
O autor deste código deve ter usado acidentalmente o
|| operador em vez de
&& na condição. Vamos ver quais valores as variáveis
write_time e
change_time podem ter:
- Ambas as variáveis possuem 0. Nesse caso, a execução passa para o ramo else : a variável mod_time sempre será avaliada como 0, independentemente da próxima condição.
- Uma das variáveis possui 0. Nesse caso, mod_time receberá 0 (dado que a outra variável possui um valor não negativo), pois MIN escolherá o menor dos dois.
- Nenhuma variável possui 0: o valor mínimo é escolhido.
Alterar essa linha para
write_time && change_time corrigirá o comportamento:
- Somente uma ou nenhuma variável possui 0: o valor diferente de zero é escolhido.
- Nenhuma variável possui 0: o valor mínimo é escolhido.
Snippet 2A expressão
V547 é sempre verdadeira. Provavelmente o operador '&&' deve ser usado aqui. disk.c 1419
static RD_NTSTATUS disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in, STREAM out) { .... if (((request >> 16) != 20) || ((request >> 16) != 9)) return RD_STATUS_INVALID_PARAMETER; .... }
Novamente, parece que o problema de usar o operador errado - ou
|| em vez de
&& ou
== em vez de
! = porque a variável não pode armazenar os valores 20 e 9 ao mesmo tempo.
Cópia ilimitada de strings
V512 Uma chamada da função 'sprintf' levará ao estouro do buffer 'caminho completo'. disk.c 1257
RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
Se você pudesse seguir a função até o fim, veria que o código está OK, mas pode ser quebrado um dia: apenas uma mudança descuidada acabará com um estouro de buffer, pois o
sprintf não é limitado de forma alguma, concatenando os caminhos podem levar a execução além dos limites da matriz. Recomendamos substituir esta chamada por
snprintf (caminho completo, PATH_MAX, ....) .
Condição redundante
V560 Uma parte da expressão condicional é sempre verdadeira: add> 0. scard.c 507
static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }
A verificação
add> 0 não faz nenhuma diferença, pois a variável sempre será maior que zero, porque
read% 4 retorna o restante, que nunca será igual a 4.
xrdp
xrdp é um servidor RDP de código aberto. O projeto consiste em duas partes:
- xrdp - a implementação do protocolo. É lançado no Apache 2.0.
- xorgxrdp - uma coleção de drivers Xorg a serem usados com o xrdp. É lançado no X11 (assim como o MIT, mas o uso de publicidade é proibido)
O desenvolvimento é baseado no rdesktop e no FreeRDP. Inicialmente, para poder trabalhar com gráficos, você precisaria usar um servidor VNC separado ou um servidor X11 especial com suporte a RDP, X11rdp, mas esses se tornaram desnecessários com o lançamento do xorgxrdp.
Não falaremos sobre xorgxrdp neste artigo.
Assim como o projeto anterior, o xrdp é pequeno, consistindo em cerca de 80 KLOC.
Mais erros de digitação
V525 O código contém a coleção de blocos semelhantes. Verifique os itens 'r', 'g', 'r' nas linhas 87, 88, 89. rfxencode_rgb_to_yuv.c 87
static int rfx_encode_format_rgb(const char *rgb_data, int width, int height, int stride_bytes, int pixel_format, uint8 *r_buf, uint8 *g_buf, uint8 *b_buf) { .... switch (pixel_format) { case RFX_FORMAT_BGRA: .... while (x < 64) { *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r;
Esse código vem da biblioteca librfxcodec, que implementa o codec jpeg2000 para funcionar com o RemoteFX. O canal colorido "vermelho" é lido duas vezes, enquanto o canal "azul" não é lido. Defeitos como esse geralmente resultam do uso de copiar e colar.
O mesmo bug foi encontrado na função semelhante
rfx_encode_format_argb :
V525 O código contém a coleção de blocos semelhantes. Verifique os itens 'a', 'r', 'g', 'r' nas linhas 260, 261, 262, 263. rfxencode_rgb_to_yuv.c 260
while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; }
Declaração de matriz
A saturação da matriz V557 é possível. O valor do índice 'i - 8' pode chegar a 129. genkeymap.c 142
No arquivo genkeymap.c, a matriz é declarada 1 elemento menor que o implícito na implementação. Porém, nenhum erro ocorrerá, porque o arquivo evdev-map.c armazena o tamanho correto, portanto não haverá excesso de matriz, o que o torna um defeito menor e não um erro verdadeiro.
Comparação incorreta
V560 Uma parte da expressão condicional é sempre falsa: (cap_len <0). xrdp_caps.c 616
O valor de uma variável do tipo
curto não assinado é lido em uma variável do tipo
int e, em seguida, verificado como negativo, o que não é necessário porque um valor lido de um tipo não assinado em um tipo maior nunca pode se tornar negativo.
Verificações redundantes
V560 Uma parte da expressão condicional é sempre verdadeira: (bpp! = 16). libxrdp.c 704
int EXPORT_CC libxrdp_send_pointer(struct xrdp_session *session, int cache_idx, char *data, char *mask, int x, int y, int bpp) { .... if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32)) { g_writeln("libxrdp_send_pointer: error"); return 1; } .... }
As verificações não iguais não são necessárias porque a primeira verificação faz o trabalho. O programador provavelmente usaria o
|| operador para filtrar argumentos incorretos.
Conclusão
O cheque de hoje não revelou nenhum erro crítico, mas revelou vários defeitos menores. Dito isto, esses projetos, por mais pequenos que sejam, ainda são usados em muitos sistemas e, portanto, precisam de polimento. Um projeto pequeno não deve necessariamente conter muitos bugs; portanto, testar o analisador apenas em projetos pequenos não é suficiente para avaliar com segurança sua eficácia. Este assunto é discutido em mais detalhes no artigo "
Sentimentos confirmados por números ".
A versão demo do PVS-Studio está disponível em nosso
site .