💪 👴🏿 ✡️ خادم مجتمع ONLYOFFICE: كيف تساهم الأخطاء في مشاكل الأمان 👢 👨‍❤️‍👨 ✊🏽

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

المقدمة

ONLYOFFICE Community Server هو نظام تعاون مجاني مفتوح المصدر مصمم لإدارة المستندات والمشاريع وتفاعلات العملاء ورسائل البريد الإلكتروني في مكان واحد. تؤكد الشركة على موقعها الإلكتروني على أمان حلولها بعبارات مثل "إدارة مكتبك الخاص باستخدام ONLYOFFICE" و "تأمين المكتب وتطبيقات الإنتاجية" ولكن ، على ما يبدو ، لا يتم استخدام أدوات لمراقبة جودة الكود في عملية التطوير.

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

لذلك ، ذهبت بعض الأمثلة إلى Twitter :

علق الممثلون لاحقًا على التغريدة ، وحتى نشروا لاحقًا إنكارًا للمشكلة:

هذا على الأرجح صحيح. لكن هذا لا يضيف نقاطًا إلى جودة المشروع. دعونا نرى ما تم العثور عليه هناك أيضًا.

"معالج" التحقق من صحة الإدخال

بعض أساليب المطور للتحقق من صحة بيانات الإدخال ملفتة للنظر في أصالتها.

تحذير 1

V3022 "string.IsNullOrEmpty (" password ") 'عبارة عن خطأ دائمًا. 104

public void SetCredentials(string userName, string password, string domain)
{
    if (string.IsNullOrEmpty(userName))
    {
        throw new ArgumentException("Empty user name.", "userName");
    }
    if (string.IsNullOrEmpty("password"))
    {
        throw new ArgumentException("Empty password.", "password");
    }
    CredentialsUserName = userName;
    CredentialsUserPassword = password;
    CredentialsDomain = domain;
}

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

تحذير 2

V3022 التعبير ' String.IsNullOrEmpty ("name")' خطأ دائمًا. إرسال المعترض CS 36

تعبير V3022 ' String.IsNullOrEmpty ("sendInterceptor")' خطأ دائمًا. 37- إرسال

public SendInterceptorSkeleton(
  string name,
  ....,
  Func<NotifyRequest, InterceptorPlace, bool> sendInterceptor)
{
    if (String.IsNullOrEmpty("name"))                           // <=
        throw new ArgumentNullException("name");
    if (String.IsNullOrEmpty("sendInterceptor"))                // <=
        throw new ArgumentNullException("sendInterceptor");

    method = sendInterceptor;
    Name = name;
    PreventPlace = preventPlace;
    Lifetime = lifetime;
}

فجأة ، كان هناك عدد من الأخطاء المماثلة في الكود. إذا كان الأمر مضحكًا في البداية ، فمن الجدير الآن التفكير في أسباب كتابة هذا الرمز. ربما بقيت هذه العادة بعد التحول من لغة برمجة أخرى. في C ++ ، غالبًا ما يتم إحضار الأخطاء بواسطة مبرمجي Python السابقين في تجربتنا في التحقق من مشاريع C ++.

تحذير 3

V3022 التعبير "معرف <0" خطأ دائمًا. قيمة النوع غير الموقعة هي دائمًا> = 0. UserFolderEngine.cs 173

public MailUserFolderData Update(uint id, string name, uint? parentId = null)
{
    if (id < 0)
        throw new ArgumentException("id");
    ....
}

المتغير id من نوع uint بدون توقيع . لذلك ، التدقيق هنا لا طائل منه. يجدر الانتباه إلى استدعاء هذه الوظيفة. أتساءل ما الذي يتم تمريره لهذه الوظيفة. على الأرجح ، تم استخدام النوع الموقّع int في كل مكان ، ولكن بعد إعادة هيكلة الشيك بقي.

نسخ ولصق الكود

تحذير 1

V3001 هناك تعبيرات فرعية متطابقة 'searchFilterData.WithCalendar == WithCalendar' على يسار ويمين عامل التشغيل '&&'. 131

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

وإليك نفس الشروط التي حذر منها المحلل. بالإضافة إلى إصلاح هذا ، أوصي بأن تقوم بتصميم الكود بشكل أفضل بحيث لا يساهم في ظهور مثل هذه الأخطاء في المستقبل.

التحذير 2

V3030 فحص متكرر. تم التحقق بالفعل من شرط "! String.IsNullOrEmpty (مستخدم)" في السطر 173. CommonLinkUtility.cs 176

public static string GetUserProfile(string user, bool absolute)
{
  var queryParams = "";

  if (!String.IsNullOrEmpty(user))
  {
      var guid = Guid.Empty;
      if (!String.IsNullOrEmpty(user) && 32 <= user.Length && user[8] == '-')
      {
        ....
}

يتم فحص سلسلة المستخدم مرتين على التوالي بنفس الطريقة. ربما يمكن إعادة هيكلة هذا الرمز قليلاً. على الرغم من من يدري ، ربما في إحدى الحالات أرادوا التحقق من المتغير المنطقي المطلق .

تحذير 3

V3021 توجد عبارتان "if" بتعبيرات شرطية متطابقة. تحتوي عبارة "if" الأولى على طريقة إرجاع. هذا يعني أن عبارة "if" الثانية لا معنى لها WikiEngine.cs 688

private static LinkType CheckTheLink(string str, out string sLink)
{
    sLink = string.Empty;

    if (string.IsNullOrEmpty(str))
        return LinkType.None;

    if (str[0] == '[')
    {
        sLink = str.Trim("[]".ToCharArray()).Split('|')[0].Trim();
    }
    else if (....)
    {
        sLink = str.Split('|')[0].Trim();
    }
    sLink = sLink.Split('#')[0].Trim();    // <=
    if (string.IsNullOrEmpty(str))         // <=
        return LinkType.None;

    if (sLink.Contains(":"))
    {
      ....
    }
    ....
}

أنا متأكد من أنه لا يمكنك العثور على خطأ هنا بعينيك. وجد المحلل فحصًا غير مفيد ، والذي تبين في الواقع أنه الرمز المنسوخ من الأعلى. بدلاً من المتغير str ، يجب فحص متغير sLink .

تحذير 4

V3004 عبارة "then" تكافئ جملة "else". SelectelStorage.cs 461

public override string[] ListFilesRelative(....)
{
    var paths = new List<String>();
    var client = GetClient().Result;

    if (recursive)
    {
        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,
            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();
    }
    else
    {
        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,
            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();
    }
    ....
}

اكتشف المحلل كود نسخ ولصق وصفي للغاية. ربما ، في إحدى الحالات ، يجب حساب متغير المسارات بشكل متكرر ، لكن لم يتم ذلك.

تحذير 5

V3009 من الغريب أن تقوم هذه الطريقة دائمًا بإرجاع قيمة واحدة ونفس قيمة "صواب". 318

//TODO: Simplify
public bool SetUnread(List<int> ids, bool unread, bool allChain = false)
{
    ....
    if (!chainedMessages.Any())
        return true;

    var listIds = allChain
        ? chainedMessages.Where(x => x.IsNew == !unread).Select(....).ToList()
        : ids;

    if (!listIds.Any())
        return true;
    ....
    return true;
}

حجم هذه الوظيفة 135 سطرًا. حتى المطورين أنفسهم تركوا تعليقًا مفاده أنه يجب تبسيطه. أنت بالتأكيد بحاجة للعمل مع رمز الوظيفة ، لأن تقوم أيضًا بإرجاع قيمة واحدة في جميع الحالات.

استدعاءات دالة عديمة الفائدة

تحذير 1

V3010 يجب استخدام قيمة الإرجاع الخاصة بالوظيفة "مميزة". 132

public IEnumerable<Tenant> GetTenants(string login, string passwordHash)
{
  //new password
  result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();
  result.Distinct();
  ....
}

يزيل الأسلوب المميز التكرارات من المجموعة. لكن في C # ، لا تقوم معظم طرق الامتداد هذه بتعديل الكائن ، ولكن تقوم بإنشاء نسخة. وبالتالي ، في هذا المثال ، تظل قائمة النتائج كما كانت قبل استدعاء الطريقة. يمكنك أن ترى أيضا أسماء الدخول و passwordHash هنا . ربما هذه مشكلة أمنية أخرى.

تحذير 2

V3010 يجب استخدام قيمة الإرجاع الخاصة بالوظيفة "ToString". 678

private static void ResizeImage(ResizeWorkerItem item)
{
  ....
  using (var stream2 = new MemoryStream(data))
  {
      item.DataStore.Save(fileName, stream2).ToString();

      AddToCache(item.UserId, item.Size, fileName);
  }
  ....
}

طريقة ToString قياسية هنا. تقوم بإرجاع تمثيل نصي للكائن ، ولكن لا يتم استخدام القيمة المعادة.

تحذير 3

V3010 يجب استخدام قيمة الإرجاع الخاصة بالوظيفة "استبدال". TextFileUserImporter.cs 252

private int GetFieldsMapping(....)
{
  ....
  if (NameMapping != null && NameMapping.ContainsKey(propertyField))
  {
      propertyField = NameMapping[propertyField];
  }

  propertyField.Replace(" ", "");
  ....
}

شخص ما ارتكب خطأ فادحا. يجب إزالة جميع المساحات من propertyField لكن هذا لم يحدث منذ لا تغير وظيفة الاستبدال الكائن الأصلي.

تحذير 4

V3038 تم تمرير الوسيطة "" yy "" إلى طريقة "استبدال" عدة مرات. من الممكن أن يتم تمرير حجة أخرى بدلاً من ذلك. MasterLocalizationResources.cs 38

private static string GetDatepikerDateFormat(string s)
{
    return s
        .Replace("yyyy", "yy")
        .Replace("yy", "yy")   // <=
        .Replace("MMMM", "MM")
        .Replace("MMM", "M")
        .Replace("MM", "mm")
        .Replace("M", "mm")
        .Replace("dddd", "DD")
        .Replace("ddd", "D")
        .Replace("dd", "11")
        .Replace("d", "dd")
        .Replace("11", "dd")
        .Replace("'", "")
        ;
}

هنا تتم كتابة استدعاءات وظائف الاستبدال بشكل صحيح ، ولكن في مكان واحد يتم ذلك باستخدام وسيطات متطابقة غريبة.

استثناءات NullReferenceException المحتملة

تحذير 1

V3022 لا يكون التعبير "portalUser.BirthDate.ToString ()" دائمًا فارغًا. عامل التشغيل "؟؟" مفرط. 436

public DateTime? BirthDate { get; set; }

private bool NeedUpdateUser(UserInfo portalUser, UserInfo ldapUser)
{
  ....
  _log.DebugFormat("NeedUpdateUser by BirthDate -> portal: '{0}', ldap: '{1}'",
      portalUser.BirthDate.ToString() ?? "NULL",  // <=
      ldapUser.BirthDate.ToString() ?? "NULL");   // <=
  needUpdate = true;
  ....
}

لن تكون ToString خالية . تم إجراء الفحص هنا لإخراج القيمة "NULL" إلى سجل التصحيح إذا لم يتم تعيين التاريخ. لكن منذ في حالة عدم وجود قيمة ، ستُرجع طريقة ToString سلسلة فارغة ، ثم قد يكون الخطأ في الخوارزمية أقل وضوحًا في السجلات.

تبدو القائمة الكاملة لأماكن التسجيل المشكوك فيها كما يلي:

تعبير V3022 "ldapUser.BirthDate.ToString ()" ليس دائمًا فارغًا. عامل التشغيل "؟؟" مفرط. 437
لا يكون تعبير V3022 "portalUser.Sex.ToString ()" دائمًا فارغًا. عامل التشغيل "؟؟" مفرط. 444
لا يكون تعبير V3022 "ldapUser.Sex.ToString ()" فارغًا دائمًا. عامل التشغيل "؟؟" مفرط. 445 مشروع زراعة الاسنان 445

تحذير 2

V3095 تم استخدام الكائن 'r.Attributes ["href"]' قبل التحقق من أنه خالٍ. فحص الأسطر: 86 ، 87. HelpCenterStorage.cs 86

public override void Init(string html, string helpLinkBlock, string baseUrl)
{
    ....
    foreach (var href in hrefs.Where(r =>
    {
        var value = r.Attributes["href"].Value;
        return r.Attributes["href"] != null
               && !string.IsNullOrEmpty(value)
               && !value.StartsWith("mailto:")
               && !value.StartsWith("http");
    }))
    {
      ....
    }
    ....
}

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

تحذير 3

V3146 مرجعية فارغة محتملة. يمكن أن ترجع "listTags.FirstOrDefault" قيمة فارغة افتراضية. 299

public static void RemoveMarkAsNew(....)
{
  ....
  var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();
  valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;
  ....
}

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

تحذير 4

V3115 يجب ألا يؤدي تمرير "فارغ" إلى أسلوب "يساوي" إلى "NullReferenceException". ResCulture.cs 28

public class ResCulture
{
    public string Title { get; set; }
    public string Value { get; set; }
    public bool Available { get; set; }

    public override bool Equals(object obj)
    {
        return Title.Equals(((ResCulture) obj).Title);
    }
    ....
}

غالبًا ما تتم مقارنة مراجع الكائنات في C # بـ null . لذلك ، عند التحميل الزائد على طرق المقارنة ، من المهم للغاية توقع مثل هذه المواقف وإضافة الفحص المناسب في بداية الوظيفة. لكن هنا لم يفعلوا.

أخطاء أخرى

تحذير 1

V3022 التعبير صحيح دائمًا. ربما يجب استخدام عامل التشغيل "&&" هنا. ListItemHistoryDao.cs 140

public virtual int CreateItem(ListItemHistory item)
{
    if (item.EntityType != EntityType.Opportunity ||   // <=
        item.EntityType != EntityType.Contact)
        throw new ArgumentException();

    if (item.EntityType == EntityType.Opportunity &&
        (DaoFactory.DealDao.GetByID(item.EntityID) == null ||
         DaoFactory.DealMilestoneDao.GetByID(item.StatusID) == null))
        throw new ArgumentException();

    if (item.EntityType == EntityType.Contact &&
        (DaoFactory.ContactDao.GetByID(item.EntityID) == null ||
         DaoFactory.ListItemDao.GetByID(item.StatusID) == null))
        throw new ArgumentException();
    ....
}

سيؤدي استدعاء طريقة CreateItem إلى طرح ArgumentException . النقطة المهمة هي أن أول تعبير شرطي يحتوي على خطأ. يتم تقييم الحالة دائمًا على أنها صحيحة . يكمن الخطأ في اختيار العامل المنطقي. يجب أن تستخدم عامل التشغيل &&.

على الأرجح ، لم يتم استدعاء هذه الطريقة بعد. إنها افتراضية وقد أعيد تعريفها دائمًا في الفئات المشتقة حتى الآن.

من أجل تجنب مثل هذه الأخطاء في المستقبل ، أوصي بقراءة مقالتي وحفظ رابط لها: " التعبيرات المنطقية. كيف يرتكب المحترفون الأخطاء". تم تقديم وتحليل كافة المجموعات الخاطئة من العوامل المنطقية.

تحذير 2

V3052 تم ابتلاع كائن الاستثناء الأصلي" ex ". قد يتم فقد حزمة الاستثناء الأصلي. GoogleDriveStorage.cs 267

public DriveFile CopyEntry(string toFolderId, string originEntryId)
{
    var body = FileConstructor(folderId: toFolderId);
    try
    {
        var request = _driveService.Files.Copy(body, originEntryId);
        request.Fields = GoogleLoginProvider.FilesFields;
        return request.Execute();
    }
    catch (GoogleApiException ex)
    {
        if (ex.HttpStatusCode == HttpStatusCode.Forbidden)
        {
            throw new SecurityException(ex.Error.Message);
        }
        throw;
    }
}

هنا قمنا بتحويل GoogleApiException إلى SecurityException ، مع فقدان المعلومات المفيدة من الاستثناء الأصلي.

تغيير بسيط مثل هذا سيجعل التحذير الذي تم إنشاؤه أكثر إفادة:

throw new SecurityException(ex.Error.Message, ex);

على الرغم من أنه من الممكن تمامًا أن يكون GoogleApiException قد تم إخفاؤه عن قصد .

تم

استخدام عنصر التحذير 3 V3118 Minutes من TimeSpan ، والذي لا يمثل فترة زمنية كاملة. من المحتمل أن تكون قيمة "TotalMinutes" مقصودة بدلاً من ذلك. NotifyClient.cs 281

public static void SendAutoReminderAboutTask(DateTime scheduleDate)
{
    ....
    var deadlineReminderDate = deadline.AddMinutes(-alertValue);

    if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;
    ....
}

كنت أعتقد أن التشخيصات كانت استباقية. في كود مشاريعي ، كان دائمًا يعطي تحذيرات خاطئة. هنا ، أنا متأكد من وجود خطأ. على الأرجح ، يجب عليك استخدام الخاصية TotalMinutes بدلاً من Minutes .

تحذير 4

V3008 يتم تعيين قيم لمتغير "المفتاح" مرتين على التوالي. ربما هذا خطأ. فحص السطور: 244 ، 240. Metadata.cs 244

private byte[] GenerateKey()
{
    var key = new byte[keyLength];

    using (var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....))
    {
        key = deriveBytes.GetBytes(keyLength);
    }

    return key;
}

تكمن مشكلة هذا الجزء في أنه عند إدخال الدوال ، يتم دائمًا إنشاء مصفوفة من البايت ، ثم الكتابة فوقها على الفور. أولئك. هناك تخصيص مستمر للذاكرة لا معنى له.

سيكون أفضل رهان هو التبديل إلى C # 8 بدلاً من C # 5 المستخدمة وكتابة رمز أقصر:

private byte[] GenerateKey()
{
  using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
  return deriveBytes.GetBytes(keyLength);
}

لا أستطيع أن أقول ما إذا كان يمكن ترقية المشروع أم لا ، ولكن هناك العديد من هذه الأماكن. يُنصح بإعادة كتابتها بطريقة ما:

V3008 يتم تعيين قيم لمتغير "hmacKey" مرتين على التوالي. ربما هذا خطأ. فحص السطور: 256 ، 252. Metadata.cs 256
V3008 يتم تعيين قيم لمتغير "hmacHash" مرتين على التوالي. ربما هذا خطأ. فحص السطور: 270 ، 264. Metadata.cs 270
V3008 يتم تخصيص قيم لمتغير "المسارات" مرتين على التوالي. ربما هذا خطأ. فحص الأسطر: 512 ، 508. RackspaceCloudStorage.cs 512
V3008 يتم تخصيص قيم للمتغير "b" مرتين على التوالي. ربما هذا خطأ. فحص السطور: 265 ، 264. BookmarkingUserControl.ascx.cs 265
V3008 يتم تخصيص قيم لمتغير "taskIds" مرتين على التوالي. ربما هذا خطأ. فحص السطور: 412 ، 391. TaskDao.cs 412

كحل أخير ، لا تحتاج إلى تخصيص ذاكرة عند التصريح عن متغير.

علة في PVS-Studio

تعتقد أننا نكتب فقط عن أخطاء الآخرين. لا ، فريقنا ينتقد نفسه ويعترف بأخطائه ولا يتردد في الكتابة عنها أيضًا. الجميع مخطئون.

في سياق العمل على المقال ، وجدنا خطأ غبيًا إلى حد ما. نعترف ونسرع في المشاركة.

رمز من نفس خادم المجتمع:

private bool IsPhrase(string searchText)
{
    return searchText.Contains(" ") || searchText.Contains("\r\n") ||
                                       searchText.Contains("\n");
}

اضطررت إلى إعطاء تحذير كامل من المحلل قبل الكود ، كما هو الحال في جميع أنحاء المقالة ، ولكن هذه هي المشكلة. يبدو التحذير كالتالي:

لا يتم تخطي أحرف التحكم \ r و \ n قبل طباعتها على الجدول.

استنتاج

لم أجد مثل هذا المشروع المثير للاهتمام لفترة طويلة. بفضل المساهمين ONLYOFFCE. أردنا الاتصال بك ، لكن لم ترد تعليقات.

نكتب مقالات مثل هذه على أساس منتظم . هذا النوع عمره أكثر من عشر سنوات. لذلك ، لا ينبغي للمطورين أخذ النقد على محمل شخصي. يسعدنا إصدار نسخة كاملة من التقرير لتحسين المشروع أو تقديم ترخيص مؤقت للتحقق من المشروع. وليس فقط لمشروع CommunityServer ، ولكن لكل من يريده لمدة شهر واحد باستخدام الرمز الترويجي #onlyoffice .

أيضًا ، سيهتم محترفو الأمن بمعرفة أننا ندعم بنشاط معيار OWASP. بعض التشخيصات متاحة بالفعل. وقريباً سيتم تحسين واجهة المحلل لجعل إدراج معيار أو آخر لتحليل الكود أكثر ملاءمة.

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فيرجى استخدام رابط الترجمة: Svyatoslav Razmyslov. ONLYOFFICE Community Server: كيف تساهم الأخطاء في ظهور مشكلات الأمان .

خادم مجتمع ONLYOFFICE: كيف تساهم الأخطاء في مشاكل الأمان