ProHoster > Blogg > Administration > Kontroll av rdesktop och xrdp med hjÀlp av PVS-Studio-analysatorn

Kontroll av rdesktop och xrdp med hjÀlp av PVS-Studio-analysatorn

Kontroll av rdesktop och xrdp med PVS-Studio-analysatorn
Detta Àr den andra översynen i en serie artiklar som kontrollerar öppna program för att arbeta med RDP-protokollet. HÀr kommer vi att titta pÄ klienten rdesktop och servern xrdp.

Som verktyg för att upptÀcka fel anvÀndes PVS-Studio. Detta Àr en statisk kodanalysator för sprÄken C, C++, C# och Java, tillgÀnglig pÄ plattformarna Windows, Linux och macOS.

Artikeln presenterar endast de fel som verkade intressanta för mig. ÄndĂ„ Ă€r projekten smĂ„, sĂ„ det fanns inte mĂ„nga fel :),

AnmÀrkning. Den föregÄende artikeln om kontrollen av projektet FreeRDP hittar du hÀr.

rdesktop

rdesktop — en fri implementation av RDP-klienten för UNIX-baserade system. Den kan ocksĂ„ anvĂ€ndas pĂ„ Windows om projektet byggs under Cygwin. Licensierad under GPLv3.

Denna klient Ă€r mycket populĂ€r — den anvĂ€nds som standard i ReactOS, och det finns Ă€ven externa grafiska front-end:er tillgĂ€ngliga för den. ÄndĂ„ Ă€r den ganska gammal: den första versionen slĂ€pptes den 4 april 2001 — vid tidpunkten för skrivandet av artikeln Ă€r dess Ă„lder 17 Ă„r.

Som jag redan nÀmnt tidigare Àr projektet mycket litet. Det innehÄller cirka 30 000 rader kod, vilket Àr lite mÀrkligt med tanke pÄ dess Älder. JÀmfört med detta innehÄller FreeRDP 320 000 rader. HÀr Àr utdata frÄn programmet Cloc:

Kontroll av rdesktop och xrdp med PVS-Studio-analysatorn

OĂ„tkomlig kod

V779 OÄtkomlig kod detekterad. Det Àr möjligt att ett fel Àr nÀrvarande. 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);
}

Felet möter oss genast i funktionen main: vi ser koden som följer efter operatorn Ă„tervĂ€nda — detta fragment utför minnesrensningen. ÄndĂ„ utgör felet inget hot: allt allokerat minne kommer att rensas av operativsystemet efter programmets avslut.

Brist pÄ felhantering

V557 Array-underrun Àr möjligt. VÀrdet av 'n'-indexet kan 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);
  }
  ....
}

Kodfragmentet i detta fall lÀser frÄn en fil till en buffert tills filen tar slut. Men det finns ingen felhantering hÀr: om nÄgot gÄr fel, sÄ lÀs kommer det att returnera -1, och dÄ kommer en överskridning av arrayen att intrÀffa. output.

AnvÀndning av EOF i typ char

V739 EOF bör inte jÀmföras med ett vÀrde av typen 'char'. '(c = fgetc(fp))' bör vara av typ '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++;
  }
  ....
}

HÀr ser vi en felaktig hantering av slutet av filen: om fgetc returnerar ett tecken vars kod Àr 0xFF, kommer det att uppfattas som slutet av filen (EOF).

EOF detta Àr en konstant som vanligtvis definieras som -1. Till exempel, i CP1251-kodningen har den sista bokstaven i det ryska alfabetet koden 0xFF, vilket motsvarar talet -1, om vi pratar om en variabel av typen char. SÄledes uppfattas tecknet 0xFF, liksom EOF (-1) som slutet av filen. För att undvika sÄdana fel bör resultatet av funktionen fgetc lagras i en variabel av typen int.

Tryckfel

Fragment 1

V547 Utdatans ‘write_time’ Ă€r alltid 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; // <=
  ....
}

Kanske har författaren till denna kod förvÀxlat || och && i villkoret. LÄt oss övervÀga möjliga vÀrden pÄ write_time och change_time:

  • BĂ„da variablerna Ă€r lika med 0: i detta fall hamnar vi i grenen else: variabeln mod_time kommer alltid att vara 0 oavsett det efterföljande villkoret.
  • En av variablerna Ă€r lika med 0: mod_time kommer att vara 0 (under förutsĂ€ttning att den andra variabeln har ett icke-negativt vĂ€rde), eftersom MIN vĂ€ljer det minsta av de tvĂ„ alternativen.
  • BĂ„da variablerna Ă€r inte lika med 0: vi vĂ€ljer det minsta vĂ€rdet.

Vid Àndring av villkoret till write_time && change_time kommer beteendet att se korrekt ut:

  • En eller bĂ„da variablerna Ă€r inte lika med 0: vi vĂ€ljer ett icke-nollvĂ€rde.
  • BĂ„da variablerna Ă€r inte lika med 0: vi vĂ€ljer det minsta vĂ€rdet.

Fragment 2

V547 Utdatans Ă€r alltid sann. Kanske bör ‘&&’ operatören anvĂ€ndas hĂ€r. 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;
  ....
}

Uppenbarligen Àr Àven hÀr operatörerna förvÀxlade || och &&, eller == och !=: variabeln kan inte samtidigt anta vÀrdet 20 och 9.

ObegrÀnsad kopiering av strÀng

V512 Ett samtal till funktionen 'sprintf' kommer att leda till ett överflöde av bufferten 'fullpath'. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* HÀmta information för katalogposten */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

NĂ€r vi betraktar funktionen i sin helhet blir det klart att denna kod inte orsakar problem. Men problem kan uppstĂ„ i framtiden: en ovarsam förĂ€ndring, och vi fĂ„r ett buffertöverflöde — sprintf inte begrĂ€nsad av nĂ„got, sĂ„ vid sammanfogning av stigar kan vi gĂ„ utanför array-grĂ€nserna. Det rekommenderas att notera detta anrop pĂ„ snprintf(fullpath, PATH_MAX, 
.).

Överflödigt villkor

V560 En del av det villkorsbundna uttrycket Àr alltid sant: lÀgg till > 0. scard.c 507

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

Kontroll add > 0 Àr irrelevant hÀr: variabeln kommer alltid att vara större Àn noll, eftersom read % 4 Äterkommer med resten av divisionen, och den kommer aldrig att vara lika med 4.

xrdp

xrdp — en implementation av RDP-server med öppen kĂ€llkod. Projektet Ă€r delat i tvĂ„ delar:

  • xrdp — en implementation av protokollet. Distribueras under Apache 2.0-licensen.
  • xorgxrdp — en uppsĂ€ttning Xorg-drivrutiner för anvĂ€ndning med xrdp. Licens — X11 (liknande MIT, men förbjuder anvĂ€ndning i reklam)

Utvecklingen av projektet bygger pĂ„ resultaten frĂ„n rdesktop och FreeRDP. Inledningsvis var det nödvĂ€ndigt att anvĂ€nda en separat VNC-server för grafik, eller en speciell X11-server med stöd för RDP — X11rdp, men med introduktionen av xorgxrdp försvann behovet av dem.

I den hÀr artikeln kommer vi inte att ta upp xorgxrdp.

Projektet xrdp, liksom det föregÄende, Àr ganska litet och innehÄller ungefÀr 80 000 rader.

Kontroll av rdesktop och xrdp med PVS-Studio-analysatorn

Fler skrivfel

V525 Koden innehÄller en samling liknande block. Kontrollera föremÄlen 'r', 'g', 'r' i raderna 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++;
      }
      ....
  }
  ....
}

Denna kod togs frĂ„n biblioteket librfxcodec, som implementerar jpeg2000-kodeken för RemoteFX. HĂ€r verkar det som att kanalerna för grafiska data har blandats — istĂ€llet för 'blĂ„' fĂ€rg skrivs 'röd'. Ett sĂ„dant fel uppstod troligen pĂ„ grund av copy-paste.

Samma problem uppstod ocksÄ i en liknande funktion rfx_encode_format_argb, vilket ocksÄ meddelades av analysatorn:

V525 Koden innehÄller en samling liknande block. Kontrollera föremÄlen 'a', 'r', 'g', 'r' i raderna 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 av array

V557 Arrayöverskridning Àr möjlig. VÀrdet av 'i - 8'-indexet kan 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 och definitionen av arrayen i dessa tvĂ„ filer Ă€r inkompatibla — storleken skiljer sig med 1. Men inga fel intrĂ€ffar — i filen evdev-map.c anges den korrekta storleken, sĂ„ det finns ingen övertrĂ€delse. SĂ„ det Ă€r bara en brist som Ă€r lĂ€tt att Ă„tgĂ€rda.

Felaktig jÀmförelse

V560 Ett villkorligt uttryck Àr alltid falskt: (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 lÀses en variabel av typen unsigned short till en variabel av typen int. Kontroll hÀr behövs inte, eftersom vi lÀser en obekant typ och tilldelar resultatet till en större variabel, sÄ variabeln kan inte fÄ ett negativt vÀrde.

Onödiga kontroller

V560 Ett villkorligt uttryck Àr alltid sant: (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: fel");
      return 1;
  }
  ....
}

OjÀmlikhetskontroller hÀr har ingen mening, eftersom vi redan har en jÀmförelse i början. Det Àr möjligt att detta Àr ett skrivfel och utvecklaren ville anvÀnda operatorn || för att filtrera bort ogiltiga argument.

Slutsats

Vid granskningen upptÀcktes inga allvarliga fel, men mÄnga mindre brister hittades. Trots detta anvÀnds dessa projekt i mÄnga system, om Àn smÄ i sin omfattning. I ett litet projekt behöver det inte finnas mÄnga fel, sÄ man bör inte döma analysatorns arbete endast pÄ smÄ projekt. Mer om detta kan lÀsas i artikeln "KÀnslor som bekrÀftades av siffrorna«.

Du kan ladda ner en testversion av PVS-Studio frÄn oss pÄ webbplats.

Kontroll av rdesktop och xrdp med PVS-Studio-analysatorn

Om du vill dela denna artikel med en engelsktalande publik, vÀnligen anvÀnd lÀnken till översÀttningen: Sergey Larin. Kontrollerar rdesktop och xrdp med PVS-Studio

KĂ€lla: habr.com

Köp pĂ„litlig webbhotell med DDoS-skydd, VPS VDS-servrar đŸ”„ Köp pĂ„litlig webbhotell med DDoS-skydd, VPS VDS-servrar | ProHoster