ProHoster > Blog > Administration > Tjek af rdesktop og xrdp med PVS-Studio analysator.

Tjek af rdesktop og xrdp med PVS-Studio analysator.

Kontrol af rdesktop og xrdp ved hjĂŠlp af PVS-Studio analysator
Dette er den anden anmeldelse i rÊkken af artikler om test af Äbne programmer til RDP-protokollen. Her ser vi pÄ klienten rdesktop og serveren xrdp.

Til fejlanalyse blev der anvendt PVS-Studio. Dette er en statisk kodeanalysator til C-, C++-, C#- og Java-sprog, tilgÊngelig pÄ platforme som Windows, Linux og macOS.

Artiklen prÊsenterer kun de fejl, der syntes interessante for mig. Men projekterne er smÄ, sÄ der var ikke mange fejl. :)

BemĂŠrk. Den tidligere artikel om test af FreeRDP-projektet kan findes her.

rdesktop

rdesktop — en fri implementering af RDP-klienten til UNIX-baserede systemer. Den kan ogsĂ„ bruges pĂ„ Windows, hvis projektet samles under Cygwin. Licenseret under GPLv3.

Denne klient har stor popularitet — den bruges som standard i ReactOS, og der findes ogsĂ„ tredjeparts grafiske front-end'er til den. Ikke desto mindre er den temmelig gammel: den fĂžrste udgivelse fandt sted den 4. april 2001 — pĂ„ tidspunktet for denne artikel er den 17 Ă„r gammel.

Som jeg nĂŠvnte tidligere, er projektet ganske lille. Det indeholder cirka 30.000 linjer kode, hvilket er lidt mĂŠrkeligt i betragtning af dets alder. Til sammenligning indeholder FreeRDP 320.000 linjer. Her er output fra programmet Cloc:

Kontrol af rdesktop og xrdp ved hjĂŠlp af PVS-Studio analysator

UopnÄelig kode

V779 UopnÄelig kode registreret. Det er muligt, at der er en fejl til stede. 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);
}

Fejlen rammer os straks i funktionen main: vi ser koden, der fþlger efter operatoren returner — dette fragment rydder op i hukommelsen. Ikke desto mindre udgþr fejlen ingen trussel: al tildelt hukommelse vil blive ryddet af operativsystemet efter programmets afslutning.

Manglende fejlhÄndtering

V557 Array underrun er muligt. Indekset ‘n’ kunne nĂ„ -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);
  }
  ....
}

Kodefragmentet lÊser her fra en fil til en buffer, indtil filen er slut. Men fejlhÄndtering mangler her: hvis noget gÄr galt, sÄ write vil det returnere -1, og sÄ vil der ske et overtrÊdelse af array-grÊnsen output.

Brug af EOF i char-type

V739 EOF skal ikke sammenlignes med en vĂŠrdi af 'char' typen. '(c = fgetc(fp))' bĂžr vĂŠre af 'int' typen. 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++;
  }
  ....
}

Her ser vi en ukorrekt hÄndtering af enden af filen: hvis fgetc returnerer et tegn, hvis kode er lig med 0xFF, vil det blive opfattet som enden af filen (EOF).

EOF det er en konstant, der normalt defineres som -1. For eksempel har den sidste bogstav i det russiske alfabet i kodning CP1251 koden 0xFF, hvilket svarer til tallet -1, hvis vi taler om en variabel af typen char. Det viser sig, at tegnet 0xFF, ligesom EOF (-1) opfattes som enden af filen. For at undgÄ sÄdanne fejl bÞr resultatet af funktionen fgetc opbevares i en variabel af typen int.

SlÄfejl

Fragment 1

V547 Udtryk ‘write_time’ er altid falsk. 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; <=
  ....
}

MĂ„ske forvekslede forfatteren af denne kode || og && i betingelsen. Lad os overveje mulige vĂŠrdier for write_time og change_time:

  • Begge variabler er lig med 0: i dette tilfĂŠlde vil vi ende i grenen ellers: variabel mod_time vil altid vĂŠre lig med 0 uanset den efterfĂžlgende betingelse.
  • En af variablerne er lig med 0: mod_time vil vĂŠre lig med 0 (forudsat at den anden variabel har en ikke-negativ vĂŠrdi), da MIN vĂŠlger den mindste af de to muligheder.
  • Begge variabler er ikke lig med 0: vĂŠlg den mindste vĂŠrdi.

Ved at ĂŠndre betingelsen til write_time && change_time vil adfĂŠrden se korrekt ud:

  • En eller begge variabler er ikke lig med 0: vĂŠlg en ikke-null vĂŠrdi.
  • Begge variabler er ikke lig med 0: vĂŠlg den mindste vĂŠrdi.

Fragment 2

V547 Udtryk er altid sandt. Sandsynligvis skal ‘&&’ operatoren bruges her. 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;
  ....
}

Det ser ud til, at operatorerne ogsÄ er forvekslet her: || og &&, eller == og !=variablen kan ikke samtidig have vÊrdien 20 og 9.

UbegrĂŠnset kopiering af streng

V512 Et kald til ‘sprintf’ funktionen vil fþre til overfyldning af bufferen ‘fullpath’. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Hent oplysninger for katalogpost */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Ved at se pĂ„ funktionen helt vil det blive klart, at denne kode ikke forĂ„rsager problemer. Men de kan opstĂ„ i fremtiden: en uforsigtig ĂŠndring, og vi fĂ„r buffer-overfyldning — sprintf er ikke begrĂŠnset, sĂ„ ved sammenkĂŠdning af stier kan vi overskride grĂŠnserne for arrayet. Det anbefales at bemĂŠrke dette kald til snprintf(fullpath, PATH_MAX, 
.).

Overskydende betingelse

V560 En del af den betingede udtryk er altid sandt: tilfĂžj > 0. scard.c 507

static void
inRepos(STREAM in, unsigned int read)
{
  SERVER_DWORD add = 4 - read % 4;
  if (add  0)
  {
    ....
  }
}

Kontrol add > 0 her er ingen grund: variablen vil altid vĂŠre stĂžrre end nul, da read % 4 vil returnere resten af divisionen, og den vil aldrig vĂŠre lig med 4.

xrdp

xrdp — implementering af RDP-server med Ă„ben kildekode. Projektet er opdelt i 2 dele:

  • xrdp — implementeringen af protokollen. Distribueres under Apache 2.0 licensen.
  • xorgxrdp — et sĂŠt Xorg-drivere til brug med xrdp. Licens — X11 (som MIT, men forbyder brug i reklame)

Udviklingen af projektet er baseret pĂ„ resultaterne fra rdesktop og FreeRDP. Oprindeligt mĂ„tte man bruge en separat VNC-server eller en sĂŠrlig X11-server med RDP-understĂžttelse — X11rdp, men med fremkomsten af xorgxrdp er behovet for dem forsvundet.

I denne artikel vil vi ikke komme ind pÄ xorgxrdp.

Projektet xrdp, ligesom det foregÄende, er ret lille og indeholder cirka 80.000 linjer.

Kontrol af rdesktop og xrdp ved hjĂŠlp af PVS-Studio analysator

Flere tastefejl

V525 Koden indeholder samlingen af lignende blokke. Tjek elementer ‘r’, ‘g’, ‘r’ i linjer 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++;
      }
      ....
  }
  ....
}

Denne kode er hentet fra biblioteket librfxcodec, der implementerer en jpeg2000 codec til brug med RemoteFX. Her ser det ud til, at de grafiske datakanaler er blevet forvekslet — i stedet for 'blĂ„' farve gemmes 'rĂžd'. En sĂ„dan fejl opstod sandsynligvis som fĂžlge af copy-paste.

Det samme problem ramte ogsÄ en lignende funktion rfx_encode_format_argb, som analysatoren ogsÄ informerede os om:

V525 Koden indeholder samlingen af lignende blokke. Tjek elementer ‘a’, ‘r’, ‘g’, ‘r’ i linjer 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++;
}

Deklaration af array

V557 Array-overtrÊdelse er muligt. VÊrdien af 'i - 8' indekset kunne nÄ 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];
    ....
  }
  ....
}

Deklarationen og definitionen af arrayet i disse to filer er inkompatible — stĂžrrelsen adskiller sig med 1. Men der opstĂ„r ikke fejl — i filen evdev-map.c er den korrekte stĂžrrelse angivet, sĂ„ der er ingen grĂŠnseoverskridelse. SĂ„ dette er bare en detalje, som er let at rette.

Ukorrekt sammenligning

V560 En del af den betingede udtryk er altid falsk: (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))
  {
    ....
  }
  ....
}

I funktionen sker der lÊsning af en variabel af typen unsigned short til en variabel af typen intHer er ingen kontrol nÞdvendig, da vi lÊser den usignede variabel og tildeler resultatet til en stÞrre variabel, sÄ variablen kan ikke fÄ en negativ vÊrdi.

UnĂždvendige kontroller

V560 En del af den betingede udtryk er altid sand: (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: fejltagning");
      return 1;
  }
  ....
}

Kontroller for ulighed her giver ingen mening, da vi allerede har en sammenligning i begyndelsen. Det er sandsynligt, at dette er en fejltagning, og udvikleren Ăžnskede at bruge operatĂžren || for at filtrere ugyldige argumenter.

Konklusion

Under kontrollen blev der ikke fundet alvorlige fejl, men mange smÄfejl blev identificeret. Ikke desto mindre anvendes disse projekter i mange systemer, omend de er smÄ i omfang. I et lille projekt behÞver der ikke vÊre mange fejl, sÄ man skal ikke dÞmme analysatorens arbejde kun ud fra smÄ projekter. LÊs mere om dette i artiklen "FÞlelser, der blev bekrÊftet af tallene«.

Du kan downloade en prÞveversion af PVS-Studio hos os pÄ websitet.

Kontrol af rdesktop og xrdp ved hjĂŠlp af PVS-Studio analysator

Hvis du Ăžnsker at dele denne artikel med et engelsktalende publikum, bedes du bruge linket til oversĂŠttelsen: Sergey Larin. Kontrol af rdesktop og xrdp med PVS-Studio

Kilde: habr.com

KĂžb pĂ„lidelig webhosting med DDoS-beskyttelse, VPS VDS-servere đŸ”„ KĂžb pĂ„lidelig webhosting med DDoS-beskyttelse, VPS VDS-servere | ProHoster