Server Komunitas ONLYOFFICE: Bagaimana Bug Berkontribusi pada Masalah Keamanan

gambar1.png


Aplikasi jaringan sisi server jarang muncul dalam laporan bug open source kami. Ini mungkin karena popularitas mereka. Bagaimanapun, kami mencoba memperhatikan proyek yang ditawarkan pembaca sendiri kepada kami. Dan server sering kali menjalankan fungsi yang sangat penting, tetapi aktivitas dan manfaatnya tetap tidak terlihat oleh sebagian besar pengguna. Jadi, hanya kebetulan, kode Server Komunitas ONLYOFFICE telah diperiksa. Ternyata review itu sangat lucu.



pengantar



ONLYOFFICE Community Server adalah sistem kolaborasi open source gratis yang dirancang untuk mengelola dokumen, proyek, interaksi pelanggan, dan komunikasi email di satu tempat. Di situs webnya, perusahaan menekankan keamanan solusinya dengan frasa seperti "Jalankan kantor pribadi Anda dengan ONLYOFFICE" dan "Amankan aplikasi kantor dan produktivitas". Namun, tampaknya, tidak ada alat untuk kontrol kualitas kode yang digunakan dalam proses pengembangan.



Semuanya dimulai dengan fakta bahwa saya melihat-lihat kode sumber dari beberapa aplikasi jaringan untuk mencari inspirasi untuk implementasi salah satu ide aplikasi saya. Penganalisis PVS-Studio berjalan di latar belakang dan saya melemparkan kesalahan lucu ke dalam obrolan korporat umum.



Jadi, beberapa contoh masuk ke Twitter :



gambar2.png


Perwakilan kemudian mengomentari tweet tersebut, dan bahkan kemudian memposting penolakan masalah:



image3.png


Ini kemungkinan besar benar. Tapi ini tidak menambah poin pada kualitas proyek. Mari kita lihat apa lagi yang ditemukan di sana.



Input Validation Wizard



image4.png


Beberapa pendekatan pengembang untuk memvalidasi data masukan sangat mencolok dalam orisinalitasnya.



Peringatan 1



V3022 Expression 'string.IsNullOrEmpty ("password")' selalu salah. SmtpSettings.cs 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;
}
      
      





Seperti yang Anda lihat, potongan kode ini menentukan nada untuk keseluruhan artikel. Ini dapat dijelaskan dengan frasa "Kode itu lucu, tetapi situasinya buruk." Anda mungkin harus sangat lelah untuk mengacaukan variabel kata sandi dengan string "kata sandi" . Kesalahan ini memungkinkan kode untuk terus dijalankan dengan kata sandi kosong. Menurut pembuat kode, kata sandi juga diperiksa di antarmuka program. Tetapi proses pemrograman dirancang sedemikian rupa sehingga fungsi yang ditulis sebelumnya sering digunakan kembali. Oleh karena itu, kesalahan ini dapat muncul di mana saja di masa mendatang. Selalu ingat pentingnya menemukan bug dalam kode Anda sejak awal.



Peringatan 2 Ekspresi



V3022 'String.IsNullOrEmpty ("name")' selalu salah. SendInterceptorSkeleton. cs 36



V3022 Expression 'String.IsNullOrEmpty ("sendInterceptor")' selalu salah. SendInterceptorSkeleton.cs 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;
}
      
      





Tiba-tiba, ada sejumlah kesalahan serupa di kode. Jika pada awalnya itu lucu, sekarang ada baiknya memikirkan alasan penulisan kode semacam itu. Mungkin kebiasaan ini tetap ada setelah berpindah dari bahasa pemrograman lain. Dalam C ++, kesalahan sering kali dibawa oleh mantan programmer Python dalam pengalaman kami memeriksa proyek C ++.



Peringatan 3



V3022 Ekspresi 'id <0' selalu salah. Nilai tipe unsigned selalu> = 0. UserFolderEngine.cs 173



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





Variabel id berjenis uint unsigned . Oleh karena itu, pemeriksaan tidak ada gunanya di sini. Perlu diperhatikan untuk memanggil fungsi ini. Saya ingin tahu apa yang diteruskan ke fungsi ini. Kemungkinan besar, tipe int bertanda tangan digunakan di mana-mana , tetapi setelah refactoring, cek tetap ada.



Salin-Tempel kode



image5.png


Peringatan 1



V3001 Ada sub-ekspresi identik 'searchFilterData.DenganCalendar == WithCalendar' di kiri dan kanan operator '&&'. MailSearchFilterData.cs 131



image6.png


Potongan kode ini harus disajikan sebagai gambar untuk menyampaikan skala ekspresi bersyarat tertulis. Ada area masalah di dalamnya. Menentukan tempat dalam peringatan penganalisis tidak dapat membantu Anda menemukan 2 pemeriksaan yang identik. Oleh karena itu, kami akan menggunakan penanda merah:



image7.png


Dan berikut adalah persyaratan yang sama yang diperingatkan oleh penganalisis. Selain memperbaikinya, saya akan merekomendasikan agar Anda menata kode dengan lebih baik sehingga tidak berkontribusi pada munculnya kesalahan seperti itu di masa mendatang.



Peringatan 2



V3030 Pemeriksaan berulang. Kondisi '! String.IsNullOrEmpty (user)' telah diverifikasi di baris 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] == '-')
      {
        ....
}
      
      





String pengguna diperiksa 2 kali berturut-turut dengan cara yang sama. Mungkin kode ini bisa sedikit direfraktorisasi. Meskipun siapa tahu, mungkin dalam salah satu kasus mereka ingin memeriksa variabel boolean absolute .



Peringatan 3



V3021 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'if' pertama berisi pengembalian metode. Ini berarti bahwa pernyataan 'jika' kedua adalah WikiEngine.cs 688 yang tidak masuk akal



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(":"))
    {
      ....
    }
    ....
}
      
      





Saya yakin Anda tidak dapat menemukan kesalahan di sini dengan mata Anda. Penganalisis menemukan cek yang tidak berguna, yang ternyata adalah kode yang disalin dari atas. Alih-alih variabel str , variabel sLink harus diperiksa .



Peringatan 4



V3004 Pernyataan 'then' sama dengan pernyataan '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();
    }
    ....
}
      
      





Penganalisis telah mendeteksi kode Salin-Tempel yang sangat deskriptif. Mungkin, dalam satu kasus, variabel jalur perlu dihitung secara rekursif, tetapi ini belum dilakukan.



Peringatan 5



V3009 Aneh bahwa metode ini selalu mengembalikan satu dan nilai yang sama dari 'true'. MessageEngine.cs 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;
}
      
      





Ukuran fungsi ini adalah 135 baris. Bahkan para pengembang sendiri meninggalkan komentar bahwa itu harus disederhanakan. Anda pasti perlu bekerja dengan kode fungsi, karena itu juga mengembalikan satu nilai dalam semua kasus.



Panggilan fungsi yang tidak berguna



image8.png


Peringatan 1



V3010 Nilai kembali dari fungsi 'Distinct' diperlukan untuk digunakan. DbTenantService.cs 132



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





Metode Distinct menghapus duplikat dari koleksi. Tetapi di C #, sebagian besar metode ekstensi ini tidak mengubah objek, tetapi membuat salinan. Jadi, dalam contoh ini, daftar hasil tetap sama seperti sebelum pemanggilan metode. Anda juga bisa melihat nama login dan passwordHash di sini . Mungkin ini masalah keamanan lainnya.



Peringatan 2



V3010 Nilai kembali dari fungsi 'ToString' diperlukan untuk digunakan. UserPhotoManager.cs 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);
  }
  ....
}
      
      





Metode ToString adalah standar di sini. Ini mengembalikan representasi tekstual dari objek, tetapi nilai yang dikembalikan tidak digunakan.



Peringatan 3



V3010 Nilai kembali dari fungsi 'Ganti' diperlukan untuk digunakan. TextFileUserImporter.cs 252



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

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





Seseorang melakukan kesalahan serius. Semua spasi harus dihapus dari properti propertyField , tetapi ini tidak terjadi, sejak itu fungsi Replace tidak mengubah objek aslinya.



Peringatan 4



V3038 Argumen '"yy"' dikirimkan ke metode 'Replace' beberapa kali. Ada kemungkinan bahwa argumen lain harus diteruskan. 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("'", "")
        ;
}
      
      





Di sini panggilan ke fungsi Replace ditulis dengan benar, tetapi di satu tempat dilakukan dengan argumen identik yang aneh.



Potensi NullReferenceException



image9.png


Peringatan 1



V3022 Expression 'portalUser.BirthDate.ToString ()' selalu tidak null. Operator '??' berlebihan. LdapUserManager.cs 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 tidak akan kosong . Pemeriksaan dilakukan di sini untuk mengeluarkan nilai "NULL" ke log debug jika tanggal tidak disetel. Tapi sejak jika tidak ada nilai, metode ToString akan mengembalikan string kosong, lalu kesalahan dalam algoritme mungkin kurang terlihat di log.



Seluruh daftar tempat penebangan yang dipertanyakan terlihat seperti ini:



  • V3022 Ekspresi 'ldapUser.BirthDate.ToString ()' selalu tidak null. Operator '??' berlebihan. LdapUserManager.cs 437
  • V3022 Expression 'portalUser.Sex.ToString ()' selalu tidak null. Operator '??' berlebihan. LdapUserManager.cs 444
  • V3022 Ekspresi 'ldapUser.Sex.ToString ()' selalu tidak null. Operator '??' berlebihan. LdapUserManager.cs 445


Peringatan 2



V3095 Objek 'r.Attributes ["href"]' digunakan sebelum diverifikasi terhadap null. Periksa baris: 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");
    }))
    {
      ....
    }
    ....
}
      
      





Saat mengurai Html atau Xml, sangat berbahaya untuk merujuk ke atribut berdasarkan nama tanpa validasi. Bug ini sangat menarik karena nilai atribut href pertama - tama diambil dan kemudian diperiksa untuk melihat apakah ada atau tidak.



Peringatan 3



V3146 Kemungkinan dereferensi nol. 'ListTags.FirstOrDefault' dapat mengembalikan nilai null default. FileMarker.cs 299



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





Penganalisis mendeteksi penggunaan yang tidak aman dari hasil pemanggilan metode FirstOrDefault . Metode ini mengembalikan nilai default jika tidak ada objek dalam daftar yang memenuhi predikat pencarian. Default untuk tipe referensi adalah referensi null. Oleh karena itu, sebelum menggunakan tautan yang dihasilkan, itu harus diperiksa, dan tidak langsung disebut properti, seperti di sini.



Peringatan 4



V3115 Meneruskan 'null' ke metode 'Equals' seharusnya tidak menghasilkan '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);
    }
    ....
}
      
      





Referensi objek di C # sering dibandingkan dengan null . Oleh karena itu, ketika membebani metode perbandingan, sangat penting untuk mengantisipasi situasi seperti itu dan menambahkan pemeriksaan yang sesuai di awal fungsi. Tapi di sini mereka tidak melakukannya.



Bug lainnya



image10.png


Peringatan 1 Ekspresi



V3022 selalu benar. Mungkin operator '&&' harus digunakan di sini. 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();
    ....
}
      
      





Memanggil metode CreateItem akan memunculkan ArgumentException . Intinya adalah ekspresi kondisional pertama berisi kesalahan. Kondisi selalu bernilai true . Kesalahannya terletak pada pilihan operator logika. Anda seharusnya menggunakan operator &&.



Kemungkinan besar, metode ini belum pernah dipanggil. itu virtual dan selalu didefinisikan ulang di kelas turunan sampai sekarang.



Untuk menghindari kesalahan seperti itu di masa mendatang, saya sarankan membaca artikel saya dan menyimpan tautan ke sana: " Ekspresi logis. Bagaimana profesional membuat kesalahan". Semua kombinasi operator logika yang salah diberikan dan dianalisis.



Peringatan 2



V3052 Objek pengecualian asli 'ex' telah ditelan. Tumpukan pengecualian asli dapat hilang. 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;
    }
}
      
      





Di sini kami telah mengubah GoogleApiException menjadi SecurityException , sementara kehilangan informasi berguna dari pengecualian aslinya.



Perubahan kecil seperti ini akan membuat peringatan yang dihasilkan lebih informatif:



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





Meskipun sangat mungkin GoogleApiException disembunyikan dengan sengaja .



Peringatan 3



V3118 Menit komponen TimeSpan digunakan, yang tidak mewakili interval waktu penuh. Mungkin nilai 'TotalMinutes' yang dimaksudkan sebagai gantinya. NotifyClient.cs 281



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

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





Saya dulu berpikir bahwa diagnosa itu preventif. Dalam kode proyek saya, itu selalu memberikan peringatan palsu. Di sini, saya cukup yakin ada bug. Kemungkinan besar, Anda harus menggunakan properti TotalMinutes daripada Minutes .



Peringatan 4



V3008 Variabel 'key' diberi nilai dua kali berturut-turut. Mungkin ini salah. Baris centang: 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;
}
      
      





Masalah dengan fragmen ini adalah ketika memasuki suatu fungsi, array byte selalu dibuat, dan kemudian segera ditimpa. Itu. ada alokasi memori konstan yang tidak masuk akal.



Taruhan terbaik Anda adalah beralih ke C # 8 daripada C # 5 yang digunakan dan menulis kode yang lebih pendek:



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





Saya tidak bisa mengatakan apakah proyek dapat ditingkatkan atau tidak, tetapi ada banyak tempat seperti itu. Disarankan untuk menulis ulang entah bagaimana:



  • V3008 Variabel 'hmacKey' diberi nilai dua kali berturut-turut. Mungkin ini salah. Baris centang: 256, 252. Metadata.cs 256
  • V3008 Variabel 'hmacHash' diberi nilai dua kali berturut-turut. Mungkin ini salah. Baris centang: 270, 264. Metadata.cs 270
  • V3008 Variabel 'jalur' diberi nilai dua kali berturut-turut. Mungkin ini salah. Periksa baris: 512, 508. RackspaceCloudStorage.cs 512
  • V3008 Variabel 'b' diberi nilai dua kali berturut-turut. Mungkin ini salah. Periksa baris: 265, 264. BookmarkingUserControl.ascx.cs 265
  • V3008 Variabel 'taskIds' diberi nilai dua kali berturut-turut. Mungkin ini salah. Periksa baris: 412, 391. TaskDao.cs 412


Sebagai upaya terakhir, Anda tidak perlu mengalokasikan memori saat mendeklarasikan variabel.



Bug di PVS-Studio



image11.png


Anda pikir kami hanya menulis tentang kesalahan orang lain. Tidak, tim kami kritis terhadap diri sendiri, mengakui kesalahannya dan tidak ragu untuk menuliskannya juga. Setiap orang salah.



Dalam proses pengerjaan artikel, kami menemukan bug yang agak bodoh. Kami mengakui dan bergegas untuk berbagi.



Kode dari Server Komunitas yang sama:



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





Saya harus memberikan peringatan penganalisis penuh sebelum kode, seperti yang dilakukan di seluruh artikel, tetapi inilah masalahnya. Peringatannya terlihat seperti ini:



image12.png


Karakter kontrol \ r dan \ n tidak di-escape sebelum dicetak ke tabel.



Kesimpulan



image13.png


Saya belum pernah menemukan proyek yang begitu menarik untuk waktu yang lama. Terima kasih kepada kontributor ONLYOFFCE. Kami ingin menghubungi Anda, tetapi tidak ada tanggapan.



Kami menulis artikel seperti ini secara teratur . Genre ini berusia lebih dari sepuluh tahun. Oleh karena itu, pengembang tidak boleh menerima kritik secara pribadi. Kami akan dengan senang hati menerbitkan versi lengkap dari laporan untuk meningkatkan proyek atau memberikan lisensi sementara untuk memeriksa proyek. Dan tidak hanya proyek CommunityServer dan semua pendatang, dan selama satu bulan di kode promosi #onlyoffice .



image14.png


Selain itu, profesional keamanan akan tertarik untuk mengetahui bahwa kami secara aktif mendukung standar OWASP. Beberapa diagnostik sudah tersedia. Dan segera antarmuka penganalisis akan ditingkatkan untuk membuat penyertaan satu atau beberapa standar lain untuk analisis kode menjadi lebih nyaman.





Jika Anda ingin berbagi artikel ini dengan audiens berbahasa Inggris, silakan gunakan tautan terjemahan: Svyatoslav Razmyslov. ONLYOFFICE Community Server: bagaimana bug berkontribusi pada munculnya masalah keamanan .



All Articles