A rdesktop és xrdp ellenőrzése a PVS-Studio elemző segítségével

Ez a második áttekintés a nyílt programok RDP protokollal való ellenőrzéséről szóló ciklusban. Itt megvizsgáljuk az rdesktop klienst és az xrdp szervert.

Hibák azonosítására használt eszközként szolgált PVS-StudioA C, C++, C# és Java nyelvekhez készült statikus kódanalizáló, amely Windows, Linux és macOS platformokon érhető el.

A cikkben csak azok a hibák szerepelnek, amelyek érdekesnek tűntek számomra. Ugyanakkor a projektek kicsik, így a hibák száma is alacsony volt : ).

Megjegyzés. Az előző cikk a FreeRDP projektről itt található itt.

rdesktop

rdesktop — egy szabad RDP kliens megvalósítása UNIX-alapú rendszerekhez. Windows alatt is használható, ha a projektet Cygwin alatt állítják össze. GPLv3 alapú licenc alatt.

Ez a kliens nagy népszerűségnek örvend — alapértelmezés szerint a ReactOS-ban használják, és azonnal megtalálhatók hozzá harmadik féltől származó grafikus frontend-ek. Ennek ellenére meglehetősen régi: az első kiadás 2001. április 4-én történt — a cikk írásakor 17 éves.

Ahogy korábban említettem, a projekt nagyon kicsi. Körülbelül 30 ezer sor kódot tartalmaz, ami kissé furcsa a korát figyelembe véve. Összehasonlításképpen, a FreeRDP 320 ezer sort tartalmaz. Íme a Cloc program kimenetele:

Elérhetetlen kód

V779 Elérhetetlen kódot észleltek. Lehetséges, hogy hiba áll fenn. 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);
}

A hiba már rögtön a következő funkcióban megjelenik main: látjuk a kódot, ami a parancs után következik return — ez a szakasz végez a memória tisztításával. Ennek ellenére a hiba nem jelent fenyegetést: az összes lefoglalt memória törlődni fog az operációs rendszer által a program befejezése után.

Hibakezelés hiánya

V557 Lehetséges az Array underrun. Az ‘n’ index értéke elérheti a -1-et. 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);
  }
  ....
}

Ebben az esetben a kódrészlet egy fájlból olvas a puffertárba addig, amíg a fájl véget nem ér. Azonban itt hibakezelés hiányzik: ha valami nem működik megfelelően, read az -1-et ad vissza, és akkor kivágás történik a tömb határain. output.

EOF karakter típus használata

V739 Az EOF-t nem szabad összehasonlítani egy ‘char’ típusú értékkel. A ‘(c = fgetc(fp))’ típusának ‘int’-nek kell lennie. 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++;
  }
  ....
}

Itt láthatjuk a fájl végét elérő feldolgozás hibás kezelését: ha fgetc visszaad egy karaktert, amelynek kódja 0xFF, akkor ez fájlvégként lesz értelmezve (EOF).

EOF ez egy állandó, amelyet általában -1-ként definiálnak. Például a CP1251 kódolásban az orosz ábécé utolsó betűje 0xFF kódú, ami -1-nek felel meg, ha int típusú változóról beszélünk. charÍgy a 0xFF karakter, akárcsak EOF (-1) fájl végét jelzi. Az ilyen hibák elkerülése érdekében a függvény fgetc kimenetének eredményét int típusú változóban kell tárolni. egész.

Elgépelések

1. rész

V547 Az 'write_time' kifejezés mindig hamis. 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; <=
  ....
}

Valószínűleg a kód szerzője összekeverte || és && a feltételt. Nézzük meg a lehetséges 'write_time' és 'change_time' értékeket. write_time és change_time:

• Mindkét változó 0: ebben az esetben a következő ágba jutunk else: a 'mod_time' változó mindig 0 lesz, függetlenül a további feltételtől. Az egyik változó 0: ez
• 0 lesz (ha a másik változó nem-negatív), mert mindig 0 lesz, függetlenül a további feltételtől. a MIN a kettő közül a legkisebbet választja. Mindkét változó nem nulla: válasszuk ki a legkisebb értéket.
• A feltételt 'write_time && change_time'-ra cserélve

a viselkedés helyesnek fog tűnni: Legalább az egyik változó nem nulla: válasszunk ki egy nem nulla értéket. A kifejezés mindig igaz. Valószínűleg az '&&' operátort kellene itt használni. 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; .... }
• A feltételt 'write_time && change_time'-ra cserélve

2. rész

V547 Valószínűleg itt is összekeverték az operátorokat

: a változó nem veheti fel egyszerre a 20 és 9 értéket.

Korlátlan karakterlánc másolás || és &&, vagy == és !=V512

A 'sprintf' függvény hívása a 'fullpath' puffer túlcsordulásához vezet. disk.c 1257

RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Az információk megszerzése a könyvtár bejegyzéséhez */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... } Ha teljes egészében megvizsgáljuk a funkciót, világossá válik, hogy ez a kód nem okoz problémát. Azonban jövőbeli problémák merülhetnek fel: egy óvatlan változtatás, és bufertúlcsordulást kapunk —

mivel nem korlátozott, a könyvtár összefűzése során túlléphetünk a tömb határain. Ajánlott észrevenni ezt a hívást

snprintf(fullpath, PATH_MAX, ….) sprintf Túlzott feltétel A feltételi kifejezés egy része mindig igaz: adjon hozzá > 0. scard.c 507.

A feltételi kifejezés túlzottan bonyolult: a feltétel mindig igaz.

V560 A feltételi kifejezés egy része mindig igaz: adjon hozzá > 0. scard.c 507

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

Ellenőrzés add > 0 itt nincs mit tenni: a változó mindig nagyobb lesz nullánál, mert read % 4 visszaadja a maradékot az osztásból, és az soha nem lesz 4.

xrdp

xrdp — nyílt forráskódú RDP szerver megvalósítása. A projekt két részre oszlik:

• xrdp — a protokoll megvalósítása. Apache 2.0 licenc alatt terjesztik.
• xorgxrdp — Xorg meghajtók készlete, amelyet az xrdp-vel való használatra terveztek. Licenc — X11 (mint az MIT, de tiltja a reklámban való használatot)

A projekt fejlesztése a rdesktop és FreeRDP eredményein alapul. Eredetileg grafika kezelésére külön VNC szerver használata volt szükséges, vagy egy speciális X11 szerver, amely támogatta az RDP-t — X11rdp, de az xorgxrdp megjelenésével erre már nincs szükség.

Ebben a cikkben nem érintjük az xorgxrdp-t.

Az xrdp projekt, mint az előző, nagyon kicsi, és körülbelül 80 ezer sort tartalmaz.

További elírások

V525 A kód hasonló blokkok gyűjteményét tartalmazza. Ellenőrizzük az 'r', 'g', 'r' elemeket a 87, 88, 89 sorokban. rfxencode_rgb_to_yuv.c 87

statikus 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++;
      }
      ....
  }
  ....
}

Ez a kód a librfxcodec könyvtárból származik, amely a RemoteFX-t támogató jpeg2000 kodeket valósítja meg. Itt nyilvánvalóan összekeverték a grafikai adatvonalakat — a 'kék' szín helyett a 'piros' színt írják be. Ez a hiba valószínűleg a másolás-beillesztés következménye.

Ez a probléma a hasonló függvényt is érintette rfx_encode_format_argb, amiről az analizáló is tájékoztatott minket:

V525 A kód hasonló blokkok gyűjteményét tartalmazza. Ellenőrizzük az 'a', 'r', 'g', 'r' elemeket a 260, 261, 262, 263 sorokban. rfxencode_rgb_to_yuv.c 260

while (x < 64)
{
    *la_buf++ = a;
    *lr_buf++ = r;
    *lg_buf++ = g;
    *lb_buf++ = r;
    x++;
}

Tömb deklarálása

V557 Tömb túlcsordulás lehetséges. Az ‘i - 8’ index értéke 129-re juthat. 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 tömb deklarálása és meghatározása e két fájlban nem kompatibilis — a méret 1-gyel eltér. Azonban hibák nem fordulnak elő — az evdev-map.c fájlban a megfelelő méret van megadva, ezért nincs határon túllépés. Ez tehát csak egy figyelmetlen hiba, amelyet könnyen lehet javítani.

Hibás összehasonlítás

V560 A feltételes kifejezés egy része mindig hamis: (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))
  {
    ....
  }
  ....
}

A függvény içlet olvas egy változót, amely típusa unsigned short egy olyan változóba, amely típusa egészItt nincs szükség ellenőrzésre, mivel a változót előjel nélküli típusúként olvassuk be, és az eredményt egy nagyobb méretű változónak adjuk, ezért a változó nem vehet fel negatív értéket.

Felesleges ellenőrzések

V560 A feltételes kifejezés egy része mindig igaz: (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: hiba");
      return 1;
  }
  ....
}

Az egyenlőtlenség ellenőrzések itt nem relevánsak, mivel már van összehasonlítás az elején. Valószínű, hogy elírás történt, és a fejlesztő a || szűrő hibás argumentumokat akarta használni.

Zárás

Az ellenőrzés során nem találtunk komoly hibákat, de sok apróbb problémára bukkantunk. Ennek ellenére ezek a projektek sok rendszerben használatosak, még ha kis volumenűek is. Egy kis projekt nem feltétlenül tartalmazhat sok hibát, ezért nem szabad az elemző munkáját csak kis projektek alapján megítélni. Erről többet olvashat a "Az érzések, amelyeket a számok megerősítettek«.

Ingyenes próbaverziót tölthet le a PVS-Studio-ból nálunk a weboldalon.

Ha szeretné megosztani ezt a cikket angol nyelvű közönséggel, kérjük, használja a fordítás hivatkozását: Sergey Larin. A rdesktop és xrdp ellenőrzése PVS-Studio-val

Forrás: habr.com