FreeRDP – une implémentation open source du protocole Remote Desktop Protocol (RDP), qui permet le contrôle à distance d'un ordinateur, développé par Microsoft. Le projet prend en charge de nombreuses plateformes, y compris Windows, Linux, macOS et même iOS avec Android. Ce projet a été choisi comme premier dans le cadre d'une série d'articles consacrés à l'évaluation des clients RDP à l'aide de l'analyseur statique PVS-Studio.
Un peu d'histoire
Projet est apparu après que Microsoft a ouvert les spécifications de son protocole propriétaire RDP. À l'époque, il existait un client rdesktop, dont l'implémentation est basée sur des résultats d'ingénierie inverse.
Dans le processus de mise en œuvre du protocole, il devenait de plus en plus difficile d'ajouter de nouvelles fonctionnalités en raison de l'architecture existante du projet. Les changements dans celle-ci ont engendré un conflit entre les développeurs, ce qui a conduit à la création d'un fork de rdesktop, – FreeRDP. La diffusion ultérieure du produit était limitée par la licence GPLv2, ce qui a conduit à la décision de le re-licencier sous la licence Apache v2. Cependant, tous n'étaient pas d'accord pour changer la licence de leur code, incitant ainsi les développeurs à réécrire le projet, ce qui nous donne l'apparence moderne de la base de code.
Pour plus de détails sur l'histoire du projet, vous pouvez lire la note du blog officiel : « L'histoire du projet FreeRDP ».
Un outil pour détecter des erreurs et des vulnérabilités potentielles dans le code a été utilisé. . Il s'agit d'un analyseur statique de code pour les langages C, C++, C# et Java, disponible sur les plateformes Windows, Linux et macOS.
L'article présente seulement les erreurs qui m'ont paru les plus intéressantes.
Fuite de mémoire
La fonction a quitté sans libérer le pointeur ‘cwd’. Une fuite de mémoire est possible. 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;
....
}Ce fragment a été extrait de la sous-système winpr, qui implémente un wrapper WINAPI pour les systèmes non-Windows, c'est-à-dire un équivalent léger de Wine. Ici, on peut observer une fuite : la mémoire allouée par la fonction getcwd, n'est libérée que lors du traitement de cas particuliers. Pour corriger cette erreur, il faut ajouter un appel à free après memcpy.
Dépassement de tableau
Un dépassement de tableau 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 <= sur <, pour ne pas dépasser la taille du tableau.
Une autre erreur de ce type a été trouvée :
- V557 Un dépassement de tableau est possible. La valeur de l'index ‘iBitmapFormat’ pourrait atteindre 8. orders.c 2623
Fautes de frappe
Fragment 1
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;
....
}Ici, nous voyons une erreur typique : dans la deuxième condition, la même variable est vérifiée que dans la première. L'erreur est probablement survenue à la suite d'une copie de code mal faite.
Fragment 2
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);
....
}Encore une faute de frappe : le commentaire du code indique que depuis le flux, il devrait arriver minorVersion, pourtant la lecture se fait dans la variable appelée majorVersion. Cependant, je ne suis pas familier avec le protocole, donc ce n'est qu'une supposition.
Fragment 3
Il est étrange que le corps de la fonction ‘trio_index_last’ soit totalement équivalent à celui 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);
}Selon le commentaire, la fonction trio_index trouve la première occurrence d'un caractère dans la chaîne, alors que trio_index_last est censée trouver la dernière. Mais les corps de ces fonctions sont identiques ! Il s'agit probablement d'une faute de frappe, et la fonction trio_index_last devrait utiliser strrchr au lieu de strchr. Cela donnerait un comportement attendu.
Fragment 4
Le pointeur ‘data’ dans l'expression est égal à nullptr. La valeur résultante des opérations arithmétiques sur ce pointeur n'a aucun sens et il ne devrait pas être utilisé. 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;
....
}Il semble qu'ici, l'opérateur de négation ait été accidentellement omis ! à côté de data. C'est étrange que cela soit passé inaperçu.
Fragment 5
Le motif ‘if (A) {…} else if (A) {…}’ a été détecté. 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 > 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);
}
....
}Les deux dernières conditions sont identiques : apparemment, quelqu'un a oublié de les vérifier après avoir copié. Le code montre que la dernière partie fonctionne avec des valeurs de quatre octets, il est donc raisonnable de supposer que la dernière condition devrait être value <= 0x3FFFFFFF.
Une autre erreur de ce type a été trouvée :
- V517 Le motif d'utilisation de 'if (A) {...} else if (A) {...}' a été détecté. Il y a une probabilité de présence d'une erreur logique. Vérifiez les lignes : 169, 173. file.c 169
Vérification des données d'entrée
Fragment 1
L'expression 'strcat(target, 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 le premier paramètre passé. Dans ce cas, il s'agit de cible. Cependant, s'il est égal à NULL, alors il est trop tard pour le vérifier, car dans la fonction strcat il sera déréférencé.
Fragment 2
L'expression 'cache' est toujours vraie. 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 cache reçoit l'adresse d'un tableau statique glyphCache->glyphCache. Ainsi, la vérification if (cache) peut être omise.
Erreur de gestion des ressources
La ressource a été acquise en utilisant la fonction 'CreateFileA', mais a été libérée en utilisant la fonction incompatibles '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;
}
....
}Le descripteur de fichier fp, créé par l'appel de la fonction CreateFile, a été fermé par erreur par la fonction fclose de la bibliothèque standard, et non CloseHandle.
Conditions identiques
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, "avertissement : NdrComplexStructBufferSize array_type : "
"0xX non implémenté", 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, "avertissement : NdrComplexStructBufferSize array_type : "
"0xX non implémenté", array_type);
}
....
}Cet exemple n'est peut-être pas une erreur. Cependant, les deux conditions contiennent des messages identiques, dont l'un peut probablement être supprimé.
Nettoyage des pointeurs nuls
Le pointeur nul est passé à la fonction ‘free’. 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 n'est pas supporté par 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);
....
}Dans la fonction free On peut transmettre un pointeur nul et l'analiseur en est conscient. Mais si une situation se présente où le pointeur est toujours transmis comme nul, comme dans ce fragment, un avertissement sera émis.
Pointeur mszGroupsA initialement égal NULL et n'est plus initialisé ailleurs. La seule branche de code où le pointeur aurait pu être initialisé est inaccessible.
Il y avait d'autres messages de ce type :
- V575 Le pointeur nul est passé à la fonction ‘free’. Inspectez le premier argument. license.c 790
- V575 Le pointeur nul est passé à la fonction ‘free’. Inspectez le premier argument. rdpsnd_alsa.c 575
Des variables oubliées comme celles-ci surviennent probablement lors du refactoring et peuvent simplement être supprimées.
Surcharge possible
Surcharge possible. Envisagez de caster les 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));
....
}Le cast du résultat à long n'est pas une protection contre la surcharge, car le calcul lui-même se fait en utilisant le type int.
Déréférencement du pointeur lors de l'initialisation
Le pointeur ‘context’ a été utilisé avant d'être vérifié contre nullptr. Vérifiez 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 context est déréférencé lors de l'initialisation — plus tôt que sa vérification ne se produise.
D'autres erreurs de ce type ont été trouvées :
- V595 Le pointeur 'ntlm' a été utilisé avant d'avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 236, 255. ntlm.c 236
- V595 Le pointeur 'context' a été utilisé avant d'avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 1003, 1007. rfx.c 1003
- V595 Le pointeur 'rdpei' a été utilisé avant d'avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 176, 180. rdpei_main.c 176
- V595 Le pointeur 'gdi' a été utilisé avant d'avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 121, 123. xf_gfx.c 121
Condition inutile
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; // state >= CONNECTION_STATE_ACTIVE) // 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'attribution de la valeur correspondante plus tôt.
Analyse de chaîne incorrecte
Format incorrect. Vérifiez le troisième argument réel de la fonction 'sscanf'. Un pointeur vers le type unsigned int est attendu. proxy.c 220
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 pour 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 une vérification douteuse : la condition à droite n'a pas de sens, car elle compare d'abord à un. Je ne sais pas ce que l'auteur de ce code voulait dire, mais il y a clairement quelque chose qui ne va pas ici.
Vérifications non ordonnées
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 atteindra la deuxième condition uniquement si status == SEC_E_OK. Le code correct pourrait ressembler à :
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 du projet peuvent vérifier eux-mêmes le projet en demandant une clé de licence temporaire sur le site. . Il y a également eu des faux positifs, dont le travail contribuera à améliorer l'analyseur. Néanmoins, l'analyse statique est importante si vous souhaitez non seulement améliorer la qualité du code, mais aussi réduire le temps consacré à la 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.
Source : habr.com
