Esta é a segunda análise de uma série de artigos sobre a verificação de programas abertos para trabalhar com o protocolo RDP. Nesta, vamos explorar o cliente rdesktop e o servidor xrdp.
Como ferramenta para identificar erros, 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, estão apresentados apenas os erros que me pareceram interessantes. No entanto, os projetos são pequenos, portanto, houve poucos erros. :)
Nota. O artigo anterior sobre a verificação do projeto FreeRDP pode ser encontrado .
rdesktop
— uma implementação livre do cliente RDP para sistemas baseados em UNIX. Ele também pode ser utilizado no Windows, se o projeto for compilado no Cygwin. Licenciado sob GPLv3.
Este cliente é bastante popular — é utilizado por padrão no ReactOS, e também é possível encontrar front-ends gráficos de terceiros para ele. No entanto, é bastante antigo: o primeiro lançamento ocorreu em 4 de abril de 2001 — na data da redação deste artigo, ele tem 17 anos.
Como já mencionei antes, o projeto é bem pequeno. Contém cerca de 30 mil linhas de código, o que é um pouco estranho, considerando sua idade. Para comparação, o FreeRDP contém 320 mil linhas. Aqui está a saída do programa Cloc:
Código inalcançável
Código inalcançável detectado. É possível que um erro esteja presente. 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 erro nos encontra logo de início na função main: vemos código seguindo a instrução return — este fragmento realiza a limpeza de memória. No entanto, o erro não representa uma ameaça: toda a memória alocada será liberada pelo sistema operacional após o término do programa.
Ausência de tratamento de erros
É possível um subfluxo de array. 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] = ' '; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}O fragmento de código, neste caso, realiza a leitura de um arquivo para o buffer até que o arquivo tenha terminado. No entanto, a ausência de tratamento de erros é notável: se algo der errado, write retornará -1 e, então, ocorrerá um estouro de buffer output.
Uso de EOF no tipo char
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++;
}
....
}Aqui vemos o tratamento incorreto ao alcançar o final do arquivo: se fgetc retornar um caractere com um código igual a 0xFF, ele será considerado como final do arquivo (EOF).
EOF esta é uma constante normalmente definida como -1. Por exemplo, na codificação CP1251, a última letra do alfabeto russo tem o código 0xFF, que corresponde ao número -1, se falarmos sobre uma variável do tipo char. Portanto, o caractere 0xFF, assim como EOF (-1), é tratado como o final do arquivo. Para evitar tais erros, o resultado da função fgetc deve ser armazenado em uma variável do tipo int.
Erros de digitação
Fragmento 1
A expressão ‘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; // <=
....
}Possivelmente, o autor desse código confundiu || e && na condição. Vamos considerar as possíveis variações dos valores write_time e change_time:
- Ambas as variáveis são iguais a 0: neste caso, entraremos no ramo else: a variável mod_time sempre será igual a 0, independentemente da condição subsequente.
- Uma das variáveis é igual a 0: mod_time será igual a 0 (desde que a outra variável tenha um valor não negativo), pois MIN escolherá o menor dos dois valores.
- Ambas as variáveis não são iguais a 0: escolhemos o valor mínimo.
Ao substituir a condição por write_time && change_time o comportamento parecerá correto:
- Uma ou ambas as variáveis não são iguais a 0: escolhemos um valor não nulo.
- Ambas as variáveis não são iguais a 0: escolhemos o valor mínimo.
Fragmento 2
A expressão é sempre verdadeira. Provavelmente o operador ‘&&’ deveria 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;
....
}Parece que aqui também houve confusão com os operadores || e &&, ou == e !=: a variável não pode assumir simultaneamente o valor 20 e 9.
Cópia ilimitada da string
Uma chamada da função ‘sprintf’ levará ao estouro do buffer ‘fullpath’. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Obter informações para a entrada do diretório */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Ao considerar a função completamente, ficará claro que esse código não causa problemas. No entanto, eles podem surgir no futuro: uma alteração descuidada, e poderemos ter um estouro de buffer — sprintf não é restrito, portanto, ao concatenar caminhos, podemos ultrapassar os limites do array. Recomenda-se observar essa chamada no snprintf(fullpath, PATH_MAX, ….).
Condição excessiva
Uma parte da expressão condicional é sempre verdadeira: adicione > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Verificação add > 0 isto não é necessário: a variável sempre será maior que zero, pois read % 4 retornará o resto da divisão, que nunca será igual a 4.
xrdp
— implementação do servidor RDP com código aberto. O projeto é dividido em 2 partes:
- xrdp — implementação do protocolo. Distribuído sob a licença Apache 2.0.
- xorgxrdp — um conjunto de drivers Xorg para uso com xrdp. Licença — X11 (como MIT, mas proíbe uso em publicidade)
O desenvolvimento do projeto é baseado nos resultados do rdesktop e FreeRDP. Inicialmente, para trabalhar com gráficos, era necessário usar um servidor VNC separado ou um servidor X11 especial com suporte a RDP — X11rdp, mas com a chegada do xorgxrdp, a necessidade deles desapareceu.
Neste artigo, não vamos abordar o xorgxrdp.
O projeto xrdp, assim como o anterior, é bem pequeno e contém cerca de 80 mil linhas.
Mais erros de digitação
O código contém a coleção de blocos similares. 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; // <=
x++;
}
....
}
....
}Esse código foi extraído da biblioteca librfxcodec, que implementa o codec jpeg2000 para funcionar com RemoteFX. Aqui, aparentemente, os canais de dados gráficos estão trocados — ao invés da cor "azul", está sendo gravada a "vermelha". Tal erro provavelmente ocorreu devido a um copy-paste.
Esse mesmo problema ocorreu em uma função similar rfx_encode_format_argb, o que também nos foi informado pelo analisador:
O código contém a coleção de blocos similares. 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 array
É possível um estouro de array. O valor do índice ‘i - 8’ pode chegar a 129. genkeymap.c 142
// evdev-map.c
int xfree86_to_evdev[137-8+1] = {
....
};
// genkeymap.c
extern int xfree86_to_evdev[137-8];
int main(int argc, char **argv)
{
....
for (i = 8; i <= 137; i++) /* Keycodes */
{
if (is_evdev)
e.keycode = xfree86_to_evdev[i-8];
....
}
....
}A declaração e definição do array nestes dois arquivos são incompatíveis — o tamanho difere em 1. No entanto, não ocorrem erros — no arquivo evdev-map.c o tamanho está correto, portanto, não há saída dos limites. Assim, isso é apenas uma falha que pode ser facilmente corrigida.
Comparação incorreta
Uma parte da expressão condicional é sempre falsa: (cap_len < 0). xrdp_caps.c 616
// common/parse.h
#if defined(B_ENDIAN) || defined(NEED_ALIGN)
#define in_uint16_le(s, v) do
....
#else
#define in_uint16_le(s, v) do
{
(v) = *((unsigned short*)((s)->p));
(s)->p += 2;
} while (0)
#endif
int
xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s)
{
int cap_len;
....
in_uint16_le(s, cap_len);
....
if ((cap_len < 0) || (cap_len > 1024 * 1024))
{
....
}
....
}Na função, ocorre a leitura de uma variável do tipo unsigned short para uma variável do tipo int. A verificação aqui não é necessária, pois estamos lendo uma variável de tipo não assinado e atribuindo o resultado a uma variável de maior tamanho, portanto, a variável não pode assumir um valor negativo.
Verificações desnecessárias
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: erro");
return 1;
}
....
}Verificações de desigualdade aqui não fazem sentido, pois já temos uma comparação no início. É bastante provável que isso seja um erro de digitação e o desenvolvedor queria usar o operador || para filtrar argumentos inválidos.
Conclusão
Durante a verificação, não foram encontradas falhas graves, mas muitos problemas menores foram identificados. No entanto, esses projetos são usados em muitos sistemas, embora sejam pequenos em volume. Em um pequeno projeto, não é necessário haver muitos erros, portanto, não devemos julgar o trabalho do analisador apenas com base em pequenos projetos. Mais detalhes sobre isso podem ser lidos no artigo "«.
Você pode baixar a versão de avaliação do PVS-Studio conosco em .
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
