ONLYOFFICE Community Server: как баги способствуют возникновению проблем с безопасностью

В наши обзоры ошибок программ с отрытым исходным кодом редко попадают серверные сетевые приложения. Наверное, это связано с их популярностью. Ведь мы стараемся обращать внимание на проекты, которые нам предлагают сами читатели. А серверы часто выполняют очень важные функции, но их деятельность и польза остаётся невидимой для большинства пользователей. Так, чисто случайно, был проверен код ONLYOFFICE Community Server. Получился очень весёлый обзор.

Введение

ONLYOFFICE Community Server — бесплатная система для совместной работы с открытым исходным кодом, разработанная для управления документами, проектами, взаимодействия с клиентами и электронной переписки в одном месте. На своём сайте компания подчёркивает безопасность своих решений такими фразами, как "Run your private office with the ONLYOFFICE" и "Secure office and productivity apps". Но какие-либо инструменты для контроля качества кода, видимо, в процессе разработки не используются.

Всё началось с того, что я просматривал исходники нескольких сетевых приложений в поисках вдохновения для воплощения в жизнь одной своей идеи приложения. В фоне работал анализатор PVS-Studio, и я кидал в общий корпоративный чат смешные ошибки.

Так, несколько примеров ушли в Twitter:

Позже представители прокомментировали твит, а ещё позже запостили опровержение проблемы:

Скорее всего, это действительно так. Но и очков качеству проекта это не добавляет. Давайте посмотрим, что там ещё нашлось.

"Мастер" проверки входных данных

Некоторые подходы разработчиков к проверке входных данных поражают своей оригинальностью.

Предупреждение 1

V3022 Expression 'string.IsNullOrEmpty("password")' is always false. 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;
}

Как вы заметили, этот фрагмент кода задаёт тон всей статье. Его можно описать фразой "Код смешной, а ситуация страшная". Наверное, надо сильно устать, чтобы перепутать переменную password со строкой "password". Эта ошибка позволяет продолжить выполнение кода с пустым паролем. По словам автора кода, пароль дополнительно проверяется в интерфейсе программы. Но процесс программирования устроен так, что написанные ранее функции часто переиспользуются. Поэтому эта ошибка может проявить себя где угодно в будущем. Всегда помните про важность своевременного выявления ошибок в коде.

Предупреждение 2

V3022 Expression 'String.IsNullOrEmpty("name")' is always false. SendInterceptorSkeleton.cs 36

V3022 Expression 'String.IsNullOrEmpty("sendInterceptor")' is always false. 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;
}

Внезапно в коде обнаружился целый ряд подобных ошибок. Если сначала было смешно, то сейчас стоит задуматься о причинах написания такого кода. Может это привычка осталась после перехода с другого языка программирования. В C++ часто приносят ошибки бывшие Python программисты по нашему опыту проверок C++ проектов.

Предупреждение 3

V3022 Expression 'id < 0' is always false. Unsigned type value is always >= 0. UserFolderEngine.cs 173

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

Переменная id имеет беззнаковый тип uint. Следовательно, здесь проверка бессмысленна. Стоит уделить внимание вызову этой функции. Интересно, что передают в эту функцию. Скорее всего, раньше везде использовался знаковый тип int, а после рефакторинга проверка осталась.

Copy-Paste код

Предупреждение 1

V3001 There are identical sub-expressions 'searchFilterData.WithCalendar == WithCalendar' to the left and to the right of the '&&' operator. MailSearchFilterData.cs 131

Этот фрагмент кода пришлось привести в виде картинки, чтобы передать масштаб написанного условного выражения. В нём есть проблемное место. Указание места в предупреждении анализатора с трудом помогает найти 2 одинаковых проверки. Поэтому воспользуемся красным маркером:

И вот те самые условные выражения, о которых предупредил анализатор. Кроме исправления этого места, я бы рекомендовал оформить код получше, чтобы он не способствовал появлению таких ошибок в будущем.

Предупреждение 2

V3030 Recurring check. The '!String.IsNullOrEmpty(user)' condition was already verified in line 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] == '-')
      {
        ....
}

Строка user проверяется 2 раза подряд одинаковым образом. Пожалуй, этот код можно немного отрефакторить. Хотя кто знает, может, в одном из случаев хотели проверить логическую переменную absolute.

Предупреждение 3

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

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 The 'then' statement is equivalent to the 'else' statement. 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();
    }
    ....
}

Анализатор обнаружил очень наглядный Copy-Paste код. Возможно, в одном из случаев переменную paths необходимо вычислять рекурсивно, но этого не было сделано.

Предупреждение 5

V3009 It's odd that this method always returns one and the same value of '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;
}

Размер этой функции 135 строк. Даже сами разработчики оставили комментарий, что её надо упростить. С кодом функции определённо надо поработать, т.к. он ещё и возвращает одно значение во всех случаях.

Бесполезные вызовы функций

Предупреждение 1

V3010 The return value of function 'Distinct' is required to be utilized. DbTenantService.cs 132

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

Метод Distinct удаляет дубликаты из коллекции. Но в C# большинство подобных методов-расширений не меняют объект, а создают копию. Таким образом, в этом примере список result остаётся всё тем же, что и до вызова метода. Ещё тут можно заметить имена login и passwordHash. Возможно, это очередная проблема с безопасностью.

Предупреждение 2

V3010 The return value of function 'ToString' is required to be utilized. 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);
  }
  ....
}

Метод ToString здесь стандартный. Он возвращает текстовое представление объекта, но возвращаемое значение не используется.

Предупреждение 3

V3010 The return value of function 'Replace' is required to be utilized. TextFileUserImporter.cs 252

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

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

Кто-то допустил серьёзную ошибку. Из свойства propertyField необходимо было удалить все пробелы, а этого не происходит, т.к. функция Replace не изменяет исходный объект.

Предупреждение 4

V3038 The '"yy"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. 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("'", "")
        ;
}

Здесь вызовы функций Replace написаны корректно, но в одном место это сделано со странными одинаковыми аргументами.

Потенциальный NullReferenceException

Предупреждение 1

V3022 Expression 'portalUser.BirthDate.ToString()' is always not null. The operator '??' is excessive. 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 не будет иметь значение 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 The 'r.Attributes["href"]' object was used before it was verified against null. Check lines: 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 Possible null dereference. The 'listTags.FirstOrDefault' can return default null value. FileMarker.cs 299

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 Passing 'null' to 'Equals' method should not result in '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 Expression is always true. Probably the '&&' operator should be used here. 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. Дело в том, что самое первое условное выражение содержит ошибку. Условие всегда имеет результат true. Ошибка заключается в выборе логического оператора. Надо было использовать оператор &&.

Скорее всего, этот метод ещё ни разу не вызывался, т.к. он виртуальный и до сего момента всегда переопределялся в производных классах.

Чтобы впредь не допускать таких ошибок, рекомендую почитать мою статью и сохранить на неё ссылку: "Логические выражения. Как ошибаются профессионалы". Там приведены и разобраны все ошибочные комбинации из логических операторов.

Предупреждение 2

V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. 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 component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalMinutes' value was intended instead. 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 The 'key' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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 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

Думаете, мы только пишем про ошибки других. Нет, наша команда самокритична, признаёт свои ошибки и не стесняется написать про них тоже. Все ошибаются.

По ходу работы над статьёй у нас нашёлся достаточно глупый баг. Признаём и спешим поделиться.

Код из того же Community Server:

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

Я должен был привести полное предупреждение анализатора перед кодом, как это делается во всей статье, но в этом то и проблема. Предупреждение выглядит так:

Управляющие символы r и n не экранируются перед выводом в таблицу.

Заключение

Давно мне не попадался столь интересный проект для проверки. Спасибо авторам ONLYOFFCE. Мы хотели с Вами связаться, но обратной связи не последовало.

Мы регулярно пишем подобные статьи. Этому жанру уже более десяти лет. Поэтому разработчикам не стоит принимать критику близко к сердцу. Мы будем рады выдать полную версию отчёта для улучшения проекта или предоставить временную лицензию для проверки проекта. Причём не только проекту CommunityServer, а всем желающим и на ОДИН МЕСЯЦ по промокоду #onlyoffice.

Также специалистам по безопасности будет интересно узнать, что мы ведём активную поддержку стандарта OWASP. Некоторые диагностики уже доступны. А скоро будет доработан интерфейс анализатора, чтобы включение того или иного стандарта для анализа кода стало ещё удобнее.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. ONLYOFFICE Community Server: how bugs contribute to the emergence of security problems.

Автор: Святослав

Источник