# ONLYOFFICE Community Server: как баги способствуют возникновению проблем с безопасностью В наши обзоры ошибок программ с отрытым исходным кодом редко попадают серверные сетевые приложения\. Наверное, это связано с их популярностью\. Ведь мы стараемся обращать внимание на проекты, которые нам предлагают сами читатели\. А серверы часто выполняют очень важные функции, но их деятельность и польза остаётся невидимой для большинства пользователей\. Так, чисто случайно, был проверен код ONLYOFFICE Community Server\. Получился очень весёлый обзор\. ![0783_ONLYOFFICE_CommunityServer_ru/image1.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image1.png) ## Введение [ONLYOFFICE Community Server](https://github.com/ONLYOFFICE/CommunityServer) \- бесплатная система для совместной работы с открытым исходным кодом, разработанная для управления документами, проектами, взаимодействия с клиентами и электронной переписки в одном месте\. На своём [сайте](https://www.onlyoffice.com/) компания подчёркивает безопасность своих решений такими фразами, как "Run your private office with the ONLYOFFICE" и "Secure office and productivity apps"\. Но какие\-либо инструменты для контроля качества кода, видимо, в процессе разработки не используются\. Всё началось с того, что я просматривал исходники нескольких сетевых приложений в поисках вдохновения для воплощения в жизнь одной своей идеи приложения\. В фоне работал анализатор [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/), и я кидал в общий корпоративный чат смешные ошибки\. Так, несколько примеров ушли в [Twitter](https://twitter.com/Code_Analysis): ![0783_ONLYOFFICE_CommunityServer_ru/image2.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image2.png) Позже представители прокомментировали твит, а ещё позже запостили опровержение проблемы: ![0783_ONLYOFFICE_CommunityServer_ru/image3.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image3.png) Скорее всего, это действительно так\. Но и очков качеству проекта это не добавляет\. Давайте посмотрим, что там ещё нашлось\. ## "Мастер" проверки входных данных ![0783_ONLYOFFICE_CommunityServer_ru/image4.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image4.png) Некоторые подходы разработчиков к проверке входных данных поражают своей оригинальностью\. **Предупреждение 1** [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\("password"\)' is always false\. SmtpSettings\.cs 104 ```cpp 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; } ``` Как вы заметили, этот фрагмент кода задаёт тон всей статье\. Его можно описать фразой "Код смешной, а ситуация страшная"\. Наверное, надо сильно устать, чтобы перепутать переменную _password_ со строкой _"password"_\. Эта ошибка позволяет продолжить выполнение кода с пустым паролем\. По словам автора кода, пароль дополнительно проверяется в интерфейсе программы\. Но процесс программирования устроен так, что написанные ранее функции часто переиспользуются\. Поэтому эта ошибка может проявить себя где угодно в будущем\. Всегда помните про важность своевременного выявления ошибок в коде\. **Предупреждение 2** [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'String\.IsNullOrEmpty\("name"\)' is always false\. SendInterceptorSkeleton\.cs 36 [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'String\.IsNullOrEmpty\("sendInterceptor"\)' is always false\. SendInterceptorSkeleton\.cs 37 ```cpp public SendInterceptorSkeleton( string name, ...., Func 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'id < 0' is always false\. Unsigned type value is always \>\= 0\. UserFolderEngine\.cs 173 ```cpp public MailUserFolderData Update(uint id, string name, uint? parentId = null) { if (id < 0) throw new ArgumentException("id"); .... } ``` Переменная _id_ имеет беззнаковый тип _uint_\. Следовательно, здесь проверка бессмысленна\. Стоит уделить внимание вызову этой функции\. Интересно, что передают в эту функцию\. Скорее всего, раньше везде использовался знаковый тип _int_, а после рефакторинга проверка осталась\. ## Copy\-Paste код ![0783_ONLYOFFICE_CommunityServer_ru/image5.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image5.png) **Предупреждение 1** [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'searchFilterData\.WithCalendar \=\= WithCalendar' to the left and to the right of the '&&' operator\. MailSearchFilterData\.cs 131 ![0783_ONLYOFFICE_CommunityServer_ru/image6.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image6.png) Этот фрагмент кода пришлось привести в виде картинки, чтобы передать масштаб написанного условного выражения\. В нём есть проблемное место\. Указание места в предупреждении анализатора с трудом помогает найти 2 одинаковых проверки\. Поэтому воспользуемся красным маркером: ![0783_ONLYOFFICE_CommunityServer_ru/image7.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image7.png) И вот те самые условные выражения, о которых предупредил анализатор\. Кроме исправления этого места, я бы рекомендовал оформить код получше, чтобы он не способствовал появлению таких ошибок в будущем\. **Предупреждение 2** [V3030](https://pvs-studio.ru/ru/docs/warnings/v3030/) Recurring check\. The '\!String\.IsNullOrEmpty\(user\)' condition was already verified in line 173\. CommonLinkUtility\.cs 176 ```cpp 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] == '-') { .... } ``` Строка _user_ проверяется 2 раза подряд одинаковым образом\. Пожалуй, этот код можно немного отрефакторить\. Хотя кто знает, может, в одном из случаев хотели проверить логическую переменную _absolute_\. **Предупреждение 3** [V3021](https://pvs-studio.ru/ru/docs/warnings/v3021/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless WikiEngine\.cs 688 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. SelectelStorage\.cs 461 ```cpp public override string[] ListFilesRelative(....) { var paths = new List(); 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(); } .... } ``` Анализатор обнаружил очень наглядный Copy\-Paste код\. Возможно, в одном из случаев переменную _paths_ необходимо вычислять рекурсивно, но этого не было сделано\. **Предупреждение 5** [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. MessageEngine\.cs 318 ```cpp //TODO: Simplify public bool SetUnread(List 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 строк\. Даже сами разработчики оставили комментарий, что её надо упростить\. С кодом функции определённо надо поработать, т\.к\. он ещё и возвращает одно значение во всех случаях\. ## Бесполезные вызовы функций ![0783_ONLYOFFICE_CommunityServer_ru/image8.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image8.png) **Предупреждение 1** [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Distinct' is required to be utilized\. DbTenantService\.cs 132 ```cpp public IEnumerable GetTenants(string login, string passwordHash) { //new password result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList(); result.Distinct(); .... } ``` Метод _Distinct_ удаляет дубликаты из коллекции\. Но в C\# большинство подобных методов\-расширений не меняют объект, а создают копию\. Таким образом, в этом примере список _result_ остаётся всё тем же, что и до вызова метода\. Ещё тут можно заметить имена _login_ и _passwordHash_\. Возможно, это очередная проблема с безопасностью\. **Предупреждение 2** [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'ToString' is required to be utilized\. UserPhotoManager\.cs 678 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Replace' is required to be utilized\. TextFileUserImporter\.cs 252 ```cpp private int GetFieldsMapping(....) { .... if (NameMapping != null && NameMapping.ContainsKey(propertyField)) { propertyField = NameMapping[propertyField]; } propertyField.Replace(" ", ""); .... } ``` Кто\-то допустил серьёзную ошибку\. Из свойства _propertyField_ необходимо было удалить все пробелы, а этого не происходит, т\.к\. функция _Replace_ не изменяет исходный объект\. **Предупреждение 4** [V3038](https://pvs-studio.ru/ru/docs/warnings/v3038/) The '"yy"' argument was passed to 'Replace' method several times\. It is possible that other argument should be passed instead\. MasterLocalizationResources\.cs 38 ```cpp 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("'", "") ; } ``` Здесь вызовы функций _Replace_ написаны корректно, но в одном место это сделано со странными одинаковыми аргументами\. ## Потенциальный NullReferenceException ![0783_ONLYOFFICE_CommunityServer_ru/image9.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image9.png) **Предупреждение 1** [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'portalUser\.BirthDate\.ToString\(\)' is always not null\. The operator '??' is excessive\. LdapUserManager\.cs 436 ```cpp 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_\. Проверка здесь делалась для того, чтобы в отладочный лог вывести значение "NULL", если дата не задана\. Но т\.к\. в случае отсутствия значения метод _ToString_ вернёт пустую строку, то ошибка в алгоритме может быть менее заметна в логах\. Весь список сомнительных мест логгирования выглядит так: * V3022 Expression 'ldapUser\.BirthDate\.ToString\(\)' is always not null\. The operator '??' is excessive\. LdapUserManager\.cs 437 * V3022 Expression 'portalUser\.Sex\.ToString\(\)' is always not null\. The operator '??' is excessive\. LdapUserManager\.cs 444 * V3022 Expression 'ldapUser\.Sex\.ToString\(\)' is always not null\. The operator '??' is excessive\. LdapUserManager\.cs 445 **Предупреждение 2** [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'r\.Attributes\["href"\]' object was used before it was verified against null\. Check lines: 86, 87\. HelpCenterStorage\.cs 86 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3146/) Possible null dereference\. The 'listTags\.FirstOrDefault' can return default null value\. FileMarker\.cs 299 ```cpp public static void RemoveMarkAsNew(....) { .... var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList(); valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count; .... } ``` Анализатор обнаружил небезопасное использование результата вызова метода _FirstOrDefault_\. Этот метод возвращают значение по умолчанию, если в списке нет ни одного объекта, удовлетворяющего предикату поиска\. Значением по умолчанию для ссылочных типов является пустая ссылка \(null\)\. Соответственно, прежде чем использовать полученную ссылку, её следует проверить, а не звать сразу свойство, как здесь\. **Предупреждение 4** [V3115](https://pvs-studio.ru/ru/docs/warnings/v3115/) Passing 'null' to 'Equals' method should not result in 'NullReferenceException'\. ResCulture\.cs 28 ```cpp 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_\. Следовательно, при перегрузке методов сравнения очень важно предусмотреть такие ситуации и добавить соответствующую проверку в начало функции\. А здесь этого не сделали\. ## Другие баги ![0783_ONLYOFFICE_CommunityServer_ru/image10.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image10.png) **Предупреждение 1** [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression is always true\. Probably the '&&' operator should be used here\. ListItemHistoryDao\.cs 140 ```cpp 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_\. Дело в том, что самое первое условное выражение содержит ошибку\. Условие всегда имеет результат _true_\. Ошибка заключается в выборе логического оператора\. Надо было использовать оператор &&\. Скорее всего, этот метод ещё ни разу не вызывался, т\.к\. он виртуальный и до сего момента всегда переопределялся в производных классах\. Чтобы впредь не допускать таких ошибок, рекомендую почитать мою статью и сохранить на неё ссылку: "[Логические выражения\. Как ошибаются профессионалы](https://pvs-studio.ru/ru/blog/posts/cpp/0390/)"\. Там приведены и разобраны все ошибочные комбинации из логических операторов\. **Предупреждение 2** [V3052](https://pvs-studio.ru/ru/docs/warnings/v3052/) The original exception object 'ex' was swallowed\. Stack of original exception could be lost\. GoogleDriveStorage\.cs 267 ```cpp 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_, потеряв при этом информацию из исходного исключения, которая может быть полезной\. Вот такое небольшое изменение сделает сгенерированное предупреждение более информативным: ```cpp throw new SecurityException(ex.Error.Message, ex); ``` Хотя вполне возможно, что исключение _GoogleApiException_ скрыли намерено\. **Предупреждение 3** [V3118](https://pvs-studio.ru/ru/docs/warnings/v3118/) Minutes component of TimeSpan is used, which does not represent full time interval\. Possibly 'TotalMinutes' value was intended instead\. NotifyClient\.cs 281 ```cpp public static void SendAutoReminderAboutTask(DateTime scheduleDate) { .... var deadlineReminderDate = deadline.AddMinutes(-alertValue); if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue; .... } ``` Раньше я считал, что диагностика имеет предупредительный характер\. В коде моих проектов она всегда выдавала ложные предупреждения\. Здесь же я почти уверен, обнаружилась ошибка\. Скорее всего, надо использовать именно свойство _TotalMinutes_, вместо _Minutes_\. **Предупреждение 4** [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'key' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 244, 240\. Metadata\.cs 244 ```cpp 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 и написать более короткий код: ```cpp private byte[] GenerateKey() { using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....); return deriveBytes.GetBytes(keyLength); } ``` Не могу сказать, возможен ли апгрейд проекта или нет, но таких мест не мало\. Их желательно переписать как\-нибудь: * V3008 The 'hmacKey' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 256, 252\. Metadata\.cs 256 * V3008 The 'hmacHash' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 270, 264\. Metadata\.cs 270 * V3008 The 'paths' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 512, 508\. RackspaceCloudStorage\.cs 512 * V3008 The 'b' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 265, 264\. BookmarkingUserControl\.ascx\.cs 265 * V3008 The 'taskIds' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 412, 391\. TaskDao\.cs 412 На крайний случай, можно не выделять память при объявлении переменной\. ## Баг в PVS\-Studio ![0783_ONLYOFFICE_CommunityServer_ru/image11.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image11.png) Думаете, мы только пишем про ошибки других\. Нет, наша команда самокритична, признаёт свои ошибки и не стесняется написать про них тоже\. Все ошибаются\. По ходу работы над статьёй у нас нашёлся достаточно глупый баг\. Признаём и спешим поделиться\. Код из того же Community Server: ```cpp private bool IsPhrase(string searchText) { return searchText.Contains(" ") || searchText.Contains("\r\n") || searchText.Contains("\n"); } ``` Я должен был привести полное предупреждение анализатора перед кодом, как это делается во всей статье, но в этом то и проблема\. Предупреждение выглядит так: ![0783_ONLYOFFICE_CommunityServer_ru/image12.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image12.png) Управляющие символы \\r и \\n не экранируются перед выводом в таблицу\. ## Заключение ![0783_ONLYOFFICE_CommunityServer_ru/image13.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image13.png) Давно мне не попадался столь интересный проект для проверки\. Спасибо авторам ONLYOFFCE\. Мы хотели с Вами связаться, но обратной связи не последовало\. Мы регулярно пишем подобные [статьи](https://pvs-studio.ru/ru/blog/inspections/)\. Этому жанру уже более десяти лет\. Поэтому разработчикам не стоит принимать критику близко к сердцу\. Мы будем рады выдать полную версию отчёта для улучшения проекта или предоставить временную лицензию для проверки проекта\. Причём не только проекту CommunityServer, а всем желающим и на ОДИН МЕСЯЦ по промокоду [\#onlyoffice](https://pvs-studio.ru/ru/pvs-studio/download/?promo=onlyoffice?promo=onlyoffice)\. ![0783_ONLYOFFICE_CommunityServer_ru/image14.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer_ru/image14.png) Также специалистам по безопасности будет интересно узнать, что мы ведём активную поддержку стандарта OWASP\. Некоторые диагностики уже доступны\. А скоро будет доработан интерфейс анализатора, чтобы включение того или иного стандарта для анализа кода стало ещё удобнее\.