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

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

بعض التاريخ

بدأ مشروع FreeRDP بعد أن فتحت Microsoft مواصفات بروتوكول الملكية RDP الخاص بها. في تلك اللحظة ، كان عميل يسمى rdesktop قيد الاستخدام بالفعل ، يعتمد معظمه على أعمال الهندسة العكسية.

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

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

لقد استخدمت PVS-Studio لمسح المشروع بحثًا عن الأخطاء ونقاط الضعف المحتملة. 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 ، أي يعمل بمثابة مكافئ أخف من Wine. يحتوي الكود أعلاه على تسرب للذاكرة: يتم تحرير الذاكرة المخصصة بواسطة وظيفة 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); .... } 

خطأ مطبعي آخر: يشير التعليق إلى أننا يجب أن نتوقع قراءة المتغير minorVersion من الدفق ، بينما تتم قراءة القيمة في المتغير 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); .... } 

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

يتم تعيين مؤشر 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)) { .... } .... } 

هذا الكود يطلق تحذيرين في آن واحد. يتم استخدام العنصر النائب ٪ u للمتغيرات من النوع غير الموقعة int ، بينما المتغير الفرعي من النوع 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 في متناول يدي.