FreeRDP – eine Open-Source-Implementierung des Remote Desktop Protocol (RDP), eines Protokolls, das die remote Steuerung von Computern ermöglicht und von Microsoft entwickelt wurde. Das Projekt unterstützt eine Vielzahl von Plattformen, darunter Windows, Linux, macOS sowie iOS und Android. Dieses Projekt wurde als erstes im Rahmen einer Artikelreihe ausgewählt, die sich mit der Überprüfung von RDP-Clients mittels des statischen Analysewerkzeugs PVS-Studio beschäftigt.
Ein wenig Geschichte
Projekt entstand, nachdem Microsoft die Spezifikationen seines proprietären RDP-Protokolls offengelegt hatte. Zu diesem Zeitpunkt gab es bereits den Client rdesktop, dessen Implementierung auf Ergebnissen des Reverse Engineering basierte.
Im Zuge der Implementierung des Protokolls wurde es zunehmend schwieriger, neue Funktionen hinzuzufügen, was auf die damals vorhandene Projektarchitektur zurückzuführen war. Änderungen daran führten zu Konflikten unter den Entwicklern, was schließlich zur Entstehung eines Forks von rdesktop – FreeRDP – führte. Die weitere Verbreitung des Produkts war durch die GPLv2-Lizenz eingeschränkt, weshalb beschlossen wurde, es unter der Apache License v2 neu zu lizenzieren. Da jedoch nicht alle Entwickler mit einer Lizenzänderung einverstanden waren, entschieden sie sich, das Projekt neu zu schreiben, was uns die moderne Codebasis beschert.
Mehr über die Geschichte des Projekts erfahren Sie in dem offiziellen Blogbeitrag: „Die Geschichte des FreeRDP-Projekts“.
Als Werkzeug zur Identifizierung von Fehlern und potenziellen Sicherheitsanfälligkeiten im Code wurde verwendet eingesetzt. Dies ist ein statischer Code-Analyzer für die Programmiersprachen C, C++, C# und Java, der auf den Plattformen Windows, Linux und macOS verfügbar ist.
In dem Artikel werden nur die Fehler vorgestellt, die mir am interessantesten erschienen.
Speicherverlust
Die Funktion wurde verlassen, ohne den ‚cwd‘-Zeiger freizugeben. Ein Speicherverlust ist möglich. environment.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;
....
}Dieser Abschnitt wurde aus dem winpr-Subsystem entnommen, das eine WINAPI-Wrapper für Nicht-Windows-Systeme implementiert, also eine leichte Alternative zu Wine. Hier kann man den Speicherverlust erkennen: Der Speicher, der von der Funktion getcwd, wird nur bei der Behandlung spezieller Fälle freigegeben. Um den Fehler zu beheben, sollte ein Aufruf hinzugefügt werden free angezeigt werden können. nach memcpy.
Array-Überlauf
Ein Überlauf des Arrays ist möglich. Der Wert des Indexes ‚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 ein neues Element zur Liste hinzugefügt, selbst wenn die Anzahl der Elemente das Maximum erreicht hat. Hier reicht es, den Operator <= findet man <, zu ersetzen, um einen Überlauf des Arrays zu vermeiden.
Es wurde ein anderer Fehler dieses Typs gefunden:
- V557 Arrayüberlauf ist möglich. Der Wert des Index ‘iBitmapFormat’ könnte 8 erreichen. orders.c 2623
Rechtschreibfehler
Fragment 1
Der 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;
....
}Hier sehen wir einen typischen Schreibfehler: Im zweiten Zustand wird dieselbe Variable überprüft wie im ersten. Wahrscheinlich ist der Fehler durch ein misslungenes Kopieren des Codes entstanden.
Fragment 2
Zwei identische Textblöcke wurden 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);
....
}Noch ein Schreibfehler: Der Kommentar zum Code impliziert, dass aus dem Stream kommen sollte minorVersion, allerdings erfolgt das Lesen in eine Variable mit dem Namen majorVersion. Dennoch bin ich mit dem Protokoll nicht vertraut, sodass dies nur eine Vermutung ist.
Fragment 3
Es ist seltsam, dass der Körper der Funktion ‘trio_index_last’ vollständig mit dem Körper der Funktion ‘trio_index’ identisch ist. 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);
}Laut dem Kommentar ist die Funktion trio_index findet das erste Vorkommen eines Zeichens in der Zeichenkette, wenn trio_index_last — das letzte. Aber die Körper dieser Funktionen sind identisch! Höchstwahrscheinlich handelt es sich um einen Tippfehler, und in der Funktion trio_index_last verwendet werden. strrchr statt strchr. Dann wird das Verhalten wie erwartet sein.
Fragment 4
Der ‘data’-Pointer im Ausdruck ist gleich nullptr. Der resultierende Wert von arithmetischen Operationen auf diesem Pointer 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;
....
}Es scheint, dass hier versehentlich der Negationsoperator ausgelassen wurde ! neben data. Seltsam, dass dies unbeachtet blieb.
Fragment 5
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 > 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 > 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 identisch: Offensichtlich hat jemand vergessen, diese nach dem Kopieren zu überprüfen. Der Code zeigt, dass der letzte Teil mit vierbytes Werten arbeitet, daher kann man annehmen, dass die letzte Bedingung value <= 0x3FFFFFFF.
Es wurde ein anderer Fehler dieses Typs gefunden:
- V517 Das Muster 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. Überprüfen Sie die Zeilen: 169, 173. file.c 169
Überprüfung der Eingabedaten
Fragment 1
Der 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 Ergebnisses der Funktion in diesem Beispiel ist inkorrekt. Die Funktion strcat gibt einen Zeiger auf die endgültige Zeichenkette zurück, d. h. den ersten übergebenen Parameter. In diesem Fall ist das target. Wenn er jedoch gleich ist NULL, ist es zu spät, ihn zu überprüfen, da in der Funktion strcat seine Dereferenzierung stattfinden wird.
Fragment 2
Der 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 Fall wird der Variable cache die Adresse eines statischen Arrays zugewiesen, glyphCache->glyphCache. Somit kann die Überprüfung if (cache) weggelassen werden.
Fehler bei der Ressourcenverwaltung
Die Ressource wurde mit der Funktion ‘CreateFileA’ erlangt, 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;
}
....
}Dateihandler fp, erstellt durch den Aufruf der Funktion CreateFile, wurde fälschlicherweise durch die Funktion fclose aus der Standardbibliothek geschlossen, und nicht durch CloseHandle..
Identische Bedingungen
Die bedingten Ausdrücke der ‘if’-Anweisungen, die nebeneinander stehen, 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);
}
....
}Möglicherweise ist dieses Beispiel kein Fehler. Allerdings enthalten beide Bedingungen identische Meldungen, von denen eine wahrscheinlich entfernt werden kann.
Nullzeigerbereinigung
Der Nullzeiger wird in die Funktion 'free' ü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 wird von pcsc-lite nicht unterstützt */
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);
....
}In die Funktion free angezeigt werden können. Es kann ein Nullzeiger übergeben werden, und der Parser ist sich dessen bewusst. Wenn jedoch festgestellt wird, dass der Zeiger immer als Null übergeben wird, wie in diesem Abschnitt, wird eine Warnung ausgegeben.
Zeiger mszGroupsA anfänglich gleich NULL und wird nirgendwo anders initialisiert. Der einzige Codepfad, in dem der Zeiger initialisiert werden könnte, ist unerreichbar.
Es gab auch andere solche Meldungen:
- V575 Der Nullzeiger wird in die Funktion ‚free‘ übergeben. Überprüfen Sie das erste Argument. license.c 790
- V575 Der Nullzeiger wird in die Funktion ‚free‘ übergeben. Überprüfen Sie das erste Argument. rdpsnd_alsa.c 575
Wahrscheinlich entstehen solche vergessenen Variablen während des Refactorings und können einfach entfernt werden.
Mögliche Überlauf
Möglicher Überlauf. Erwägen Sie, Operanden und nicht das Ergebnis zu casten. 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));
....
}Typumwandlung des Ergebnisses zu long stellt keinen Schutz gegen Überläufe dar, da die Berechnung selbst mit dem Typ erfolgt int.
Pointer-Dereferenzierung in der Initialisierung
Der ‚context‘ Pointer wurde verwendet, bevor er auf nullptr geprüft 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 Pointer context in der Initialisierung dereferenziert – bevor er geprüft wird.
Es wurden weitere Fehler dieses Typs gefunden:
- V595 Der ‚ntlm‘ Pointer wurde verwendet, bevor er auf nullptr geprüft wurde. Überprüfen Sie die Zeilen: 236, 255. ntlm.c 236
- V595 Der ‚context‘ Pointer wurde verwendet, bevor er auf nullptr geprüft wurde. Überprüfen Sie die Zeilen: 1003, 1007. rfx.c 1003
- V595 Der ‚rdpei‘ Pointer wurde verwendet, bevor er auf nullptr geprüft wurde. Überprüfen Sie die Zeilen: 176, 180. rdpei_main.c 176
- V595 Der ‚gdi‘ Pointer wurde verwendet, bevor er auf nullptr geprüft wurde. Überprüfen Sie die Zeilen: 121, 123. xf_gfx.c 121
Sinnloser Ausdruck
Der 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; // state >= CONNECTION_STATE_ACTIVE) // Activate, client->activated, client);
if (!client->activated)
return -1;
}
....
}
....
}Es ist leicht zu erkennen, dass die erste Bedingung aufgrund der vorherigen Zuweisung eines entsprechenden Wertes keinen Sinn ergibt.
Ungültige Zeichenfolgenanalyse
Falsches Format. Überprüfen Sie das dritte tatsächliche Argument der Funktion ‘sscanf’. Ein Zeiger auf den Typ unsigned int wird erwartet. proxy.c 220
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 Analyzer für dieses Fragment gibt sofort 2 Warnungen aus. Der Spezifizierer %u erwartet eine Variable des Typs unsigned int, aber die Variable sub hat den Typ int. Außerdem sehen wir eine verdächtige Prüfung: Die Bedingung rechts macht keinen Sinn, da zu Beginn mit eins verglichen wird. Ich weiß nicht, was der Autor dieses Codes im Sinn hatte, aber hier stimmt eindeutig etwas nicht.
Ungeordnete Prüfungen
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 werden immer falsch sein, da die Ausführung zum zweiten Zustand nur kommt, wenn status == SEC_E_OK. Korrekt könnte der Code so 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 hat zahlreiche Probleme aufgezeigt, jedoch wurde in dem Artikel nur die interessanteste Teil beschrieben. Die Entwickler des Projekts können das Projekt selbst überprüfen, indem sie einen temporären Lizenzschlüssel auf der Webseite anfordern. . Es gab auch Fehlalarme, deren Bearbeitung zur Verbesserung des Analysators beitragen wird. Dennoch ist die statische Analyse wichtig, wenn Sie nicht nur die Codequalität erhöhen, sondern auch die Zeit zur Fehlersuche verkürzen möchten, wobei PVS-Studio Ihnen dabei helfen kann.
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Larin.
Quelle: habr.com
