FreeRDP és una implementació de codi obert del Remote Desktop Protocol (RDP), un protocol desenvolupat per Microsoft per al control remot d'ordinadors. El projecte admet múltiples plataformes, incloent-hi Windows, Linux, macOS i fins i tot iOS amb AndroidAquest projecte va ser escollit com el primer d'una sèrie d'articles dedicats a provar clients RDP mitjançant l'analitzador estàtic PVS-Studio.
Una mica d'història
Projecte va sorgir després que Microsoft obrís les especificacions del seu protocol RDP propietari. En aquell moment, hi havia un client rdesktop, la implementació del qual es basava en els resultats de l'enginyeria inversa.
A mesura que es va implementar el protocol, es va fer més difícil afegir noves funcionalitats a causa de l'arquitectura del projecte existent. Els canvis en ell van donar lloc a un conflicte entre desenvolupadors, que va portar a la creació d'una bifurcació de rdesktop - FreeRDP. La distribució addicional del producte estava limitada per la llicència GPLv2, com a resultat de la qual cosa es va prendre la decisió de rellicenciar-lo a la llicència d'Apache v2. Tanmateix, no tothom va acceptar canviar la llicència del seu codi, de manera que els desenvolupadors van decidir reescriure el projecte, donant lloc a una base de codi moderna.
Podeu llegir més sobre la història del projecte a la publicació oficial del blog: "La història del projecte FreeRDP".
S'utilitza com a eina per identificar errors i vulnerabilitats potencials en el codi. És un analitzador de codi estàtic per a C, C++, C# i Java, disponible a plataformes Windows, Linux и macOS.
L'article només presenta aquells errors que em van semblar més interessants.
Pèrdua de memòria
S'ha sortit de la funció sense deixar anar el punter 'cwd'. És possible una fuga de memòria. medi ambient.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;
....
}Aquest fragment s'ha extret del subsistema winpr, que implementa un contenidor WINAPI per a no-Windows sistemes, és a dir, és un anàleg lleuger de Wine. Aquí podeu veure una fuita: memòria assignada per la funció getcwd, només s'allibera quan es tracten casos especials. Per corregir l'error, cal afegir una trucada lliure després memcpy.
Array fora dels límits
És possible l'excés de matriu. El valor de l'índex "event->EventHandlerCount" podria arribar a 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++;
}
....
}Aquest exemple afegeix un nou element a la llista encara que el nombre d'elements hagi arribat al màxim. Aquí n'hi ha prou amb substituir l'operador <= en <, per no anar més enllà dels límits de la matriu.
S'ha trobat un altre error d'aquest tipus:
- V557 L'excés de matriu és possible. El valor de l'índex "iBitmapFormat" podria arribar a 8. orders.c 2623
Errors tipogràfics
Fragment 1
L'expressió '!pipe->In' sempre és 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;
....
}Aquí veiem una errada comú: la segona condició verifica la mateixa variable que la primera. El més probable és que l'error hagi aparegut com a resultat d'una còpia de codi sense èxit.
Fragment 2
Es van trobar dos blocs de text idèntic. El segon bloc comença des de la línia 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);
....
}Una altra errada: el comentari del codi implica que el fil hauria d'arribar minorVersion, però, la lectura es produeix en una variable anomenada majorVersió. Tanmateix, no estic familiaritzat amb el protocol, així que això és només una suposició.
Fragment 3
És estrany que el cos de la funció 'trio_index_last' sigui totalment equivalent al cos de la funció '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);
}A jutjar pel comentari, la funció trio_index troba la primera coincidència de caràcter en una cadena quan trio_index_últim - l'última cosa. Però els cossos d'aquestes funcions són idèntics! El més probable és que això sigui una errada d'ortografia i en la funció trio_index_últim cal utilitzar strhrchr en comptes de strchr. Aleshores s'esperarà el comportament.
Fragment 4
El punter "dades" de l'expressió és igual a nullptr. El valor resultant de les operacions aritmètiques en aquest punter no té sentit i no s'ha d'utilitzar. 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;
....
}Sembla que l'operador de negació es va perdre aquí accidentalment ! A prop dades. És estrany que això passi desapercebut.
Fragment 5
S'ha detectat l'ús del patró "si (A) {…} else if (A) {...}". Hi ha una probabilitat de presència d'error lògic. Consulteu les línies: 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);
}
....
}Les dues últimes condicions són les mateixes: sembla que algú s'ha oblidat de comprovar-les després de copiar-les. Des del codi es nota que l'última part funciona amb valors de quatre bytes, de manera que podem suposar que l'última condició hauria de ser valor <= 0x3FFFFFFFF.
S'ha trobat un altre error d'aquest tipus:
- V517 S'ha detectat l'ús del patró "si (A) {…} else if (A) {...}". Hi ha una probabilitat de presència d'error lògic. Línies de control: 169, 173. fitxer.c 169
Validació de les dades d'entrada
Fragment 1
L'expressió 'strcat(target, source) != NULL' sempre és certa. 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);
}Comprovar el resultat de la funció en aquest exemple és incorrecte. Funció strcat retorna un punter a la versió final de la cadena, és a dir. el primer paràmetre passat. En aquest cas ho és target. Tanmateix, si és igual NULL, llavors és massa tard per comprovar-ho, ja que a la funció strcat serà desreferenciat.
Fragment 2
L'expressió "caché" sempre és certa. glif.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)
{
....
}
....
}En aquest cas la variable cache s'assigna l'adreça d'una matriu estàtica glyphCache->glyphCache. Per tant, comproveu si (caché) es pot ometre.
Error de gestió de recursos
El recurs es va adquirir mitjançant la funció 'CreateFileA', però es va publicar mitjançant la funció 'fclose' incompatible. certificat.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;
}
....
}Descriptor de fitxer fp, creat per una crida de funció CreateFile tancat per error per funció f tancar de la biblioteca estàndard, no CloseHandle.
Mateixes condicions
Les expressions condicionals de les declaracions "si" situades una al costat de l'altra són idèntiques. Comproveu les línies: 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);
}
....
}Aquest exemple pot no ser un error. Tanmateix, ambdues condicions contenen els mateixos missatges, un dels quals probablement es pot eliminar.
Neteja de punters nuls
El punter nul es passa a la funció "lliure". Inspeccioneu el primer argument. targeta intel·ligent_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);
....
}En funció lliure podeu passar un punter nul i l'analitzador ho sap. Però si es detecta una situació en què el punter sempre es passa nul, com en aquest fragment, s'emetrà un avís.
Punter mszGroupsA inicialment iguals NULL i no s'inicia en cap altre lloc. L'única branca de codi on es podria inicialitzar el punter és inaccessible.
Hi havia altres missatges com:
- V575 El punter nul es passa a la funció "lliure". Inspeccioneu el primer argument. llicència.c 790
- V575 El punter nul es passa a la funció "lliure". Inspeccioneu el primer argument. rdpsnd_alsa.c 575
Molt probablement, aquestes variables oblidades sorgeixen durant el procés de refactorització i simplement es poden eliminar.
Possible desbordament
Possible desbordament. Considereu l'emissió d'operands, no el resultat. 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));
....
}Portant el resultat a llarg no és protecció contra desbordament, ja que el càlcul en si es produeix utilitzant el tipus int.
Desreferència del punter en la inicialització
El punter "context" es va utilitzar abans de verificar-lo amb nullptr. Consulteu les línies: 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;
....
}Aquí teniu el punter context es desfereix durant la inicialització, abans de comprovar-ho.
S'han trobat altres errors d'aquest tipus:
- V595 El punter 'ntlm' es va utilitzar abans de verificar-lo contra nullptr. Consulteu les línies: 236, 255. ntlm.c 236
- V595 El punter "context" es va utilitzar abans de verificar-lo amb nullptr. Comproveu les línies: 1003, 1007. rfx.c 1003
- V595 El punter 'rdpei' es va utilitzar abans de verificar-lo contra nullptr. Consulteu les línies: 176, 180. rdpei_main.c 176
- V595 El punter 'gdi' es va utilitzar abans de verificar-lo contra nullptr. Comproveu les línies: 121, 123. xf_gfx.c 121
Condició sense sentit
L'expressió 'rdp->state >= CONNECTION_STATE_ACTIVE' sempre és certa. connexió.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;
}
....
}
....
}És fàcil veure que la primera condició no té sentit a causa del valor corresponent que s'ha assignat abans.
Anàlisi de cadenes incorrecta
Format incorrecte. Penseu en comprovar el tercer argument real de la funció 'sscanf'. S'espera un punter al tipus int sense signar. proxy.c 220
Una part de l'expressió condicional sempre és certa: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}L'analitzador emet immediatament 2 advertències per a aquest fragment. Especificador %u espera una variable de tipus int signat, però variable sub té tipus int. A continuació veiem un control sospitós: la condició de la dreta no té sentit, ja que al principi hi ha una comparació amb una. No sé què volia dir l'autor d'aquest codi, però hi ha alguna cosa clarament incorrecte.
Comprovacions fora de comanda
L'expressió 'estat == 0x00090314' sempre és 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;
....
}Les condicions verificades sempre seran falses, ja que l'execució només arribarà a la segona condició si estat == SEC_E_OK. El codi correcte podria semblar així:
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;
}Conclusió
Així, la comprovació del projecte va revelar molts problemes, però només la part més interessant es va descriure a l'article. Els desenvolupadors del projecte poden comprovar el projecte ells mateixos sol·licitant una clau de llicència temporal al lloc web . També hi va haver falsos positius, treballs que ajudaran a millorar l'analitzador. Tanmateix, l'anàlisi estàtica és important si no només voleu millorar la qualitat del vostre codi, sinó també reduir el temps dedicat a trobar errors, i PVS-Studio us pot ajudar.
Si voleu compartir aquest article amb un públic de parla anglesa, utilitzeu l'enllaç de traducció: Sergey Larin.
Font: www.habr.com
