FreeRDP ist eine Open-Source-Implementierung des Remote Desktop Protocol (RDP), eines von Microsoft entwickelten Protokolls zur Fernsteuerung von Computern. Das Projekt unterstützt mehrere Plattformen, darunter Windows, Linux, macOS und sogar iOS mit AndroidDieses Projekt wurde als erstes in einer Artikelserie ausgewählt, die sich mit dem Testen von RDP-Clients mithilfe des statischen Analysators PVS-Studio befasst.
Ein wenig Geschichte
Projekt entstand, nachdem Microsoft die Spezifikationen für sein proprietäres RDP-Protokoll offengelegt hatte. Damals gab es einen rdesktop-Client, dessen Implementierung auf den Ergebnissen des Reverse Engineering basierte.
Mit der Implementierung des Protokolls wurde es aufgrund der damals bestehenden Projektarchitektur schwieriger, neue Funktionen hinzuzufügen. Änderungen darin führten zu einem Konflikt zwischen Entwicklern, der zur Schaffung eines Forks von rdesktop – FreeRDP – führte. Die weitere Verbreitung des Produkts war durch die GPLv2-Lizenz eingeschränkt, weshalb beschlossen wurde, es auf die Apache-Lizenz v2 umzulizenzieren. Da jedoch nicht alle einer Änderung der Lizenz ihres Codes zustimmten, beschlossen die Entwickler, das Projekt neu zu schreiben, was zu einer modernen Codebasis führte.
Mehr über die Geschichte des Projekts können Sie im offiziellen Blogbeitrag „Die Geschichte des FreeRDP-Projekts“ lesen.
Wird als Tool zur Identifizierung von Fehlern und potenziellen Schwachstellen im Code verwendet. Es handelt sich um einen statischen Codeanalysator für C, C++, C# und Java, der auf folgenden Plattformen verfügbar ist: Windows, Linux и macOS.
Der Artikel stellt nur die Fehler vor, die mir am interessantesten erschienen.
Utechka pamyati
Die Funktion wurde beendet, ohne den „cwd“-Zeiger freizugeben. 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 stammt aus dem winpr-Subsystem, das einen WINAPI-Wrapper für Nicht-Windows Systeme, d. h. es handelt sich um ein schlankes Analogon zu Wine. Hier ist ein Speicherleck zu sehen: Speicher, der von der Funktion belegt wird. getcwd, wird nur bei der Bearbeitung von Sonderfällen freigegeben. Um den Fehler zu beheben, müssen Sie einen Anruf hinzufügen kostenlos nach memcpy.
Array außerhalb der Grenzen
Ein 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. Hier reicht es aus, den Antrieb auszutauschen <= auf <, um die Grenzen des Arrays nicht zu überschreiten.
Ein weiterer Fehler dieser Art wurde gefunden:
- V557 Array-Überlauf ist möglich. Der Wert des Index „iBitmapFormat“ könnte 8 erreichen.orders.c 2623
Tippfehler
Fragment 1
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 häufigen Tippfehler: Die zweite Bedingung prüft dieselbe Variable wie die erste. Höchstwahrscheinlich ist der Fehler auf ein fehlgeschlagenes Kopieren des Codes zurückzuführen.
Fragment 2
Es wurden zwei Blöcke mit identischem Text 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 zum Code impliziert, dass der Thread kommen sollte NebenversionDas Lesen erfolgt jedoch in eine Variable mit dem Namen Hauptversion. Allerdings kenne ich das Protokoll nicht, daher ist das nur eine Vermutung.
Fragment 3
Es ist seltsam, 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);
}Dem Kommentar nach zu urteilen, der Funktion trio_index findet die erste Zeichenübereinstimmung in einer Zeichenfolge, wenn trio_index_last - letztes Ding. Aber die Körper dieser Funktionen sind identisch! Höchstwahrscheinlich handelt es sich hierbei um einen Tippfehler, und zwar in der Funktion trio_index_last verwenden müssen strchr statt strchr. Dann wird das Verhalten erwartet.
Fragment 4
Der „Daten“-Zeiger im Ausdruck ist gleich nullptr. Der resultierende Wert arithmetischer Operationen an 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 sieht so aus, als ob der Negationsoperator hier versehentlich übersehen wurde ! neben an frustrierten. Es ist seltsam, dass dies unbemerkt blieb.
Fragment 5
Die Verwendung des Musters „if (A) {…} else if (A) {…}“ wurde erkannt. Es besteht die Wahrscheinlichkeit, dass ein logischer Fehler vorliegt. Ü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: Offenbar hat jemand vergessen, sie nach dem Kopieren zu überprüfen. Aus dem Code geht hervor, dass der letzte Teil mit Vier-Byte-Werten arbeitet, wir können also davon ausgehen, dass die letzte Bedingung sein sollte Wert <= 0x3FFFFFFFF.
Ein weiterer Fehler dieser Art wurde gefunden:
- V517 Die Verwendung des Musters „if (A) {…} else if (A) {…}“ wurde erkannt. Es besteht die Wahrscheinlichkeit, dass ein logischer Fehler vorliegt. Überprüfen Sie die Zeilen: 169, 173. file.c 169
Validierung 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 falsch. Funktion strcat gibt einen Zeiger auf die endgültige Version der Zeichenfolge zurück, d. h. der erste übergebene Parameter. In diesem Fall ist es so Ziel. Allerdings, wenn es gleich ist NULL, dann ist es zu spät, es zu überprüfen, da in der Funktion strcat es wird dereferenziert.
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 die Variable Cache-Speicher die Adresse eines statischen Arrays wird zugewiesen glyphCache->glyphCache. Also prüfen if (cache) kann ausgelassen werden.
Fehler bei der Ressourcenverwaltung
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;
}
....
}Dateideskriptor fp, erstellt durch einen Funktionsaufruf Erstelle Datei versehentlich durch Funktion geschlossen fschließen aus der Standardbibliothek, nicht CloseHandle.
Gleiche Bedingungen
Die bedingten Ausdrü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 Beispiel ist möglicherweise kein Fehler. Beide Bedingungen enthalten jedoch dieselben Meldungen, von denen eine höchstwahrscheinlich entfernt werden kann.
Bereinigen von Nullzeigern
Der Nullzeiger wird an die Funktion „free“ übergeben. Untersuchen 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);
....
}In Funktion kostenlos Sie können einen Nullzeiger übergeben und der Analysator weiß davon. Wenn jedoch eine Situation erkannt wird, in der der Zeiger immer als Null übergeben wird, wie in diesem Snippet, wird eine Warnung ausgegeben.
Index mszGroupsA zunächst gleich NULL und wird nirgendwo anders initialisiert. Der einzige Codezweig, in dem der Zeiger initialisiert werden könnte, ist nicht erreichbar.
Es gab andere Nachrichten wie:
- V575 Der Nullzeiger wird an die Funktion „free“ übergeben. Untersuchen Sie das erste Argument. Lizenz.c 790
- V575 Der Nullzeiger wird an die Funktion „free“ übergeben. Untersuchen Sie das erste Argument. rdpsnd_alsa.c 575
Höchstwahrscheinlich entstehen solche vergessenen Variablen während des Refactoring-Prozesses und können einfach entfernt werden.
Möglicher Überlauf
Möglicher Überlauf. Erwägen Sie die Umwandlung von Operanden, nicht des Ergebnisses. 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));
....
}Das Ergebnis bringen lange ist kein Überlaufschutz, da die Berechnung selbst über den Typ erfolgt int.
Zeiger-Dereferenzierung bei der Initialisierung
Der „Kontext“-Zeiger wurde verwendet, bevor er anhand von 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 ist der Zeiger Kontext wird während der Initialisierung dereferenziert – bevor es überprüft wird.
Weitere Fehler dieser Art wurden gefunden:
- V595 Der „ntlm“-Zeiger wurde verwendet, bevor er anhand von nullptr überprüft wurde. Prüfzeilen: 236, 255. ntlm.c 236
- V595 Der „Kontext“-Zeiger wurde verwendet, bevor er anhand von nullptr überprüft wurde. Überprüfen Sie die Zeilen: 1003, 1007. rfx.c 1003
- V595 Der „rdpei“-Zeiger wurde verwendet, bevor er anhand von nullptr überprüft wurde. Prüfzeilen: 176, 180. rdpei_main.c 176
- V595 Der „gdi“-Zeiger wurde verwendet, bevor er anhand von nullptr überprüft wurde. Überprüfen Sie die Zeilen: 121, 123. xf_gfx.c 121
Sinnloser Zustand
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 bedeutungslos ist, da der entsprechende Wert früher zugewiesen wurde.
Falsches String-Parsing
Falsches Format. Erwägen Sie die Überprüfung des dritten tatsächlichen Arguments der Funktion „sscanf“. Es wird ein Zeiger auf den unsigned int-Typ 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 gibt für dieses Fragment sofort 2 Warnungen aus. Spezifizierer %u erwartet eine Variable vom Typ Unsigned int, aber variabel unten Typ hat int. Als nächstes sehen wir eine verdächtige Prüfung: Die Bedingung rechts ergibt keinen Sinn, da am Anfang ein Vergleich mit einer steht. Ich weiß nicht, was der Autor dieses Codes meinte, aber hier stimmt eindeutig etwas nicht.
Schecks außer Betrieb
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 überprüften Bedingungen sind immer falsch, da die Ausführung die zweite Bedingung nur dann erreicht, 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 ergab daher viele Probleme, von denen jedoch nur der interessanteste Teil im Artikel beschrieben wurde. Projektentwickler können das Projekt selbst überprüfen, indem sie auf der Website einen temporären Lizenzschlüssel anfordern . Es gab auch Fehlalarme, deren Arbeit zur Verbesserung des Analysegeräts beitragen wird. Allerdings ist die statische Analyse wichtig, wenn Sie nicht nur die Qualität Ihres Codes verbessern, sondern auch den Zeitaufwand für die Fehlersuche reduzieren möchten, und PVS-Studio kann Ihnen dabei helfen.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Sergey Larin.
Source: habr.com
