Geekly articles weekly

Überprüfen von FreeRDP mit PVS-Studio

Bild 2

FreeRDP ist eine Open-Source-Implementierung des Remote Desktop Protocol (RDP), eines proprietären Protokolls 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 Reihe von Artikeln über die Überprüfungen von RDP-Clients analysiert wurde.

Etwas Geschichte


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

Bei der Implementierung des Protokolls fiel es den Entwicklern aufgrund von Architekturproblemen schwer, neue Funktionen hinzuzufügen. Änderungen an der Architektur führten zu einem Konflikt zwischen den Entwicklern und führten zur Schaffung einer Verzweigung von rdesktop, die als FreeRDP bekannt ist. Die weitere Verbreitung wurde durch die GPLv2-Lizenz eingeschränkt, und die Autoren entschieden sich für eine erneute Lizenzierung an Apache License v2. Einige waren jedoch nicht bereit, die Lizenz zu ändern, und so 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 finden Sie im offiziellen Blog: " Die Geschichte des FreeRDP-Projekts ".

Ich habe PVS-Studio verwendet , um das Projekt nach Fehlern und potenziellen Schwachstellen zu durchsuchen. PVS-Studio ist ein statischer Analysator für Code, der in C, C ++, C # und Java geschrieben wurde und unter Windows, Linux und macOS ausgeführt wird.

Beachten Sie, dass ich nur die Fehler besprechen werde, die für mich am interessantesten waren.

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 Snippet stammt aus dem winpr-Subsystem, das einen WINAPI-Wrapper für Nicht-Windows-Systeme implementiert, dh als leichteres Äquivalent zu Wine fungiert. Der obige Code enthält einen Speicherverlust: Der von der Funktion getcwd zugewiesene Speicher wird nur in Sonderfallzweigen freigegeben. Um dies zu beheben, sollten die Autoren nach dem Aufruf von memcpy einen kostenlosen Aufruf hinzufügen.

Array-Index außerhalb der Grenzen


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 dieses bereits die maximale Anzahl von Elementen erreicht hat. Dieser Fehler kann behoben werden, indem einfach der Operator <= durch <ersetzt wird .

Der Analysator hat einen weiteren Fehler dieses Typs gefunden:

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

Tippfehler


Snippet 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.

Snippet 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; .... /* 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 erwarten sollten, dass die Variable minorVersion aus dem Stream gelesen wird, während der Wert in die Variable majorVersion eingelesen wird . Ich bin mit dem Projekt jedoch nicht gut genug vertraut, um es mit Sicherheit zu sagen.

Snippet 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

 /** 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 aus dem Kommentar hervorgeht, findet die Funktion trio_index das erste Zeichen in der Zeichenfolge, während die Funktion trio_index_last das letzte Vorkommen findet. Die Körper dieser beiden Funktionen sind jedoch genau gleich! Dies muss ein Tippfehler sein und die Funktion trio_index_last sollte wahrscheinlich strrchr anstelle von strchr zurückgeben. In diesem Fall würde sich das Programm wie erwartet verhalten.

Snippet 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; .... }

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

Snippet 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 dieselben: Der Programmierer muss vergessen haben, die Kopie zu ändern. Nach der Logik des Codes zu urteilen, behandelt der letzte Teil Vier-Byte-Werte, sodass wir davon ausgehen können, dass die letzte Bedingung prüfen sollte, ob der Wert <= 0x3FFFFFFF ist .

Ein weiterer Fehler dieses Typs:

  • 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

Eingabedaten überprüfen


Snippet 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); }

Die Überprüfung des Rückgabewerts der Funktion ist fehlerhaft. Die strcat- Funktion gibt einen Zeiger auf die Zielzeichenfolge zurück, d. H. Den ersten Parameter, der in diesem Fall target ist . Wenn es jedoch gleich NULL ist, ist es zu spät, dies zu überprüfen, da es in der strcat- Funktion bereits dereferenziert wurde.

Snippet 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 Snippet wird der Cache- Variablen die Adresse des statischen Arrays glyphCache-> glyphCache zugewiesen . Die Prüfung ob (Cache) kann daher entfernt 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; } .... }

Das fp- Handle für die von der Funktion CreateFile erstellte Datei wurde versehentlich geschlossen, indem die Funktion fclose aus der Standardbibliothek anstelle der Funktion CloseHandle aufgerufen wurde .

Identische 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); } .... }

Dieses Snippet mag korrekt sein, aber es ist verdächtig, dass beide Bedingungen identische Nachrichten enthalten - eine davon ist wahrscheinlich nicht erforderlich.

Nullzeiger freigeben


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; /* 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 freie Funktion kann auf einem Nullzeiger aufgerufen werden, und PVS-Studio weiß das. Wenn sich herausstellt, dass der Zeiger wie in diesem Snippet immer null ist, gibt der Analysator eine Warnung aus.

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

Einige andere Warnungen dieses Typs:

  • 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 verlassenen Variablen scheinen Rückstände 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)); .... }

Wenn Sie das Ausdrucksergebnis zu lang setzen, wird ein Überlauf nicht verhindert, da die Auswertung des Werts erfolgt, solange dieser noch vom Typ int ist .

Dereferenzierungszeiger 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; .... }

Der Kontextzeiger wird während seiner Initialisierung, dh vor der Prüfung, dereferenziert.

Andere Fehler dieses Typs:

  • 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; // <= .... 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 ergibt, da der betreffende Wert bereits zuvor zugewiesen wurde.

Falsche Zeichenfolgenbehandlung


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)) { .... } .... }

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

Überprüft in der falschen Reihenfolge


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) // <= 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 richtige 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; }

Fazit


Die Überprüfung ergab viele Fehler, und die oben diskutierten sind nur die interessantesten. Die Projektentwickler können gerne ein Formular für einen temporären Lizenzschlüssel auf der PVS-Studio- Website einreichen, um ihre eigene Prüfung durchzuführen. Der Analysator hat auch eine Reihe von Fehlalarmen erzeugt, die wir beheben werden, um seine Leistung zu verbessern. Beachten Sie, dass eine statische Analyse unabdingbar ist, wenn Sie nicht nur die Codequalität verbessern, sondern auch die Fehlersuche weniger zeitaufwändig machen möchten - und hier ist PVS-Studio hilfreich.

Source: https://habr.com/ru/post/de444246/

More articles:


All Articles