Überprüfung von FreeRDP mit PVS-Studio

FreeRDP ist eine Open-Source-Implementierung des Remote Desktop Protocol (RDP), einem proprietären Protokoll von Microsoft. Das Projekt unterstützt mehrere Plattformen, darunter Windows, Linux, macOS und sogar iOS und Android. Wir haben es als erstes Projekt ausgewählt, das mit dem statischen Code-Analysator PVS-Studio für eine Artikelserie über die Überprüfung von RDP-Clients analysiert wurde.

Einige Geschichte

Das FreeRDP-Projekt wurde gestartet, nachdem Microsoft die Spezifikationen für ihr proprietäres Protokoll RDP geöffnet hatte. Zu diesem Zeitpunkt war bereits ein Client namens rdesktop im Einsatz, der hauptsächlich auf Reverse-Engineering-Arbeiten basierte.

Bei der Implementierung des Protokolls fiel es den Entwicklern aufgrund architektonischer Probleme schwer, neue Funktionen hinzuzufügen. Änderungen an der Architektur führten zu einem Konflikt zwischen den Entwicklern und führten zur Erstellung eines Forks von rdesktop namens FreeRDP. Die weitere Verbreitung wurde durch die GPLv2-Lizenz eingeschränkt, und die Autoren entschieden sich für eine erneute Lizenzierung auf Apache License v2. Einige waren jedoch nicht bereit, die Lizenz zu ändern, also beschlossen die Entwickler, die Codebasis von Grund auf neu zu schreiben, und so entstand das Projekt, wie wir es heute kennen.

Die vollständige Geschichte des Projekts ist im offiziellen Blog verfügbar:„Die Geschichte des FreeRDP-Projekts“.

Ich habe PVS-Studio verwendet, um das Projekt auf Fehler und potenzielle Schwachstellen zu scannen. PVS-Studio ist ein statischer Analysator für in C, C++, C# und Java geschriebenen Code und läuft unter Windows, Linux und macOS.

Beachten Sie, dass ich nur die Fehler besprechen werde, die mir am interessantesten erschienen.

Speicherleck

V773 Die Funktion wurde verlassen, ohne den 'cwd'-Zeiger loszulassen. Ein Speicherleck ist möglich. Umgebung.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }

  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }

  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

Dieses Snippet stammt aus dem winpr-Subsystem, das einen WINAPI-Wrapper für Nicht-Windows-Systeme implementiert, d. h. als leichteres Äquivalent zu Wine fungiert. Der obige Code enthält ein Speicherleck:den von getcwd zugewiesenen Speicher Funktion wird nur in Sonderfallzweigen freigegeben. Um dies zu beheben, sollten die Autoren einen Aufruf zu free hinzufügen nach dem Aufruf von memcpy .

Array-Index außerhalb der Grenzen

V557 Array-Überlauf ist möglich. Der Wert des 'event->EventHandlerCount'-Index könnte 32 erreichen. PubSub.c 117

#define MAX_EVENT_HANDLERS  32

struct _wEventType
{
  ....
  int EventHandlerCount;
  pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};

int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
      pEventHandler EventHandler)
{
  ....
  if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
  {
    event->EventHandlers[event->EventHandlerCount] = EventHandler;
    event->EventHandlerCount++;
  }
  ....
}

In diesem Beispiel wird der Liste ein neues Element hinzugefügt, auch wenn diese bereits die maximale Anzahl von Elementen erreicht hat. Dieser Fehler kann behoben werden, indem einfach das <= ersetzt wird Operator mit < .

Der Analysator hat einen weiteren Fehler dieses Typs gefunden:

• V557 Array-Überlauf ist möglich. Der Wert des 'iBitmapFormat'-Index könnte 8 erreichen. orders.c 2623

Tippfehler

Ausschnitt 1

V547 Ausdruck '!pipe->In' ist immer falsch. MessagePipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;

  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....

}

Was wir hier sehen, ist ein gewöhnlicher Tippfehler:Sowohl die erste als auch die zweite Bedingung prüfen dieselbe Variable. Es sieht aus wie ein Produkt von schlechtem Copy-Paste.

Ausschnitt 2

V760 Es wurden zwei identische Textblöcke gefunden. Der zweite Block beginnt ab Zeile 771. tsg.c 770

typedef struct _TSG_PACKET_VERSIONCAPS
{
  ....
  UINT16 majorVersion;
  UINT16 minorVersion;
  ....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;

static BOOL TsProxyCreateTunnelReadResponse(....)
{
  ....
  PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Ein weiterer Tippfehler:Der Kommentar besagt, dass wir die minorVersion erwarten sollten Variable, die aus dem Stream gelesen werden soll, während der Wert in die Variable majorVersion eingelesen wird . Ich kenne das Projekt jedoch nicht gut genug, um das mit Sicherheit sagen zu können.

Ausschnitt 3

V524 Es ist merkwürdig, dass der Hauptteil der Funktion „trio_index_last“ vollständig dem Hauptteil der Funktion „trio_index“ entspricht. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Wie der Kommentar schon sagt, der trio_index Die Funktion findet das erste Zeichenvorkommen in der Zeichenfolge, während die trio_index_last Funktion findet das letzte Vorkommen. Doch die Körper dieser beiden Funktionen sind genau gleich! Dies muss ein Tippfehler sein und der trio_index_last Funktion sollte wahrscheinlich strrchr zurückgeben statt strchr - In diesem Fall würde sich das Programm wie erwartet verhalten.

Ausschnitt 4

V769 Der 'data'-Zeiger im Ausdruck ist gleich nullptr. Der resultierende Wert von arithmetischen Operationen auf diesem Zeiger ist sinnlos und sollte nicht verwendet werden. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Der Entwickler muss versehentlich den Negationsoperator ! weggelassen haben vor Daten . Ich frage mich, warum es niemand früher bemerkt hat.

Ausschnitt 5

V517 Die Verwendung des Musters „if (A) {...} else if (A) {...}“ wurde erkannt. Es besteht die Wahrscheinlichkeit des Vorliegens eines logischen Fehlers. Überprüfen Sie die Zeilen:213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;

  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

Die letzten beiden Bedingungen sind gleich:Der Programmierer muss vergessen haben, die Kopie zu ändern. Der Logik des Codes nach zu urteilen, verarbeitet der letzte Teil Vier-Byte-Werte, sodass wir annehmen könnten, dass die letzte Bedingung prüfen sollte, ob Wert <=0x3FFFFFFF .

Noch ein Fehler dieser Art:

• V517 Die Verwendung des 'if (A) {...} else if (A) {...}'-Musters wurde erkannt. Es besteht die Wahrscheinlichkeit des Vorliegens eines logischen Fehlers. Überprüfen Sie die Zeilen:169, 173. file.c 169

Eingabedaten prüfen

Ausschnitt 1

V547 Ausdruck 'strcat(target, source) !=NULL' ist immer wahr. triostr.c 425

TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
     char *target,
     TRIO_CONST char *source)
{
  assert(target);
  assert(source);
  
  return (strcat(target, source) != NULL);
}

Die Überprüfung des Rückgabewerts der Funktion ist fehlerhaft. Die strcat Die Funktion gibt einen Zeiger auf die Zielzeichenfolge zurück, d. h. den ersten Parameter, der in diesem Fall Ziel ist . Aber wenn es gleich NULL ist, ist es zu spät, es zu überprüfen, da es bereits in der strcat dereferenziert wurde Funktion.

Ausschnitt 2

V547 Ausdruck 'Cache' ist immer wahr. glyph.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;

struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};

void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;

  if (cache)
  {
    ....
  }
  ....
}

In diesem Snippet der Cache Variable wird die Adresse des statischen Arrays glyphCache->glyphCache zugewiesen . Die Prüfung if (cache) kann also entfernt werden.

Ressourcenverwaltungsfehler

V1005 Die Ressource wurde mit der Funktion „CreateFileA“ erworben, aber mit der inkompatiblen Funktion „fclose“ freigegeben. certificate.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

Das fp Handle auf die Datei, die von CreateFile erstellt wurde Funktion wurde versehentlich durch den Aufruf von fclose geschlossen Funktion aus der Standardbibliothek und nicht die Funktion CloseHandle .

Identische Bedingungen

V581 Die Bedingungsausdrücke der nebeneinander stehenden if-Anweisungen sind identisch. Überprüfen Sie die Zeilen:269, 283. ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }

  if (conformant_array_description)
  {
    unsigned char array_type;
    array_type = conformant_array_description[0];
    pStubMsg->MaxCount = MaxCount;
    pStubMsg->ActualCount = ActualCount;
    pStubMsg->Offset = Offset;
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
  }
  ....
}

Dieses Snippet könnte richtig sein, aber es ist verdächtig, dass beide Bedingungen identische Nachrichten enthalten - eine davon ist wahrscheinlich unnötig.

Nullzeiger freigeben

V575 Der Nullzeiger wird an die 'freie' Funktion übergeben. Überprüfen Sie das erste Argument. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */

  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);

  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);

  if (status == SCARD_S_SUCCESS)
  {
    ....
  }

  free(mszGroupsA);
  ....
}

Die kostenlose Funktion kann auf einen Nullzeiger aufgerufen werden, und PVS-Studio weiß das. Aber wenn festgestellt wird, dass der Zeiger immer null ist, wie in diesem Ausschnitt, gibt der Analysator eine Warnung aus.

Die mszGroupsA Zeiger ist anfänglich auf NULL gesetzt und wird nirgendwo anders initialisiert. Der einzige Zweig, in dem es initialisiert werden könnte, ist nicht erreichbar.

Ein paar andere Warnungen dieser Art:

• V575 Der Nullzeiger wird an die 'freie' Funktion übergeben. Überprüfen Sie das erste Argument. lizenz.c 790
• V575 Der Nullzeiger wird an die 'freie' Funktion übergeben. Überprüfen Sie das erste Argument. rdpsnd_alsa.c 575

Aufgegebene Variablen wie diese scheinen Reste zu sein, die nach dem Refactoring zurückgeblieben sind und entfernt werden können.

Möglicher Überlauf

V1028 Möglicher Überlauf. Betrachten Sie das Casting von Operanden, nicht das Ergebnis. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);

struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};

typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;

int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Umwandlung des Ausdrucksergebnisses in long wird einen Überlauf nicht verhindern, da die Auswertung für den Wert erfolgt, während er noch vom Typ int ist .

Dereferenzierungszeiger bei Initialisierung

V595 Der 'Kontext'-Zeiger wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:746, 748. gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;

  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

Der Kontext Pointer wird während seiner Initialisierung, also vor der Prüfung, dereferenziert.

Andere Fehler dieser Art:

• V595 Der 'ntlm'-Zeiger wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:236, 255. ntlm.c 236
• V595 Der 'Kontext'-Zeiger wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:1003, 1007. rfx.c 1003
• V595 Der 'rdpei'-Zeiger wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:176, 180. rdpei_main.c 176
• V595 Der 'gdi'-Zeiger wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:121, 123. xf_gfx.c 121

Bedeutungsloser Zustand

V547 Ausdruck 'rdp->state>=CONNECTION_STATE_ACTIVE' ist immer wahr. connection.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // <=
      ....
      if (rdp->state >= CONNECTION_STATE_ACTIVE)     // <=
      {
        IFCALLRET(client->Activate, client->activated, client);

        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

Es ist leicht zu erkennen, dass die erste Bedingung keinen Sinn macht, da der betreffende Wert bereits zuvor zugewiesen wurde.

Falsche Verarbeitung von Zeichenfolgen

V576 Falsches Format. Erwägen Sie, das dritte tatsächliche Argument der Funktion „sscanf“ zu überprüfen. Es wird ein Zeiger auf den Typ unsigned int erwartet. Proxy.c 220

V560 Ein Teil des Bedingungsausdrucks ist immer wahr:(rc>=0). Proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);

  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

Dieser Code löst zwei Warnungen gleichzeitig aus. Der %u Platzhalter wird für Variablen vom Typ unsigned int verwendet , während das sub Variable ist vom Typ int . Die zweite Warnung weist auf eine verdächtige Prüfung hin:Der rechte Teil des Bedingungsausdrucks ergibt keinen Sinn, da die Variable bereits im linken Teil auf 1 geprüft wurde. Ich bin mir über die Absichten des Autors nicht sicher, aber irgendetwas stimmt offensichtlich mit diesem Code nicht.

Checks in der falschen Reihenfolge

V547 Ausdruck 'status ==0x00090314' ist immer falsch. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }

  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

Die markierten Bedingungen sind immer falsch, da die zweite Bedingung nur ausgeführt werden kann, wenn status ==SEC_E_OK . So könnte die korrekte Version aussehen:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

Schlussfolgerung

Die Überprüfung ergab viele Fehler, und die oben besprochenen sind nur die interessantesten. Zur eigenen Prüfung können die Projektentwickler gerne ein Formular für einen temporären Lizenzschlüssel auf der PVS-Studio Website einreichen. Der Analysator hat auch eine Reihe von Fehlalarmen erzeugt, die wir beheben werden, um seine Leistung zu verbessern. Beachten Sie, dass die statische Analyse unverzichtbar ist, wenn Ihr Ziel nicht nur darin besteht, die Codequalität zu verbessern, sondern auch die Suche nach Fehlern weniger zeitaufwändig zu machen - und hier wird PVS-Studio hilfreich sein.