التحقق من FreeRDP مع محلل PVS-Studio

FreeRDP هو تطبيق مفتوح المصدر لبروتوكول سطح المكتب البعيد (RDP) ، وهو بروتوكول يقوم بتطبيق التحكم في الكمبيوتر عن بُعد الذي طورته شركة مايكروسوفت. يدعم المشروع العديد من المنصات ، بما في ذلك Windows و Linux و macOS وحتى iOS مع Android. تم اختيار هذا المشروع كأول مجموعة في سلسلة من المقالات المخصصة للتحقق من عملاء RDP باستخدام محلل ثابت PVS-Studio.

قليلا من التاريخ

جاء مشروع FreeRDP بعد أن كشفت Microsoft عن مواصفات بروتوكول RDP الخاص بها. في ذلك الوقت ، كان هناك عميل rdesktop ، يعتمد تطبيقه على نتائج Reverse Engineering.

في عملية تنفيذ البروتوكول ، أصبح من الصعب إضافة وظائف جديدة بسبب بنية المشروع القائمة آنذاك. التغييرات التي حدثت في ذلك خلق تعارض بين المطورين ، مما أدى إلى إنشاء شوكة rdesktop - FreeRDP. تم تقييد التوزيع الإضافي للمنتج بواسطة ترخيص GPLv2 ، ونتيجة لذلك تم اتخاذ قرار بإعادة ترخيص Apache License v2. ومع ذلك ، لم يوافق الجميع على تغيير ترخيص الكود الخاص بهم ، لذلك قرر المطورون إعادة كتابة المشروع ، ونتيجة لذلك أصبح لدينا نظرة حديثة لقاعدة الكود.

يمكن العثور على مزيد من التفاصيل حول تاريخ المشروع في مذكرة المدونة الرسمية: "تاريخ مشروع FreeRDP".

تم استخدام PVS-Studio كأداة للكشف عن الأخطاء ونقاط الضعف المحتملة في التعليمات البرمجية. إنه محلل ثابت للكود C و C ++ و C # و Java ، وهو متوفر على أنظمة Windows و Linux و macOS.

يعرض المقال فقط تلك الأخطاء التي بدا لي أنها الأكثر إثارة للاهتمام.

تسرب الذاكرة

V773 تم إنهاء الوظيفة دون تحرير مؤشر "cwd". تسرب الذاكرة ممكن. 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; .... } 

تم أخذ هذه القطعة من النظام الفرعي winpr الذي يقوم بتطبيق برنامج تجميع WINAPI للأنظمة التي لا تعمل بنظام Windows ، أي وهو نظير خفيف الوزن إلى النبيذ. يوجد تسرب هنا: يتم تحرير الذاكرة المخصصة بواسطة وظيفة getcwd فقط خلال الحالات الخاصة. لإصلاح الخطأ ، أضف مكالمة مجانية بعد memcpy .

تجاوز حدود المصفوفة

V557 تجاوز الصفيف هو ممكن. قد تصل قيمة فهرس "Event-> EventHandlerCount" إلى 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++; } .... } 

في هذا المثال ، تتم إضافة عنصر جديد إلى القائمة ، حتى إذا وصل عدد العناصر إلى الحد الأقصى. يكفي استبدال المشغل <= ب < حتى لا يتجاوز حدود المصفوفة.

تم العثور على خطأ آخر من هذا النوع:

• V557 تجاوز الصفيف هو ممكن. قد تصل قيمة مؤشر "iBitmapFormat" إلى 8. orders.c 2623

أخطاء مطبعية

جزء 1

تعبير V547 '! Pipe-> In' غير صحيح دائمًا. 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; .... } 

هنا نرى الخطأ المطبعي المعتاد: في الحالة الثانية ، يتم التحقق من المتغير نفسه كما في الأول. على الأرجح ، ظهر الخطأ نتيجة النسخ غير الناجح للرمز.

جزء 2

V760 تم العثور على كتلتين متطابقتين من النص. يبدأ الكتلة الثانية من السطر 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); .... } 

خطأ مطبعي آخر: يشير التعليق على الكود إلى أنه يجب أن يكون قاصر ثانوي من الدفق ، ومع ذلك ، تتم القراءة في متغير باسم majorVersion . ومع ذلك ، لست على دراية بالبروتوكول ، لذلك هذا مجرد افتراض.

جزء 3

V524 من الغريب أن يكون نص الدالة 'trio_index_last' مساوياً بالكامل لنص الدالة '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); } 

اذا حكمنا من خلال التعليق ، فإن الدالة trio_index تعثر على المطابقة الأولى للحرف في السلسلة عندما يكون trio_index_last هو الأخير. لكن جثث هذه الوظائف متطابقة! على الأرجح ، هذا خطأ مطبعي ، وفي الدالة trio_index_last تحتاج إلى استخدام strrchr بدلاً من strchr . ثم سيتم توقع السلوك.

جزء 4

V769 مؤشر "البيانات" في التعبير يساوي nullptr. القيمة الناتجة عن العمليات الحسابية على هذا المؤشر لا معنى لها ولا يجب استخدامها. 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; .... } 

يبدو أنهم غابوا بطريق الخطأ عن عامل النفي ! بجانب البيانات . والغريب أن هذا ذهب دون أن يلاحظها أحد.

جزء 5

V517 استخدام نمط "if (A) {...} آخر إذا تم اكتشاف (A) {...}". هناك احتمال لوجود خطأ منطقي. خطوط التحقق: 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); } .... } 

الشرطان الأخيران متشابهان: على ما يبدو ، نسي أحدهم التحقق منها بعد النسخ. وفقًا للرمز ، من الملاحظ أن الجزء الأخير يعمل مع قيم أربعة بايت ، لذلك يمكننا افتراض أن الشرط الأخير يجب أن يكون القيمة <= 0x3FFFFFFF .

تم العثور على خطأ آخر من هذا النوع:

• V517 استخدام نمط "if (A) {...} آخر إذا تم اكتشاف (A) {...}". هناك احتمال لوجود خطأ منطقي. خطوط التحقق: 169 ، 173. file.c 169

التحقق من صحة المدخلات

جزء 1

V547 التعبير 'strcat (الهدف ، المصدر)! = NULL' صحيح دائمًا. 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); } 

التحقق من نتيجة الوظيفة في هذا المثال غير صحيح. تقوم دالة strcat بإرجاع مؤشر إلى الإصدار النهائي من السلسلة ، أي المعلمة الأولى مرت. في هذه الحالة ، هو الهدف . ومع ذلك ، إذا كان NULL ، فقد فات الأوان للتحقق ، لأنه في وظيفة strcat سيتم إلغاء ترجمته .

جزء 2

V547 التعبير "ذاكرة التخزين المؤقت" هو دائما صحيح. 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) { .... } .... } 

في هذه الحالة ، يتم تعيين متغير ذاكرة التخزين المؤقت عنوان صفيف ثابت glyphCache-> glyphCache . وبالتالي ، يمكن حذف الاختيار (ذاكرة التخزين المؤقت) .

خطأ في إدارة الموارد

V1005 تم الحصول على المورد باستخدام وظيفة "CreateFileA" ولكن تم إصداره باستخدام وظيفة "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; } .... } 

يتم إغلاق واصف ملف fp الذي تم إنشاؤه عن طريق استدعاء دالة CreateFile عن طريق الخطأ بواسطة الدالة fclose من المكتبة القياسية ، وليس CloseHandle .

نفس الظروف

V581 التعبيرات الشرطية لعبارات "if" الموجودة بجانب بعضها البعض متطابقة. خطوط التحقق: 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); } .... } 

ربما هذا المثال ليس خطأ. ومع ذلك ، كلا الشرطين يحتوي على نفس الرسالة ، واحدة منها ، على الأرجح ، يمكن إزالتها.

مسح مؤشرات فارغة

V575 يتم تمرير المؤشر الفارغ إلى وظيفة "حرة". تفقد الحجة الأولى. 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); .... } 

يمكنك تمرير مؤشر فارغ إلى الوظيفة الحرة ، والمحلل يعرف عنها. ولكن إذا تم الكشف عن موقف يتم فيه تمرير المؤشر دائمًا إلى الصفر ، كما في هذا الجزء ، فسيصدر تحذير.

مؤشر mszGroupsA مبدئيًا NULL ولا تتم تهيئته في أي مكان آخر. الفرع الوحيد من التعليمات البرمجية حيث يمكن تهيئة المؤشر غير قابل للوصول.

كانت هناك مشاركات أخرى مثل هذا:

• V575 يتم تمرير المؤشر الفارغ إلى وظيفة "حرة". تفقد الحجة الأولى. license.c 790
• V575 يتم تمرير المؤشر الفارغ إلى وظيفة "حرة". تفقد الحجة الأولى. rdpsnd_alsa.c 575

على الأرجح ، تنشأ هذه المتغيرات المنسية أثناء إعادة التجهيز ويمكن حذفها ببساطة.

فيضان ممكن

V1028 تجاوز السعة المحتملة. النظر في صب المعاملات ، وليس النتيجة. 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)); .... } 

إن تحقيق النتيجة لفترة طويلة ليس حماية ضد التدفق الزائد ، حيث يتم إجراء الحساب نفسه باستخدام النوع int .

مؤشر إلغاء التسجيل في التهيئة

V595 تم استخدام مؤشر "السياق" قبل أن يتم التحقق منه مقابل nullptr. خطوط الفحص: 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; .... } 

هنا ، يتم إلغاء تحديد مؤشر السياق في التهيئة - قبل تحديده.

تم العثور على أخطاء أخرى من هذا النوع:

• V595 تم استخدام مؤشر 'ntlm' قبل أن يتم التحقق منه ضد nullptr. خطوط التحقق: 236 ، 255. ntlm.c 236
• V595 تم استخدام مؤشر "السياق" قبل أن يتم التحقق منه مقابل nullptr. خطوط التحقق: 1003 ، 1007. rfx.c 1003
• V595 تم استخدام مؤشر 'rdpei' قبل أن يتم التحقق منه ضد nullptr. خطوط الفحص: 176 ، 180. rdpei_main.c 176
• V595 تم استخدام مؤشر "gdi" قبل أن يتم التحقق منه ضد nullptr. خطوط التحقق: 121 ، 123. xf_gfx.c 121

حالة لا معنى لها

تعبير V547 'rdp-> state> = CONNECTION_STATE_ACTIVE' صحيح دائمًا. 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; } .... } .... } 

من السهل أن نرى أن الشرط الأول لا معنى له بسبب تخصيص القيمة المقابلة في وقت سابق.

تحليل خط غير صحيح

V576 تنسيق غير صحيح. النظر في التحقق من الوسيطة الفعلية الثالثة للدالة 'sscanf'. مؤشر إلى النوع int غير الموقعة متوقع. proxy.c 220

V560 جزء من التعبير الشرطي صحيح دائمًا: (rc> = 0). proxy.c 222

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

محلل لهذه الشظية يعطي على الفور 2 تحذيرات. يتوقع محدد ٪ u متغير نوع غير موقَّع ، لكن المتغير الفرعي هو من النوع int . بعد ذلك ، نرى فحصًا مشبوهًا: الحالة على اليمين لا معنى لها ، لأنه في البداية هناك مقارنة مع الوحدة. لا أعرف ما الذي وضعه صاحب هذا الرمز في الاعتبار ، ولكن من الواضح أن هناك خطأ ما هنا.

الشيكات غير مرتبة

تعبير V547 'status == 0x00090314' غير صحيح دائمًا. 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; .... } 

دائمًا ما تكون الشروط المحددة خاطئة ، لأن التنفيذ لن يصل إلى الشرط الثاني إلا إذا كانت الحالة == SEC_E_OK . قد يبدو الرمز الصحيح كالتالي:

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

استنتاج

وهكذا ، كشفت عملية التحقق من المشروع عن العديد من المشكلات ، لكن الجزء الأكثر إثارة للاهتمام تم وصفه في المقال. يمكن لمطوري المشروع التحقق من المشروع بأنفسهم عن طريق طلب مفتاح ترخيص مؤقت على موقع PVS-Studio . كانت هناك أيضا ايجابيات كاذبة ، والتي سوف تساعد على تحسين محلل. ومع ذلك ، يعد التحليل الثابت أمرًا مهمًا إذا كنت لا ترغب في تحسين جودة الشفرة فحسب ، ولكن أيضًا تقليل وقت البحث عن الأخطاء ، ويمكن لبرنامج PVS-Studio المساعدة في ذلك.

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فالرجاء استخدام الرابط الخاص بترجمة: Sergey Larin. التحقق من FreeRDP مع PVS-Studio