ONLYOFFICE Community Server：バグがセキュリティ問題にどのように寄与するか

サーバー側のネットワークアプリケーションがオープンソースのバグレポートに表示されることはめったにありません。これはおそらく彼らの人気によるものです。結局のところ、私たちは読者自身が私たちに提供するプロジェクトに注意を払うようにしています。また、サーバーは非常に重要な機能を実行することがよくありますが、そのアクティビティと利点はほとんどのユーザーには見えません。したがって、純粋に偶然に、ONLYOFFICE CommunityServerコードがチェックされました。それは非常に面白いレビューであることが判明しました。

前書き

ONLYOFFICE Community Serverは、ドキュメント、プロジェクト、顧客とのやり取り、および電子メール通信を1か所で管理するように設計された無料のオープンソースコラボレーションシステムです。同社の ウェブサイトでは、「ONLYOFFICEでプライベートオフィスを運営する」や「オフィスと生産性の高いアプリを保護する」などのフレーズでソリューションのセキュリティを強調しています。しかし、明らかに、コードの品質管理のためのツールは開発プロセスで使用されていません。



それはすべて、自分のアプリケーションのアイデアの1つを実装するためのインスピレーションを求めて、いくつかのネットワークアプリケーションのソースコードを調べたという事実から始まりました。PVS-Studioアナライザーはバックグラウンドで実行されていました そして、私は一般的な企業のチャットに面白い間違いを投げかけました。



だから、いくつかの例がツイッターに行きまし た：





代表者は後でツイートにコメントし、さらに後で問題の否定を投稿しました：





これはおそらく真実です。しかし、これはプロジェクトの品質にポイントを追加しません。そこで他に何が見つかったか見てみましょう。

入力検証ウィザード

入力データを検証するための開発者のアプローチのいくつかは、その独創性に際立っています。



警告



1V3022式 'string.IsNullOrEmpty（ "password"）'は常に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"文字列と 混同するには、おそらく非常に疲れる必要があり ます。このエラーにより、空のパスワードでコードを実行し続けることができます。コードの作成者によると、パスワードはプログラムインターフェイスで追加でチェックされます。しかし、プログラミングプロセスは、以前に作成された関数が頻繁に再利用されるように設計されています。したがって、このエラーは将来どこにでも現れる可能性があります。コードのバグを早期に発見することの重要性を常に覚えておいてください。



警告



2V3022式 'String.IsNullOrEmpty（ "name"）'は常にfalseです。 SendInterceptorSkeleton。 cs 36



V3022式 'String.IsNullOrEmpty（ "sendInterceptor"）'は常に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 ++では、C ++プロジェクトをチェックした経験で、元Pythonプログラマーが間違いを犯すことがよくあります。



警告



3V3022式 'id <0'は常にfalseです。符号なしタイプの値は常に> = 0です。UserFolderEngine.cs173

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

IDの変数は 符号なしであるUINTのタイプ 。したがって、ここではチェックは無意味です。この関数の呼び出しには注意が必要です。この関数に何が渡されているのだろうか。ほとんどの場合、signed type intがどこでも使用されて いましたが、リファクタリング後もチェックが残っていました。

コードをコピーして貼り付ける

1警告



V3001左にと「&&」演算子の右側に同じ部分式「searchFilterData.WithCalendar == WithCalendar」をあります。MailSearchFilterData.cs 131





このコードは、記述された条件式のスケールを伝えるために画像として提示する必要がありました。その中に問題領域があります。アナライザーの警告で場所を指定しても、2つの同一のチェックを見つけるのに役立つことはほとんどありません。したがって、赤いマーカーを使用します。





そして、これはアナライザーが警告したのと同じ条件です。これを修正することに加えて、将来そのようなエラーの出現に寄与しないように、コードのスタイルを改善することをお勧めします。



警告



2V3030定期的なチェック。'！String.IsNullOrEmpty（user）'条件は、173行目ですでに検証されています。CommonLinkUtility.cs176

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] == '-')
      {
        ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

同じ方法で、ユーザー文字列を 2回続けてチェックします。おそらく、このコードは少しリファクタリングできます。誰が知っているとしても、おそらく1つのケースでは、ブール変数absoluteをチェックしたかったのでしょう 。



警告



3V3021同一の条件式を持つ2つの「if」ステートメントがあります。最初の 'if'ステートメントにはメソッドreturnが含まれています。これは、2番目の「if」ステートメントが無意味なWikiEngine.cs688であることを意味します。

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変数をチェックする必要があります 。



警告



4V3004「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();
    }
    ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

アナライザーは、非常にわかりやすいコピーアンドペーストコードを検出しました。おそらく、あるケースでは、paths変数を 再帰的に計算する必要がありますが、これは行われていません。



警告



5V3009このメソッドが常に同じ値の「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つの値を返します。

役に立たない関数呼び出し

警告



1V3010関数「Distinct」の戻り値を使用する必要があります。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＃では、これらの拡張メソッドのほとんどはオブジェクトを変更せず、コピーを作成します。したがって、この例では、結果リスト はメソッド呼び出し前と同じままです。loginと passwordHashの名前もここで確認でき ます。おそらく、これは別のセキュリティ問題です。



警告



2V3010関数「ToString」の戻り値を利用する必要があります。 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メソッド が標準です。オブジェクトのテキスト表現を返しますが、戻り値は使用されません。



警告



3V3010関数「Replace」の戻り値を使用する必要があります。 TextFileUserImporter.cs 252

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

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

誰かが重大な間違いを犯しました。propertyFieldプロパティから すべてのスペースを削除する必要がありましたが、これは発生しません。置換機能 は元のオブジェクトを変更しません。



警告



4V3038 '"yy"'引数が 'Replace'メソッドに数回渡されました。代わりに他の引数を渡す必要がある可能性があります。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

警告



1V3022式 'portalUser.BirthDate.ToString（）'は常にnullではありません。オペレーター '？？' 過剰です。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式 'ldapUser.BirthDate.ToString（）'は常にnullではありません。オペレーター '？？' 過剰です。LdapUserManager.cs 437
• V3022式 'portalUser.Sex.ToString（）'は常にnullではありません。オペレーター '？？' 過剰です。LdapUserManager.cs 444
• V3022式 'ldapUser.Sex.ToString（）'は常にnullではありません。オペレーター '？？' 過剰です。LdapUserManager.cs 445

2警告



V3095それがnullに照らして検証される前に、ザ・「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属性の値が最初に取得され、次にそれが存在するかどうかを確認するという点で特に魅力的 です。



警告



3V3146nullの逆参照の可能性。 'listTags.FirstOrDefault'は、デフォルトのnull値を返すことができます。 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参照です。したがって、結果のリンクを使用する前に、ここのようにすぐにプロパティを呼び出すのではなく、チェックする必要があります。



警告



4V3115「null」を「Equals」メソッドに渡しても「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と比較され ます。したがって、比較メソッドをオーバーロードする場合、そのような状況を予測し、関数の先頭に適切なチェックを追加することが非常に重要です。しかし、ここではそうではありませんでした。

その他のバグ

警告



1V3022式は常に真です。おそらく、ここでは「&&」演算子を使用する必要があります。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と評価されます。エラーは、論理演算子の選択にあります。 &&演算子を使用する必要があります。



ほとんどの場合、このメソッドはまだ呼び出されていません。これは仮想であり、これまで派生クラスで常に再定義されてきました。



今後このような間違いを避けるために、私の記事を読んで、その記事へのリンクを保存することをお勧めします。「 論理表現。専門家が間違いを犯す方法"。論理演算子のすべての誤った組み合わせが与えられ、分析されます。



警告



2V3052元の例外オブジェクト 'ex'が飲み込まれました。元の例外のスタックが失われる可能性があります。GoogleDriveStorage.cs267

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が意図的に隠されている可能性は十分にあり ますが。



警告



3TimeSpanのV3118Minutesコンポーネントが使用されていますが、これは完全な時間間隔を表すものではありません。おそらく、「TotalMinutes」値が代わりに意図されていました。 NotifyClient.cs 281

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

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

私は、診断は予防的だと思っていました。私のプロジェクトのコードでは、それは常に誤った警告を出していました。ここでは、バグがあったと確信しています。ほとんどの場合、Minutesの代わりに TotalMinutesプロパティを使用する必要があります 。 警告4V3008「key」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェック行：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＃5ではなくC＃8に切り替えて、より短いコードを作成することです。

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

プロジェクトがアップグレードできるかどうかはわかりませんが、そのような場所はたくさんあります。なんらかの方法で書き直すことをお勧めします。

• V3008「hmacKey」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェック行：256、252。Metadata.cs 256
• V3008「hmacHash」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェック行：270、264。Metadata.cs 270
• V3008「paths」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェックライン：512、508。RackspaceCloudStorage.cs 512
• V3008「b」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェック行：265、264。BookmarkingUserControl.ascx.cs 265
• V3008「taskIds」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェックライン：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の貢献者に感謝します。連絡したかったのですが、フィードバックはありませんでした。



私たちは定期的にこのような記事を書いています 。このジャンルは10年以上前のものです。したがって、開発者は個人的に批判するべきではありません。プロジェクトを改善するため、またはプロジェクトをチェックするための一時的なライセンスを提供するために、レポートのフルバージョンを発行させていただきます。そして、CommunityServerプロジェクトだけでなく、＃onlyofficeプロモーションコードを使用して1か月間それを希望するすべての人に 。





また、セキュリティの専門家は、私たちがOWASP標準を積極的にサポートしていることを知りたいと思うでしょう。いくつかの診断はすでに利用可能です。そしてまもなく、アナライザーのインターフェースが改善され、コード分析のための1つまたは別の標準を含めることがさらに便利になります。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクSvyatoslavRazmyslovを使用してください。 ONLYOFFICE Community Server：バグがセキュリティ問題の発生にどのように寄与するか。