ONLYOFFICE Community Server: como bugs contribuem para problemas de segurança

image1.png


Os aplicativos de rede do lado do servidor raramente aparecem em nossos relatórios de bug de código aberto. Isso provavelmente se deve à sua popularidade. Afinal, procuramos estar atentos aos projetos que os próprios leitores nos oferecem. E os servidores geralmente executam funções muito importantes, mas suas atividades e benefícios permanecem invisíveis para a maioria dos usuários. Portanto, por acaso, o código do ONLYOFFICE Community Server foi verificado. Acabou sendo uma crítica muito engraçada.



Introdução



O ONLYOFFICE Community Server é um sistema de colaboração de código aberto gratuito projetado para gerenciar documentos, projetos, interações com clientes e comunicações por email em um só lugar. Em seu site, a empresa enfatiza a segurança de suas soluções com frases como "Administre seu escritório particular com o ONLYOFFICE" e "Segurança de escritório e aplicativos de produtividade". Mas, aparentemente, nenhuma ferramenta de controle de qualidade do código é utilizada no processo de desenvolvimento.



Tudo começou com o fato de que eu procurei no código-fonte de vários aplicativos de rede em busca de inspiração para a implementação de uma das minhas ideias de aplicativo. O analisador PVS-Studio estava sendo executado em segundo plano e eu joguei erros engraçados no chat corporativo geral.



Então, alguns exemplos foram para o Twitter :



image2.png


Mais tarde, os representantes comentaram sobre o tweet e, ainda mais tarde, postaram uma negação do problema:



image3.png


Provavelmente, isso é verdade. Mas isso não agrega pontos à qualidade do projeto. Vamos ver o que mais foi encontrado lá.



Assistente de validação de entrada



image4.png


Algumas das abordagens do desenvolvedor para validar os dados de entrada são impressionantes em sua originalidade.



Aviso 1



A expressão V3022 'string.IsNullOrEmpty ("senha")' é sempre falsa. 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;
}
      
      





Como você pode ver, esse trecho de código dá o tom para todo o artigo. Ele pode ser descrito com a frase "O código é engraçado, mas a situação é terrível." Você provavelmente terá que ficar muito cansado para confundir a variável de senha com a string "senha" . Este erro permite que você continue executando o código com uma senha vazia. De acordo com o autor do código, a senha é verificada adicionalmente na interface do programa. Mas o processo de programação é projetado de tal forma que as funções escritas anteriormente são frequentemente reutilizadas. Portanto, esse erro pode se manifestar em qualquer lugar no futuro. Lembre-se sempre da importância de detectar os bugs no seu código o quanto antes.



Aviso 2



A expressão V3022 'String.IsNullOrEmpty ("nome")' é sempre falsa. SendInterceptorSkeleton. cs 36



A expressão V3022 'String.IsNullOrEmpty ("sendInterceptor")' é sempre falsa. 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;
}
      
      





De repente, ocorreram vários erros semelhantes no código. Se no começo era engraçado, agora vale a pena pensar nos motivos para escrever esse código. Talvez esse hábito permaneceu após a mudança de outra linguagem de programação. Em C ++, os erros geralmente são trazidos por ex-programadores de Python em nossa experiência de verificação de projetos C ++.



Aviso 3



A expressão V3022 'id <0' é sempre falsa. O valor do tipo sem sinal é sempre> = 0. UserFolderEngine.cs 173



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





A variável id é do tipo uint sem sinal . Portanto, verificar é inútil aqui. Vale a pena prestar atenção ao chamar esta função. Gostaria de saber o que está sendo passado para esta função. Provavelmente, o tipo assinado int foi usado em todos os lugares , mas após a refatoração a verificação permaneceu.



Código Copiar-Colar



image5.png


Aviso 1



V3001 Existem subexpressões idênticas 'searchFilterData.WithCalendar == WithCalendar' à esquerda e à direita do operador '&&'. MailSearchFilterData.cs 131



image6.png


Este trecho de código teve que ser mostrado como uma imagem para transmitir a escala da expressão condicional escrita. Existe uma área problemática nisso. Especificar um local no aviso do analisador dificilmente pode ajudá-lo a encontrar 2 verificações idênticas. Portanto, usaremos o marcador vermelho:



image7.png


E aqui estão as mesmas condicionais sobre as quais o analisador alertou. Além de corrigir esse ponto, eu recomendo que você estilize melhor o código para que ele não contribua com esses erros no futuro.



Aviso 2



V3030 Verificação recorrente. A condição '! String.IsNullOrEmpty (usuário)' já foi verificada na linha 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] == '-')
      {
        ....
}
      
      





A string do usuário é verificada 2 vezes seguidas da mesma maneira. Talvez este código possa ser um pouco refatorado. Embora quem sabe, talvez em um dos casos eles quisessem verificar a variável booleana absoluta .



Aviso 3



V3021 Existem duas declarações 'if' com expressões condicionais idênticas. A primeira instrução 'if' contém o retorno do método. Isso significa que a segunda declaração 'se' não tem sentido. 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(":"))
    {
      ....
    }
    ....
}
      
      





Tenho certeza de que você não pode encontrar um erro aqui com seus olhos. O analisador encontrou um cheque inútil, que era o código copiado de cima. Em vez da variável str , a variável sLink deve ser verificada .



Aviso 4



V3004 A instrução 'then' é equivalente à instrução '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();
    }
    ....
}
      
      





O analisador detectou um código Copiar-Colar muito descritivo. Talvez, em um caso, a variável de caminhos precise ser calculada recursivamente, mas isso não foi feito.



Aviso 5



V3009 É estranho que este método sempre retorne um e o mesmo valor de 'verdadeiro'. 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;
}
      
      





O tamanho desta função é 135 linhas. Até os próprios desenvolvedores deixaram um comentário que deveria ser simplificado. Você definitivamente precisa trabalhar com o código da função, porque ele também retorna um valor em todos os casos.



Chamadas de função inúteis



image8.png


Aviso 1



V3010 O valor de retorno da função 'Distinto' deve ser utilizado. DbTenantService.cs 132



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





O método Distinct remove duplicatas da coleção. Mas em C #, a maioria desses métodos de extensão não modifica o objeto, mas cria uma cópia. Portanto, neste exemplo, a lista de resultados permanece a mesma de antes da chamada do método. Você também pode ver os nomes de login e senhaHash aqui . Talvez este seja outro problema de segurança.



Aviso 2



V3010 O valor de retorno da função 'ToString' deve ser utilizado. 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);
  }
  ....
}
      
      





O método ToString é padrão aqui. Ele retorna uma representação textual do objeto, mas o valor de retorno não é usado.



Aviso 3



V3010 O valor de retorno da função 'Substituir' deve ser utilizado. TextFileUserImporter.cs 252



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

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





Alguém cometeu um erro grave. Todos os espaços tiveram que ser removidos da propriedade propertyField , mas isso não acontece, pois a função Substituir não altera o objeto original.



Aviso 4



V3038 O argumento '"yy"' foi passado para o método 'Substituir' várias vezes. É possível que outro argumento seja passado em seu lugar. 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("'", "")
        ;
}
      
      





Aqui, as chamadas para as funções Replace são escritas corretamente, mas em um lugar isso é feito com argumentos idênticos estranhos.



Potencial NullReferenceException



image9.png


Aviso 1



A expressão V3022 'portalUser.BirthDate.ToString ()' sempre não é nula. O operador '??' é excessivo. 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 não será nulo . A verificação foi feita aqui para gerar o valor "NULL" no log de depuração se a data não estiver definida. Mas desde se não houver valor, o método ToString retornará uma string vazia, então o erro no algoritmo pode ser menos perceptível nos logs.



Toda a lista de locais de registro questionáveis ​​é assim:



  • A expressão V3022 'ldapUser.BirthDate.ToString ()' sempre não é nula. O operador '??' é excessivo. LdapUserManager.cs 437
  • A expressão V3022 'portalUser.Sex.ToString ()' não é sempre nula. O operador '??' é excessivo. LdapUserManager.cs 444
  • A expressão V3022 'ldapUser.Sex.ToString ()' sempre não é nula. O operador '??' é excessivo. LdapUserManager.cs 445


Aviso 2



V3095 O objeto 'r.Attributes ["href"]' foi usado antes de ser verificado em relação ao nulo. Verifique as linhas: 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");
    }))
    {
      ....
    }
    ....
}
      
      





Ao analisar Html ou Xml, é muito perigoso referir-se a atributos pelo nome sem validação. Este bug é especialmente atraente porque o valor do atributo href é primeiro recuperado e depois verificado para ver se ele está presente.



Aviso 3



V3146 Possível desreferência nula. O 'listTags.FirstOrDefault' pode retornar um valor nulo padrão. FileMarker.cs 299



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





O analisador detectou um uso não seguro do resultado da chamada do método FirstOrDefault . Este método retorna um valor padrão se não houver objetos na lista que satisfaçam o predicado de pesquisa. O padrão para tipos de referência é uma referência nula. Assim, antes de usar o link resultante, ele deve ser verificado, e não imediatamente chamado de propriedade, como aqui.



Aviso 4



V3115 Passar o método 'null' para 'Equals' não deve resultar em '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);
    }
    ....
}
      
      





As referências de objeto em C # são frequentemente comparadas a nulas . Portanto, ao sobrecarregar os métodos de comparação, é muito importante antecipar tais situações e adicionar verificações apropriadas no início da função. Mas aqui eles não fizeram.



Outros bugs



image10.png


Aviso 1



A expressão V3022 é sempre verdadeira. Provavelmente, o operador '&&' deve ser usado aqui. 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();
    ....
}
      
      





Chamar o método CreateItem lançará uma ArgumentException . A questão é que a primeira expressão condicional contém um erro. A condição sempre é avaliada como verdadeira . O erro está na escolha do operador lógico. Você deveria ter usado o operador &&.



Provavelmente, esse método nunca foi chamado ainda. é virtual e sempre foi redefinido em classes derivadas até agora.



Para evitar tais erros no futuro, recomendo ler meu artigo e guardar um link para ele: " Expressões lógicas. Como os profissionais cometem erros". Todas as combinações errôneas de operadores lógicos são fornecidas e analisadas.



Aviso 2



V3052 O objeto de exceção original 'ex' foi engolido. A pilha da exceção original pode ser perdida. 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;
    }
}
      
      





Aqui, convertemos GoogleApiException em SecurityException , enquanto perdemos informações úteis da exceção original.



Uma pequena mudança como esta tornará o aviso gerado mais informativo:



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





Embora seja perfeitamente possível que GoogleApiException tenha sido ocultado propositalmente .



Aviso 3



O componente Minutos V3118 de TimeSpan é usado, o que não representa o intervalo de tempo completo. Possivelmente, o valor 'TotalMinutes' era o pretendido. NotifyClient.cs 281



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

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





Eu achava que o diagnóstico era preventivo. No código dos meus projetos, sempre deu falsos avisos. Aqui, tenho quase certeza de que havia um bug. Provavelmente, você deve usar a propriedade TotalMinutes em vez de Minutos .



Aviso 4



V3008 A variável 'chave' recebe valores duas vezes sucessivamente. Talvez seja um engano. Verifique as linhas: 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;
}
      
      





O problema com esse fragmento é que, ao inserir funções, uma matriz de bytes é sempre criada e, em seguida, imediatamente substituída. Essa. há uma alocação constante de memória que não faz sentido.



Sua melhor aposta seria mudar para C # 8 em vez do C # 5 usado e escrever um código mais curto:



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





Não posso dizer se o projeto pode ser atualizado ou não, mas existem muitos desses lugares. É aconselhável reescrevê-los de alguma forma:



  • V3008 A variável 'hmacKey' recebe valores duas vezes sucessivamente. Talvez seja um engano. Verifique as linhas: 256, 252. Metadata.cs 256
  • V3008 A variável 'hmacHash' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 270, 264. Metadata.cs 270
  • V3008 A variável 'caminhos' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 512, 508. RackspaceCloudStorage.cs 512
  • V3008 A variável 'b' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 265, 264. BookmarkingUserControl.ascx.cs 265
  • V3008 A variável 'taskIds' tem valores atribuídos duas vezes sucessivamente. Talvez seja um engano. Verifique as linhas: 412, 391. TaskDao.cs 412


Como último recurso, você não precisa alocar memória ao declarar uma variável.



Bug no PVS-Studio



image11.png


Você acha que só escrevemos sobre os erros dos outros. Não, nossa equipe é autocrítica, admite seus erros e não hesita em escrever sobre eles também. Todo mundo está errado.



No decorrer do trabalho no artigo, encontramos um bug bastante estúpido. Reconhecemos e nos apressamos em compartilhar.



Código do mesmo Community Server:



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





Tive que dar um alerta completo do analisador antes do código, como é feito ao longo do artigo, mas esse é o problema. O aviso é parecido com este:



image12.png


Os caracteres de controle \ r e \ n não são escapados antes de serem impressos na tabela.



Conclusão



image13.png


Faz muito tempo que não vejo um projeto tão interessante. Agradecimentos aos colaboradores do ONLYOFFCE. Queríamos entrar em contato com você, mas não houve feedback.



Escrevemos artigos como este regularmente . Este gênero tem mais de dez anos. Portanto, os desenvolvedores não devem levar as críticas para o lado pessoal. Teremos o maior prazer em emitir uma versão completa do relatório para melhorar o projeto ou fornecer uma licença temporária para verificar o projeto. E não apenas para o projeto CommunityServer, mas para todos que o desejam por UM MÊS usando o código promocional #onlyoffice .



image14.png


Além disso, os profissionais de segurança estarão interessados ​​em saber que apoiamos ativamente o padrão OWASP. Alguns diagnósticos já estão disponíveis. E em breve a interface do analisador será aprimorada para tornar a inclusão de um ou outro padrão para análise de código ainda mais conveniente.





Se você deseja compartilhar este artigo com um público de língua inglesa, por favor use o link de tradução: Svyatoslav Razmyslov. ONLYOFFICE Community Server: como os bugs contribuem para o surgimento de problemas de segurança .



All Articles