FreeRDP – è un'implementazione open source del Remote Desktop Protocol (RDP), il protocollo creato da Microsoft per il controllo remoto dei computer. Il progetto supporta molte piattaforme, tra cui Windows, Linux, macOS, e persino iOS e Android. Questo progetto è stato scelto come primo nell'ambito di una serie di articoli dedicati alla verifica dei client RDP attraverso un analizzatore statico come PVS-Studio.
Un po' di storia
Progetto è nato dopo che Microsoft ha reso pubbliche le specifiche del suo protocollo proprietario RDP. All'epoca esisteva il client rdesktop, la cui implementazione si basa su risultati di reverse engineering.
Durante l'implementazione del protocollo, è diventato sempre più difficile aggiungere nuove funzionalità a causa dell'architettura del progetto esistente. Le modifiche hanno creato conflitti tra gli sviluppatori, portando alla creazione di un fork di rdesktop – FreeRDP. La diffusione del prodotto era limitata dalla licenza GPLv2, quindi si è deciso di rilasciarlo sotto la Apache License v2. Tuttavia, non tutti erano d'accordo nel cambiare la licenza del proprio codice, quindi gli sviluppatori hanno deciso di riscrivere il progetto, portando all'attuale versione della base di codice.
Maggiori dettagli sulla storia del progetto sono disponibili nell'articolo del blog ufficiale: «La storia del progetto FreeRDP».
Per identificare errori e potenziali vulnerabilità nel codice, è stato utilizzato . Si tratta di un analizzatore statico del codice per i linguaggi C, C++, C# e Java, disponibile su Windows, Linux e macOS.
L'articolo presenta solo gli errori che mi sono sembrati più interessanti.
Fuga di memoria
La funzione è stata interrotta senza rilasciare il puntatore ‘cwd’. È possibile una fuga di memoria. 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;
....
}Questo frammento è stato preso dal sottosistema winpr, che implementa un wrapper WINAPI per sistemi non Windows, ovvero è un'alternativa leggera a Wine. Qui si può notare una fuga di memoria: la memoria allocata dalla funzione getcwd, viene liberata solo nella gestione di casi speciali. Per risolvere l'errore, è necessario aggiungere una chiamata a free dopo memcpy.
Uscita dai limiti dell'array
È possibile un sovraccarico dell'array. Il valore dell'indice 'event->EventHandlerCount' potrebbe raggiungere 32. 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 questo esempio, un nuovo elemento viene aggiunto all'elenco anche se il numero di elementi ha raggiunto il massimo. È sufficiente sostituire l'operatore <= con <, per non sforare i limiti dell'array.
È stato trovato un altro errore di questo tipo:
- V557 È possibile un superamento dell'array. Il valore dell'indice ‘iBitmapFormat’ potrebbe raggiungere 8. orders.c 2623
Refusi
Fragmento 1
L'espressione ‘!pipe->In’ è sempre falsa. 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;
....
}Qui vediamo un comune refuso: nella seconda condizione viene controllata la stessa variabile della prima. Probabilmente, l'errore è stato causato da una copia sbagliata del codice.
Fragmento 2
Sono stati trovati due blocchi di testo identici. Il secondo blocco inizia dalla riga 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 byte) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* MinorVersion (2 byte) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Un altro refuso: il commento nel codice implica che dal flusso debba arrivare minorVersion, tuttavia, la lettura avviene in una variabile chiamata majorVersion. Tuttavia, non conosco il protocollo, quindi questa è solo un'ipotesi.
Fragmento 3
È strano che il corpo della funzione ‘trio_index_last’ sia del tutto equivalente a quello della funzione ‘trio_index’. 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);
}Da quanto emerge dal commento, la funzione trio_index trova la prima corrispondenza di un carattere in una stringa, mentre trio_index_last — trova l'ultima. Ma i corpi di queste funzioni sono identici! Probabilmente è un errore di battitura, e nella funzione trio_index_last bisogna usare strrchr invece di strchr. Allora il comportamento sarà previsto.
Fragmento 4
Il puntatore ‘data’ nell'espressione è uguale a nullptr. Il valore risultante delle operazioni aritmetiche su questo puntatore è privo di senso e non dovrebbe essere usato. 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;
....
}Sembra che qui sia accidentalmente stato omesso l'operatore di negazione ! accanto a data. È strano che ciò sia rimasto inosservato.
Fragmento 5
È stato rilevato l'uso del modello ‘if (A) {…} else if (A) {…}’. C'è una probabilità di presenza di un errore logico. Controlla le righe: 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);
}
....
}Le ultime due condizioni sono identiche: evidentemente, qualcuno ha dimenticato di controllarle dopo la copia. Dall'analisi del codice, sembra che l'ultima parte lavori con valori a quattro byte, quindi si può ipotizzare che l'ultima condizione dovrebbe essere value <= 0x3FFFFFFF.
È stato trovato un altro errore di questo tipo:
- V517 È stato rilevato l'uso del modello 'if (A) {...} else if (A) {...}'. C'è la probabilità della presenza di un errore logico. Controlla le righe: 169, 173. file.c 169
Controllo dei dati di input
Fragmento 1
L'espressione 'strcat(target, source) != NULL' è sempre vera. 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);
}Il controllo del risultato di esecuzione della funzione in questo esempio è errato. La funzione strcat restituisce un puntatore alla variante finale della stringa, cioè al primo parametro passato. In questo caso è target. Tuttavia, se è uguale a NULL, è troppo tardi per controllarlo, poiché nella funzione strcat ci sarà la sua dereferenziazione.
Fragmento 2
L'espressione 'cache' è sempre vera. 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 questo caso, alla variabile cache viene assegnato l'indirizzo di un array statico glyphCache->glyphCache. Così, il controllo if (cache) può essere omesso.
Errore di gestione delle risorse
La risorsa è stata acquisita utilizzando la funzione 'CreateFileA' ma è stata rilasciata usando la funzione 'fclose' incompatibile. 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;
}
....
}Descrittore del file fp, creato dalla chiamata della funzione CreateFile, chiuso erroneamente dalla funzione fclose della libreria standard, invece di CloseHandle.
Condizioni identiche
Le espressioni condizionali delle istruzioni 'if' situate una accanto all'altra sono identiche. Controlla le righe: 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);
}
....
}Questo esempio potrebbe non essere un errore. Tuttavia, entrambe le condizioni contengono lo stesso messaggio, uno dei quali può probabilmente essere rimosso.
Pulizia dei puntatori nulli
Il puntatore nullo è passato nella funzione 'free'. Controllare il primo argomento. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups non è supportato da 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);
....
}Nella funzione free È possibile passare un puntatore nullo e l'analizzatore lo sa. Ma se si verifica una situazione in cui il puntatore è sempre passato come nullo, come in questo frammento, verrà emesso un avviso.
Puntatore mszGroupsA inizialmente uguale a NULL e non viene più inizializzato da nessun'altra parte. L'unico ramo di codice in cui il puntatore potrebbe essere inizializzato è inaccessibile.
Ci sono stati anche altri messaggi di questo tipo:
- V575 Il puntatore nullo viene passato alla funzione 'free'. Controllare il primo argomento. license.c 790
- V575 Il puntatore nullo viene passato alla funzione 'free'. Controllare il primo argomento. rdpsnd_alsa.c 575
Probabilmente, queste variabili dimenticate emergono durante il refactoring e possono essere semplicemente rimosse.
Possibile overflow
Possibile overflow. Considerare di effettuare il cast degli operandi, non del risultato. 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 del risultato in long non è una protezione contro il buffer overflow, poiché il calcolo stesso avviene utilizzando il tipo -int.
Dereferenziamento del puntatore durante l'inizializzazione
Il puntatore ‘context’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 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;
....
}Qui il puntatore context viene dereferenziato nell'inizializzazione — prima che venga eseguita la sua verifica.
Sono stati trovati altri errori di questo tipo:
- V595 Il puntatore ‘ntlm’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 236, 255. ntlm.c 236
- V595 Il puntatore ‘context’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 1003, 1007. rfx.c 1003
- V595 Il puntatore ‘rdpei’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 176, 180. rdpei_main.c 176
- V595 Il puntatore ‘gdi’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 121, 123. xf_gfx.c 121
Condizione priva di senso
L'espressione ‘rdp->state >= CONNECTION_STATE_ACTIVE’ è sempre vera. 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;
}
....
}
....
}È facile notare che la prima condizione non ha senso a causa dell'assegnazione del valore corrispondente precedente.
Analisi della stringa non valida
Formato errato. Controlla il terzo argomento effettivo della funzione ‘sscanf’. Ci si aspetta un puntatore di tipo unsigned int. proxy.c 220
Una parte dell'espressione condizionale è sempre vera: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}L'analizzatore per questo frammento genera immediatamente 2 avvisi. Il specificatore %u si aspetta una variabile di tipo unsigned int, ma la variabile sub ha tipo -int. Inoltre, vediamo un controllo sospetto: la condizione a destra non ha senso, poiché inizialmente si compara con uno. Non so cosa intendesse l'autore di questo codice, ma qui chiaramente c'è qualcosa che non va.
Controlli non ordinati
L'espressione ‘status == 0x00090314’ è sempre falsa. 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;
....
}Le condizioni contrassegnate saranno sempre false, poiché l'esecuzione raggiungerà la seconda condizione solo quando status == SEC_E_OK. Il codice corretto potrebbe apparire così:
se (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;
}Conclusione
La revisione del progetto ha rivelato numerosi problemi, ma solo le parti più interessanti sono state descritte nell'articolo. Gli sviluppatori del progetto possono controllare il proprio lavoro richiedendo una chiave di licenza temporanea sul sito. . Ci sono stati anche falsi positivi, il cui lavoro aiuterà a migliorare l'analizzatore. Tuttavia, l'analisi statica è importante se si desidera non solo migliorare la qualità del codice, ma anche ridurre il tempo necessario per trovare errori, e PVS-Studio può aiutare in questo.
Se desideri condividere questo articolo con un pubblico anglofono, ti prego di utilizzare il link alla traduzione: Sergey Larin.
Fonte: habr.com
