Vérification de FreeRDP avec l'analyseur PVS-Studio

FreeRDP est une implémentation open source du protocole Remote Desktop Protocol (RDP), un protocole qui implémente le contrôle informatique à distance développé par Microsoft. Le projet prend en charge de nombreuses plates-formes, notamment Windows, Linux, macOS et même iOS avec Android. Ce projet a été sélectionné comme le premier d'une série d'articles consacrés à la vérification des clients RDP à l'aide de l'analyseur statique PVS-Studio.

Un peu d'histoire

Le projet FreeRDP est venu après que Microsoft a dévoilé les spécifications de son protocole propriétaire RDP. A cette époque, il y avait un client rdesktop, dont l'implémentation est basée sur les résultats de la Reverse Engineering.

Au cours de la mise en œuvre du protocole, il est devenu plus difficile d'ajouter de nouvelles fonctionnalités en raison de l'architecture de projet alors existante. Des modifications ont créé un conflit entre les développeurs, ce qui a conduit à la création de la fourche rdesktop - FreeRDP. La distribution ultérieure du produit a été limitée par la licence GPLv2, à la suite de laquelle il a été décidé de renouveler la licence pour Apache License v2. Cependant, tout le monde n'a pas accepté de changer la licence de leur code, les développeurs ont donc décidé de réécrire le projet, ce qui nous donne une apparence moderne de la base de code.

Plus de détails sur l'histoire du projet peuvent être trouvés dans la note officielle du blog: «L'histoire du projet FreeRDP».

PVS-Studio a été utilisé comme un outil pour détecter les erreurs et les vulnérabilités potentielles dans le code. Il s'agit d'un analyseur de code statique pour C, C ++, C # et Java, disponible sur Windows, Linux et macOS.

L'article ne présente que les erreurs qui m'ont paru les plus intéressantes.

Fuite de mémoire

V773 La fonction a été quittée sans libérer le pointeur 'cwd'. Une fuite de mémoire est possible. environnement.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; .... } 

Ce fragment a été extrait du sous-système winpr qui implémente l'encapsuleur WINAPI pour les systèmes non Windows, c'est-à-dire C'est une contrepartie légère à Wine. Il y a une fuite ici: la mémoire allouée par la fonction getcwd n'est libérée que lors de cas particuliers. Pour corriger l'erreur, ajoutez un appel à free après memcpy .

Aller au-delà des limites d'un tableau

V557 Le dépassement de matrice est possible. La valeur de l'index 'event-> EventHandlerCount' pourrait atteindre 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++; } .... } 

Dans cet exemple, un nouvel élément est ajouté à la liste, même si le nombre d'éléments a atteint le maximum. Il suffit de remplacer l'opérateur <= par < pour ne pas dépasser les limites du tableau.

Une autre erreur de ce type a été trouvée:

• V557 Le dépassement de matrice est possible. La valeur de l'indice 'iBitmapFormat' pourrait atteindre 8. Orders.c 2623

Typos

Fragment 1

V547 L' expression '! Pipe-> In' est toujours fausse. 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; .... } 

Nous voyons ici la faute de frappe habituelle: dans la deuxième condition, la même variable est vérifiée que dans la première. Très probablement, l'erreur est apparue à la suite d'une copie infructueuse du code.

Fragment 2

V760 Deux blocs de texte identiques ont été trouvés. Le deuxième bloc commence à la ligne 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); .... } 

Autre faute de frappe: un commentaire sur le code implique que minorVersion doit provenir du flux, cependant, la lecture a lieu dans une variable nommée majorVersion . Cependant, je ne connais pas le protocole, ce n'est donc qu'une hypothèse.

Fragment 3

V524 Il est étrange que le corps de la fonction 'trio_index_last' soit entièrement équivalent au corps de la fonction '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 en juger par le commentaire, la fonction trio_index trouve la première correspondance d'un caractère dans une chaîne lorsque trio_index_last est le dernier. Mais les corps de ces fonctions sont identiques! Il s'agit très probablement d'une faute de frappe et dans la fonction trio_index_last , vous devez utiliser strrchr au lieu de strchr . Ensuite, le comportement sera attendu.

Fragment 4

V769 Le pointeur 'données' dans l'expression est égal à nullptr. La valeur résultante des opérations arithmétiques sur ce pointeur est insensée et ne doit pas être utilisée. 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; .... } 

On dirait qu'ils ont accidentellement raté l'opérateur de négation ! à côté des données . Il est étrange que cela soit passé inaperçu.

Fragment 5

V517 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifiez les lignes: 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 deux dernières conditions sont les mêmes: apparemment, quelqu'un a oublié de les vérifier après la copie. Selon le code, il est à noter que la dernière partie fonctionne avec des valeurs à quatre octets, nous pouvons donc supposer que la dernière condition doit être une valeur <= 0x3FFFFFFF .

Une autre erreur de ce type a été trouvée:

• V517 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifier les lignes: 169, 173. file.c 169

Validation des entrées

Fragment 1

V547 L' expression 'strcat (cible, source)! = NULL' est toujours vraie. 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); } 

La vérification du résultat de la fonction dans cet exemple est incorrecte. La fonction strcat renvoie un pointeur sur la version finale de la chaîne, c'est-à-dire premier paramètre passé. Dans ce cas, c'est la cible . Cependant, s'il est NULL , il est trop tard pour le vérifier, car dans la fonction strcat , il sera déréférencé.

Fragment 2

V547 Le «cache» d'expression est toujours vrai. 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) { .... } .... } 

Dans ce cas, la variable de cache se voit attribuer l'adresse du tableau statique glyphCache-> glyphCache . Ainsi, la vérification if (cache) peut être omise.

Erreur de gestion des ressources

V1005 La ressource a été acquise à l'aide de la fonction 'CreateFileA' mais a été libérée à l'aide de la fonction 'fclose' incompatible. 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; } .... } 

Le descripteur de fichier fp créé en appelant la fonction CreateFile est fermé par erreur par la fonction fclose de la bibliothèque standard, et non par CloseHandle .

Mêmes conditions

V581 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 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); } .... } 

Cet exemple n'est peut-être pas une erreur. Cependant, les deux conditions contiennent le même message, dont l'un, très probablement, peut être supprimé.

Effacement des pointeurs nuls

V575 Le pointeur nul est passé dans la fonction 'libre'. Inspectez le premier 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); .... } 

Vous pouvez transmettre un pointeur nul à la fonction libre et l'analyseur le sait. Mais si une situation est révélée dans laquelle le pointeur est toujours passé à zéro, comme dans ce fragment, un avertissement sera émis.

Le pointeur mszGroupsA est initialement NULL et n'est initialisé nulle part ailleurs. La seule branche de code où le pointeur a pu être initialisé est inaccessible.

Il y avait d'autres articles comme celui-ci:

• V575 Le pointeur nul est passé dans la fonction 'libre'. Inspectez le premier argument. license.c 790
• V575 Le pointeur nul est passé dans la fonction 'libre'. Inspectez le premier argument. rdpsnd_alsa.c 575

Très probablement, ces variables oubliées surviennent lors du refactoring et peuvent être simplement supprimées.

Débordement possible

V1028 Débordement possible. Pensez à couler des opérandes, pas le résultat. 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)); .... } 

Porter le résultat trop longtemps n'est pas une protection contre le débordement, puisque le calcul lui-même est effectué en utilisant le type int .

Déréférencement du pointeur lors de l'initialisation

V595 Le pointeur «contexte» a été utilisé avant d'être vérifié par rapport à nullptr. Vérifier les lignes: 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; .... } 

Ici, le pointeur de contexte est déréférencé dans l'initialisation - avant qu'il ne soit vérifié.

D'autres erreurs de ce type ont été trouvées:

• V595 Le pointeur 'ntlm' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 236, 255. ntlm.c 236
• V595 Le pointeur «contexte» a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 1003, 1007. rfx.c 1003
• V595 Le pointeur 'rdpei' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 176, 180. rdpei_main.c 176
• V595 Le pointeur 'gdi' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 121, 123. xf_gfx.c 121

Condition vide de sens

V547 L' expression 'rdp-> state> = CONNECTION_STATE_ACTIVE' est toujours vraie. connection.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; } .... } .... } 

Il est facile de voir que la première condition n'a pas de sens en raison de l'affectation antérieure de la valeur correspondante.

Analyse de ligne incorrecte

V576 Format incorrect. Pensez à vérifier le troisième argument réel de la fonction 'sscanf'. Un pointeur vers le type int non signé est attendu. proxy.c 220

V560 Une partie de l'expression conditionnelle est toujours vraie: (rc> = 0). proxy.c 222

 static BOOL check_no_proxy(....) { .... int sub; int rc = sscanf(range, "%u", &sub); if ((rc == 1) && (rc >= 0)) { .... } .... } 

L'analyseur de ce fragment émet immédiatement 2 avertissements. Le spécificateur % u attend une variable de type unsigned int , mais la variable sub est de type int . Ensuite, nous voyons un contrôle suspect: la condition de droite n'a pas de sens, car au début il y a une comparaison avec l'unité. Je ne sais pas ce que l’auteur de ce code avait en tête, mais il y a clairement quelque chose qui cloche ici.

Chèques non ordonnés

V547 L' expression 'status == 0x00090314' est toujours fausse. 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 conditions marquées seront toujours fausses, car l'exécution n'atteindra la deuxième condition que si status == SEC_E_OK . Le code correct pourrait ressembler à ceci:

 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; } 

Conclusion

Ainsi, la vérification du projet a révélé de nombreux problèmes, mais seule la partie la plus intéressante a été décrite dans l'article. Les développeurs de projets peuvent vérifier le projet eux-mêmes en demandant une clé de licence temporaire sur le site Web de PVS-Studio . Il y avait également des faux positifs, dont les travaux contribueront à améliorer l'analyseur. Néanmoins, l'analyse statique est importante si vous souhaitez non seulement améliorer la qualité du code, mais également réduire le temps de recherche d'erreurs, et PVS-Studio peut vous y aider.

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Larin. Vérification de FreeRDP avec PVS-Studio