FreeRDP – un'implementazione open source del Remote Desktop Protocol (RDP), un protocollo per il controllo remoto di un computer, sviluppato da Microsoft. Il progetto supporta diverse piattaforme, tra cui Windows, Linux, macOS e anche iOS e Android. Questo progetto è stato scelto come primo argomento nel ciclo di articoli dedicati alla verifica dei client RDP utilizzando l'analizzatore statico PVS-Studio.
Un po' di storia
Progetto è nato dopo che Microsoft ha reso pubbliche le specifiche del proprio protocollo proprietario RDP. All'epoca esisteva il client rdesktop, la cui implementazione si basa sui risultati del reverse engineering.
Nella fase di implementazione del protocollo, diventava sempre più difficile aggiungere nuove funzionalità a causa dell'architettura del progetto esistente. Le modifiche a questa architettura causarono conflitti tra gli sviluppatori, portando alla creazione del fork rdesktop – FreeRDP. La successiva diffusione del prodotto fu limitata dalla licenza GPLv2, dopo di che si decise di rilasciarlo sotto la Apache License v2. Tuttavia, non tutti erano d'accordo nel cambiare la licenza del proprio codice, quindi gli sviluppatori decisero di riscrivere il progetto, portandoci all'attuale stato della base di codice.
Puoi leggere di più sulla storia del progetto nel post ufficiale del blog: "The history of the FreeRDP project".
Come strumento per identificare errori e potenziali vulnerabilità nel codice è stato utilizzato . Questo è un analizzatore statico di codice per i linguaggi C, C++, C# e Java, disponibile sulle piattaforme Windows, Linux e macOS.
Nell'articolo sono elencati solo quegli 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'analoga leggera di Wine. Qui si può notare una fuga: la memoria allocata dalla funzione getcwd, viene liberata solo nel trattamento di casi speciali. Per risolvere l'errore è necessario aggiungere una chiamata a free la data rientri nel PITR. memcpy.
Accesso oltre i limiti dell'array
È possibile un accesso oltre i limiti. 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 alla lista, anche se il numero di elementi ha raggiunto il massimo. Qui è sufficiente sostituire l'operatore <= in <, per non superare i limiti dell'array.
È stato trovato un altro errore di questo tipo:
- V557 È possibile un superamento dell'array. Il valore dell'indice ‘iBitmapFormat’ potrebbe arrivare a 8. orders.c 2623
Refusi
Frammento 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 errore di battitura comune: nella seconda condizione viene controllata la stessa variabile della prima. Probabilmente l'errore è sorto a causa di un cattivo copia-incolla del codice.
Frammento 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 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* MinorVersion (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Un altro errore di battitura: il commento nel codice implica che dal flusso debba arrivare minorVersion, tuttavia la lettura avviene nella variabile di nome majorVersion. Tuttavia, non conosco il protocollo, quindi è solo un'ipotesi.
Frammento 3
È strano che il corpo della funzione ‘trio_index_last’ sia completamente equivalente al corpo 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);
}Stando al commento, la funzione trio_index trova la prima occorrenza di un carattere nella stringa, mentre trio_index_last trova l'ultima. Ma i corpi di queste funzioni sono identici! Probabilmente si tratta di un errore di battitura, e nella funzione trio_index_last devi usare strrchr anziché strchr. Allora il comportamento sarà previsto.
Frammento 4
Il puntatore ‘data’ nell'espressione è uguale a nullptr. Il valore risultante delle operazioni aritmetiche su questo puntatore è privo di significato e non dovrebbe essere utilizzato. 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 sia stato accidentalmente omesso l'operatore di negazione ! vicino a data. È strano che questo sia passato inosservato.
Frammento 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. Dal codice è evidente che l'ultima parte lavora con valori a quattro byte, quindi si può presupporre 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'è una probabilità di presenza di errore logico. Controlla le righe: 169, 173. file.c 169
Controllo dei dati in ingresso
Frammento 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 della funzione in questo esempio è errato. La funzione strcat restituisce un puntatore alla versione finale della stringa, ovvero il primo parametro passato. In questo caso è target. Tuttavia, se è uguale a NULL, controllarlo è tardivo, poiché nella funzione strcat si verificherà il suo de-referenzamento.
Frammento 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. Quindi, il controllo if (cache) può essere omesso.
Errore di gestione delle risorse
La risorsa è stata acquisita utilizzando la funzione 'CreateFileA' ma è stata rilasciata tramite una funzione incompatibile 'fclose'. 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;
}
....
}Il descrittore del file fp, creato con la chiamata della funzione CreateFile, è stato erroneamente chiuso dalla funzione fclose della libreria standard, invece di CloseHandle.
Condizioni identiche
Le espressioni condizionali delle istruzioni 'if' situate affiancate 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: "
"0xX non implementato", 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: "
"0xX non implementato", array_type);
}
....
}Questo esempio potrebbe non essere un errore. Tuttavia, entrambe le condizioni contengono messaggi identici, uno dei quali può probabilmente essere rimosso.
Pulizia dei puntatori nulli
Il puntatore nullo è passato alla funzione 'free'. Controlla 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 ne è a conoscenza. Ma se si verifica una situazione in cui il puntatore viene sempre passato come nullo, come in questo frammento, verrà emesso un avviso.
Puntatore mszGroupsA inizialmente è uguale a NULL e non viene più inizializzato. L'unico ramo di codice in cui il puntatore potrebbe essere stato inizializzato è irraggiungibile.
Ci sono stati altri messaggi di questo tipo:
- V575 Il puntatore nullo è passato alla funzione 'free'. Controlla il primo argomento. license.c 790
- V575 Il puntatore nullo è passato alla funzione 'free'. Controlla il primo argomento. rdpsnd_alsa.c 575
Probabilmente variabili simili dimenticate emergono durante il refactoring e possono essere semplicemente rimosse.
Possibile overflow
Possibile overflow. Considera di eseguire 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));
....
}Il cast del risultato a long non è una protezione contro l'overflow, poiché stesso il calcolo avviene utilizzando il tipo int.
Dereferenziare un 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 durante l'inizializzazione, prima di essere verificato.
Sono stati trovati altri errori di questo tipo:
- V595 Il puntatore ‘ntlm’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 236, 255. ntlm.c 236
- V595 Il puntatore ‘context’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 1003, 1007. rfx.c 1003
- V595 Il puntatore ‘rdpei’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 176, 180. rdpei_main.c 176
- V595 Il puntatore ‘gdi’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 121, 123. xf_gfx.c 121
Condizione priva di significato
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 significato a causa dell'assegnazione del valore corrispondente precedente.
Analisi della stringa non valida
Formato errato. Considera di controllare il terzo argomento effettivo della funzione ‘sscanf’. Si prevede 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 restituisce subito 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 significato poiché all'inizio viene confrontata 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 se status == SEC_E_OK. Il codice corretto potrebbe apparire così:
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;
}Conclusione
Pertanto, la verifica del progetto ha evidenziato diversi problemi, ma solo la parte più interessante è stata descritta nell'articolo. Gli sviluppatori del progetto possono verificare autonomamente il progetto richiedendo una chiave di licenza temporanea sul sito. . Ci sono stati anche falsi positivi, lavorare sui quali 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 di ricerca degli errori, e PVS-Studio può aiutare in questo.
Se vuoi condividere quest'articolo con un pubblico di lingua inglese, ti prego di utilizzare il link alla traduzione: Sergey Larin.
Fonte: habr.com
