FreeRDP – eine Open-Source-Implementierung des Remote Desktop Protocol (RDP), eines Protokolls, das die Fernsteuerung von Computern ermöglicht und von Microsoft entwickelt wurde. Das Projekt unterstützt zahlreiche Plattformen, darunter 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 wenig Geschichte
Projekt entstand, nachdem Microsoft die Spezifikationen seines proprietären RDP-Protokolls veröffentlicht hatte. Zu dem Zeitpunkt existierte der Client rdesktop, dessen Implementierung auf Ergebnissen der Umkehrtechnik basierte.
Im Zuge der Implementierung des Protokolls wurde es immer schwieriger, neue Funktionen aufgrund der damaligen Projektarchitektur hinzuzufügen. Änderungen an dieser führten zu Konflikten zwischen den Entwicklern, was zur Schaffung eines Forks von rdesktop – FreeRDP – führte. Die weitere Verbreitung des Produkts war durch die GPLv2-Lizenz eingeschränkt, weshalb die Entscheidung getroffen wurde, auf die Apache License v2 zu wechseln. Da jedoch nicht alle damit einverstanden waren, die Lizenz ihres Codes zu ändern, beschlossen die Entwickler, das Projekt neu zu schreiben, was zur modernen Form der Codebasis führte.
Eine ausführlichere Geschichte des Projekts kann im offiziellen Blogbeitrag gelesen werden: „Die Geschichte des FreeRDP-Projekts“.
Als Tool zur Identifizierung von Fehlern und potenziellen Sicherheitsanfälligkeiten im Code wurde verwendet . Dies ist ein statisches Code-Analysewerkzeug für die Programmiersprachen C, C++, C# und Java, das auf den Plattformen Windows, Linux und macOS verfügbar ist.
Der Artikel stellt nur die Fehler vor, die mir am interessantesten erschienen.
Speicherleck
Die Funktion wurde verlassen, ohne den 'cwd'-Zeiger freizugeben. Ein Speicherleck 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 der Winpr-Subsystem entnommen, das eine Wrapper-API für Nicht-Windows-Systeme implementiert, d.h. es ist ein leichtgewichtiger Äquivalent zu Wine. Hier kann man ein Leck erkennen: Der von der Funktion getcwd, zugewiesene Speicher wird nur in speziellen Fällen freigegeben. Um den Fehler zu beheben, muss ein Aufruf von free nach memcpy.
Arrayüberlauf
Ein Arrayüberlauf 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, obwohl die Anzahl der Elemente das Maximum erreicht hat. Hier reicht es aus, den Operator <= auf <, um nicht über die Grenzen des Arrays hinauszugehen.
Es wurde ein weiterer Fehler dieses Typs gefunden:
- V557 Array-Überlauf ist möglich. Der Wert des Index ‘iBitmapFormat’ könnte 8 erreichen. orders.c 2623
Tippfehler
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 Tippfehler: Im zweiten Bedingung wird dieselbe Variable geprüft wie im ersten. Wahrscheinlich ist der Fehler durch unglückliches 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 Tippfehler: Der Kommentar im Code deutet darauf hin, dass aus dem Stream kommen sollte minorVersion, aber das Lesen erfolgt in die 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 äquivalent zum Körper der Funktion ‘trio_index’ 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);
}Nach dem Kommentar sucht die Funktion trio_index das erste Vorkommen eines Zeichens in der Zeichenkette, während trio_index_last das letzte gefunden wird. Aber die Körper dieser Funktionen sind identisch! Wahrscheinlich ist dies ein Tippfehler, und in der Funktion trio_index_last sollte benutzt werden strrchr anstatt strchr. Dann wäre das Verhalten zu erwarten.
Fragment 4
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;
....
}Es scheint, dass hier versehentlich der Negationsoperator ! in der Nähe von data. Es ist seltsam, dass das unbemerkt blieb.
Fragment 5
Das Muster ‘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, sie nach dem Kopieren zu überprüfen. Im Code ist deutlich zu erkennen, dass der letzte Teil mit vier Byte langen Werten arbeitet, daher kann man annehmen, dass die letzte Bedingung sein sollte value <= 0x3FFFFFFF.
Es wurde ein weiterer 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
Prü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 Funktionsergebnisses in diesem Beispiel ist fehlerhaft. Die Funktion strcat gibt einen Zeiger auf die endgültige Form des Strings zurück, d.h. den ersten übergebenen Parameter. In diesem Fall ist das target. Wenn er jedoch gleich NULL, dann ist es zu spät, ihn zu überprüfen, da in der Funktion strcat eine Dereferenzierung stattfindet.
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 Variablen cache die Adresse des statischen Arrays glyphCache->glyphCachezugewiesen. Somit kann die Überprüfung if (cache) weggelassen werden.
Fehler bei der Ressourcenverwaltung
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;
}
....
}Der Dateadeskriptor fp, erstellt durch den Aufruf der Funktion CreateFile, wurde fälschlicherweise durch die Funktion fclose aus der Standardbibliothek geschlossen, anstatt durch CloseHandle.
Identische Bedingungen
Die bedingten Ausdrücke der 'if'-Anweisungen, die nebeneinander liegen, 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, "Warnung: NdrComplexStructBufferSize array_type: "
"0xX nicht implementiert", 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, "Warnung: NdrComplexStructBufferSize array_type: "
"0xX nicht implementiert", array_type);
}
....
}Möglicherweise ist dieses Beispiel kein Fehler. Allerdings enthalten beide Bedingungen identische Meldungen, von denen eine wahrscheinlich entfernt werden kann.
Bereinigung von Nullzeigern
Der Nullzeiger wird an 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 Es ist möglich, einen Nullzeiger zu übergeben, und der Analyzer ist darüber informiert. Wenn jedoch eine Situation erkannt wird, in der der Zeiger immer als Null übergeben wird, wie in diesem Fragment, wird eine Warnung ausgegeben.
Zeiger mszGroupsA anfangs gleich NULL und nirgendwo anders initialisiert. Der einzige Codepfad, in dem der Zeiger initialisiert werden könnte, ist unerreichbar.
Es gab auch andere Meldungen dieser Art:
- V575 Der Nullzeiger wird an die Funktion 'free' übergeben. Überprüfen Sie das erste Argument. license.c 790
- V575 Der Nullzeiger wird an die Funktion 'free' übergeben. Überprüfen Sie das erste Argument. rdpsnd_alsa.c 575
Ähnliche vergessene Variablen treten wahrscheinlich während der Refaktorisierung auf und können einfach entfernt werden.
Mögliche Überlauf
Möglicher Überlauf. Ziehen Sie in Betracht, 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));
....
}Cast des Ergebnisses zu long schützt nicht vor Überlauf, da die Berechnung selbst mit dem Typ erfolgt int.
Dereferenzierung des Zeigers in der Initialisierung
Der 'context'-Zeiger wurde verwendet, bevor er mit nullptr überprü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 Zeiger context entdereferenziert in der Initialisierung — bevor er überprüft wird.
Es wurden auch andere Fehler dieses Typs gefunden:
- V595 Der Zeiger 'ntlm' wurde verwendet, bevor er gegen nullptr überprüft wurde. Überprüfen Sie die Zeilen: 236, 255. ntlm.c 236
- V595 Der Zeiger 'context' wurde verwendet, bevor er gegen nullptr überprüft wurde. Überprüfen Sie die Zeilen: 1003, 1007. rfx.c 1003
- V595 Der Zeiger 'rdpei' wurde verwendet, bevor er gegen nullptr überprüft wurde. Überprüfen Sie die Zeilen: 176, 180. rdpei_main.c 176
- V595 Der Zeiger 'gdi' wurde verwendet, bevor er gegen nullptr überprüft wurde. Überprüfen Sie die Zeilen: 121, 123. xf_gfx.c 121
Sinnloses Bedingung
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 vorher zugewiesenen Werte keinen Sinn macht.
Ungültige Zeichenfolgenanalyse
Ungültiges 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 Analysator für diesen Fragment gibt sofort 2 Warnungen aus. Der Spezifizierer %u erwartet eine Variable des Typs unsigned int, aber die Variable sub hat den Typ int. Weiterhin sehen wir eine verdächtige Überprüfung: Die Bedingung rechts macht keinen Sinn, da zuerst mit eins verglichen wird. Ich weiß nicht, was der Autor dieses Codes gemeint hat, aber hier stimmt offensichtlich etwas nicht.
Ungeordnete Überprü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 nur zur zweiten Bedingung gelangt, wenn status == SEC_E_OK. Der richtige Code könnte 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 viele Probleme aufgezeigt, aber nur der interessanteste Teil wurde in dem Artikel 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, an denen gearbeitet wird, um den Analysator zu verbessern. Dennoch ist die statische Analyse wichtig, wenn Sie nicht nur die Codequalität erhöhen, sondern auch die Zeit für die Fehlersuche verkürzen möchten, und PVS-Studio kann dabei helfen.
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, nutzen Sie bitte den Link zur Übersetzung: Sergey Larin.
Quelle: habr.com
