FreeRDP ist eine Open Source-Implementierung des Remote Desktop Protocol (RDP), eines Protokolls, das die von Microsoft entwickelte Remotecomputersteuerung implementiert. Das Projekt unterstützt viele Plattformen, einschließlich Windows, Linux, MacOS und sogar iOS mit Android. Dieses Projekt wurde als erstes in einer Reihe von Artikeln ausgewählt, die sich mit der Überprüfung von RDP-Clients mithilfe des statischen Analysators PVS-Studio befassen.
Ein bisschen Geschichte
Das
FreeRDP- Projekt entstand, nachdem Microsoft die Spezifikationen seines proprietären RDP-Protokolls enthüllt hatte. Zu dieser Zeit gab es einen rdesktop-Client, dessen Implementierung auf den Ergebnissen von Reverse Engineering basiert.
Bei der Implementierung des Protokolls wurde es aufgrund der damals vorhandenen Projektarchitektur schwieriger, neue Funktionen hinzuzufügen. Änderungen führten zu einem Konflikt zwischen Entwicklern, der zur Erstellung der rdesktop-Gabel - FreeRDP führte. Die weitere Verbreitung des Produkts wurde durch die GPLv2-Lizenz eingeschränkt, weshalb beschlossen wurde, eine erneute Lizenzierung an Apache License v2 vorzunehmen. Da sich jedoch nicht alle bereit erklärten, die Lizenz ihres Codes zu ändern, beschlossen die Entwickler, das Projekt neu zu schreiben, wodurch wir ein modernes Erscheinungsbild der Codebasis haben.
Weitere Details zur Projektgeschichte finden Sie im offiziellen Blog-Hinweis: „Die Geschichte des FreeRDP-Projekts“.
PVS-Studio wurde als Tool zum Erkennen von Fehlern und potenziellen Schwachstellen im Code verwendet. Es ist ein statischer Code-Analysator für C, C ++, C # und Java, der unter Windows, Linux und macOS verfügbar ist.
Der Artikel präsentiert nur die Fehler, die mir am interessantesten erschienen.
Speicherverlust
V773 Die Funktion wurde beendet, ohne den Zeiger 'cwd' loszulassen. Ein Speicherverlust ist möglich. Umwelt.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 Fragment wurde aus dem winpr-Subsystem entnommen, das den WINAPI-Wrapper für Nicht-Windows-Systeme implementiert, d. H. Es ist ein leichtes Gegenstück zu Wein. Hier liegt ein Leck vor: Der von der Funktion
getcwd zugewiesene Speicher wird nur in besonderen Fällen freigegeben. Um den Fehler zu beheben, fügen Sie einen Anruf hinzu,
um nach
memcpy freizugeben .
Über die Grenzen eines Arrays hinausgehen
V557 Array-Überlauf ist möglich. Der Wert des Index 'event-> EventHandlerCount' 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 die Anzahl der Elemente das Maximum erreicht hat. Es reicht aus, den Operator
<= durch
< zu ersetzen, um die Grenzen des Arrays nicht zu überschreiten.
Ein weiterer Fehler dieses Typs wurde gefunden:
- V557 Array-Überlauf ist möglich. Der Wert des 'iBitmapFormat'-Index könnte 8 Bestellungen erreichen.c 2623
Tippfehler
Fragment 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)
Hier sehen wir den üblichen Tippfehler: In der zweiten Bedingung wird dieselbe Variable wie in der ersten überprüft. Höchstwahrscheinlich trat der Fehler als Ergebnis eines erfolglosen Kopierens des Codes auf.
Fragment 2
V760 Es wurden zwei identische Textblöcke gefunden. Der zweite Block beginnt in 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; .... Stream_Read_UINT16(pdu->s, versionCaps->majorVersion); Stream_Read_UINT16(pdu->s, versionCaps->majorVersion); .... }
Ein weiterer Tippfehler: Ein Kommentar zum Code impliziert, dass
minorVersion aus dem Stream stammen sollte. Das Lesen erfolgt jedoch in einer Variablen namens
majorVersion . Ich bin jedoch nicht mit dem Protokoll vertraut, daher ist dies nur eine Annahme.
Fragment 3
V524 Es ist seltsam, dass der Hauptteil der Funktion 'trio_index_last' dem Hauptteil der Funktion 'trio_index' vollständig entspricht. triostr.c 933
TRIO_PUBLIC_STRING char * trio_index TRIO_ARGS2((string, character), TRIO_CONST char *string, int character) { assert(string); return strchr(string, character); } TRIO_PUBLIC_STRING char * trio_index_last TRIO_ARGS2((string, character), TRIO_CONST char *string, int character) { assert(string); return strchr(string, character); }
Dem Kommentar nach zu urteilen, findet die Funktion
trio_index die erste Übereinstimmung eines Zeichens in einer Zeichenfolge, wenn
trio_index_last die letzte ist. Aber die Körper dieser Funktionen sind identisch! Dies ist höchstwahrscheinlich ein Tippfehler, und in der Funktion
trio_index_last müssen
Sie strrchr anstelle von
strchr verwenden . Dann wird das Verhalten erwartet.
Fragment 4
V769 Der '
Daten' -Zeiger im Ausdruck entspricht nullptr. Der resultierende Wert von arithmetischen Operationen für diesen 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; .... }
Sieht so aus, als hätten sie versehentlich den Negationsoperator verpasst
! neben
Daten . Es ist seltsam, dass dies unbemerkt blieb.
Fragment 5
V517 Die Verwendung des
Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit 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 die gleichen: Anscheinend hat jemand vergessen, sie nach dem Kopieren zu überprüfen. Laut Code fällt auf, dass der letzte Teil mit Vier-Byte-Werten arbeitet, sodass wir davon ausgehen können, dass die letzte Bedingung der
Wert <= 0x3FFFFFFF sein sollte .
Ein weiterer Fehler dieses Typs wurde gefunden:
- V517 Die Verwendung des Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. Überprüfen Sie die Zeilen: 169, 173. file.c 169
Eingabevalidierung
Fragment 1
V547 Der Ausdruck 'strcat (Ziel, Quelle)! = 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); }
Das Überprüfen des Ergebnisses der Funktion in diesem Beispiel ist falsch. Die
strcat- Funktion gibt einen Zeiger auf die endgültige Version der Zeichenfolge zurück, d. H. erster Parameter übergeben. In diesem Fall ist es das
Ziel . Wenn es jedoch
NULL ist , ist es zu spät, dies zu überprüfen, da es in der
strcat- Funktion dereferenziert wird.
Fragment 2
V547 Ausdruck 'Cache' ist immer wahr. Glyphe 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 Fall wird der
Cache- Variablen die Adresse des statischen Arrays
glyphCache-> glyphCache zugewiesen . Somit kann die
if (Cache) -Prüfung weggelassen werden.
Ressourcenverwaltungsfehler
V1005 Die Ressource wurde mit der Funktion 'CreateFileA' erfasst, aber mit der inkompatiblen Funktion 'fclose' freigegeben. Zertifikat.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; } .... }
Der durch Aufrufen der
CreateFile- Funktion erstellte
fp- Dateideskriptor wird fälschlicherweise von der
fclose- Funktion aus der Standardbibliothek und nicht von
CloseHandle geschlossen .
Gleiche Bedingungen
V581 Die bedingten Ausdrücke der nebeneinander angeordneten 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); } .... }
Vielleicht ist dieses Beispiel kein Fehler. Beide Bedingungen enthalten jedoch dieselbe Nachricht, von der höchstwahrscheinlich eine entfernt werden kann.
Nullzeiger löschen
V575 Der Nullzeiger wird an die Funktion 'frei' ü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; 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); .... }
Sie können einen Nullzeiger an die
freie Funktion übergeben, und der Analysator weiß davon. Wenn jedoch eine Situation aufgedeckt wird, in der der Zeiger wie in diesem Fragment immer auf Null gesetzt wird, wird eine Warnung ausgegeben.
Der Zeiger
mszGroupsA ist anfangs
NULL und wird nirgendwo anders initialisiert. Der einzige Codezweig, in dem der Zeiger initialisiert werden könnte, ist nicht erreichbar.
Es gab andere Beiträge wie diesen:
- V575 Der Nullzeiger wird an die Funktion 'frei' übergeben. Überprüfen Sie das erste Argument. Lizenz.c 790
- V575 Der Nullzeiger wird an die Funktion 'frei' übergeben. Überprüfen Sie das erste Argument. rdpsnd_alsa.c 575
Solche vergessenen Variablen entstehen höchstwahrscheinlich beim Refactoring und können einfach gelöscht werden.
Möglicher Überlauf
V1028 Möglicher Überlauf. Betrachten Sie das Casting von Operanden, nicht das Ergebnis. makecert.c 1087
Ein zu
langes Ergebnis
ist kein Schutz vor Überlauf, da die Berechnung selbst mit dem Typ
int durchgeführt wird .
Zeiger-Dereferenzierung bei der 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; .... }
Hier wird der
Kontextzeiger bei der Initialisierung dereferenziert - bevor er überprüft wird.
Andere Fehler dieses Typs wurden gefunden:
- V595 Der Zeiger 'ntlm' 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 Zeiger 'rdpei' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 176, 180. rdpei_main.c 176
- V595 Der Zeiger 'gdi' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 121, 123. xf_gfx.c 121
Sinnloser Zustand
V547 Der Ausdruck 'rdp-> state> = CONNECTION_STATE_ACTIVE' ist immer wahr. Verbindung.c 1489
int rdp_server_transition_to_state(rdpRdp* rdp, int state) { .... switch (state) { .... case CONNECTION_STATE_ACTIVE: rdp->state = CONNECTION_STATE_ACTIVE;
Es ist leicht zu erkennen, dass die erste Bedingung aufgrund der früheren Zuweisung des entsprechenden Wertes keinen Sinn ergibt.
Falsche Zeilenanalyse
V576 Falsches Format. Überprüfen Sie das dritte tatsächliche Argument der Funktion 'sscanf'. Ein Zeiger auf den vorzeichenlosen int-Typ wird erwartet. proxy.c 220
V560 Ein Teil des bedingten Ausdrucks 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)) { .... } .... }
Der Analysator für dieses Fragment gibt sofort 2 Warnungen aus. Der
% u- Bezeichner erwartet eine Variable vom Typ
unsigned int , aber die Variable
sub ist vom Typ
int . Als nächstes sehen wir eine verdächtige Überprüfung: Die Bedingung auf der rechten Seite macht keinen Sinn, da am Anfang ein Vergleich mit der Einheit besteht. Ich weiß nicht, was der Autor dieses Codes vorhatte, aber hier stimmt eindeutig etwas nicht.
Ungeordnete Schecks
V547 Der 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)
Die markierten Bedingungen sind immer falsch, da die Ausführung nur dann die zweite Bedingung erreicht, wenn
status == SEC_E_OK . Der richtige Code könnte folgendermaßen 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; }
Fazit
Die Überprüfung des Projekts ergab daher viele Probleme, aber nur der interessanteste Teil wurde in dem Artikel beschrieben. Projektentwickler können das Projekt selbst überprüfen, indem sie auf der
PVS-Studio- Website einen temporären Lizenzschlüssel anfordern. Es gab auch falsch positive Ergebnisse, an denen gearbeitet wurde, um den Analysator zu verbessern. Eine statische Analyse ist jedoch wichtig, wenn Sie nicht nur die Qualität des Codes verbessern, sondern auch die Zeit für die Suche nach Fehlern verkürzen möchten. PVS-Studio kann Ihnen dabei helfen.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Larin.
Überprüfen von FreeRDP mit PVS-Studio