# ONLYOFFICE Community Server: how bugs contribute to the emergence of security problems Server\-side network applications rarely get the chance to join the ranks of our reviews of errors found in open source software\. This is probably due to their popularity\. After all, we try to pay attention to the projects that readers themselves offer us\. At the same time, servers often perform very important functions, but their performance and benefits remain invisible to most users\. So, by chance, the code of ONLYOFFICE Community Server was checked\. It turned out to be a very fun review\. ![0783_ONLYOFFICE_CommunityServer/image1.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image1.png) ## Introduction [ONLYOFFICE Community Server](https://github.com/ONLYOFFICE/CommunityServer) \- free open\-source collaborative system developed to manage documents, projects, customer relationship and email correspondence, all in one place\. On its [website](https://www.onlyoffice.com/), the company emphasizes the security of its solutions with phrases such as "Run your private office with the ONLYOFFICE " and "Secure office and productivity apps"\. However, no tools for code quality control are apparently used in the development process\. It all started with the fact that I was looking through the source code of several network applications in search of inspiration for implementing one of my application ideas\. The [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) analyzer was working in the background, and I was sending funny errors into the general corporate chat\. This resulted in several posts of error examples on [Twitter](https://twitter.com/Code_Analysis): ![0783_ONLYOFFICE_CommunityServer/image2.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image2.png) Later, representatives commented on the tweet, and even later posted a denial of the problem: ![0783_ONLYOFFICE_CommunityServer/image3.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image3.png) Most likely, this is true\. Anyway, this does not add points to the quality of the project\. Let's see what else I managed to find there\. ## "Wizard" of checking input data ![0783_ONLYOFFICE_CommunityServer/image4.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image4.png) I am flabbergasted by the singularity of some developers' approaches to checking input data\. **Warning 1** [V3022](https://pvs-studio.com/en/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; } ``` As you might have noticed, this code fragment sets the tone for the entire article\. It can be described with the phrase "The code is funny, but the situation is terrible"\. One must be very tired to confuse the _password_ variable with the string _"password"_\. This error allows code execution to continue with an empty password\. According to the author of the code, the password is additionally checked in the program's interface\. However, the programming process is designed so that previously written functions are often reused\. Therefore, this error can manifest itself anywhere in the future\. Always remember the importance of well\-timed detecting errors in your code\. **Warning 2** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'String\.IsNullOrEmpty\("name"\)' is always false\. SendInterceptorSkeleton\.cs 36 [V3022](https://pvs-studio.com/en/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; } ``` Suddenly, several similar errors were found in the code\. It is funny at first, but one should think about the reasons for writing such code\. Maybe this is a habit left after switching from another programming language\. From our experience of checking C\+\+ projects, when it comes to C\+\+, errors are often brought by former Python programmers\. **Warning 3** [V3022](https://pvs-studio.com/en/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"); .... } ``` The _id_ variable has an unsigned _uint_ type\. Therefore, the check is pointless here\. Call of this function has to be given special attention\. I wonder what is passed to this function\. Most likely, the signed _int_ type was used everywhere before, but after refactoring, the check remained\. ## Copy\-paste code ![0783_ONLYOFFICE_CommunityServer/image5.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image5.png) **Warning 1** [V3001](https://pvs-studio.com/en/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/image6.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image6.png) This code fragment had to be rendered as an image to convey the scale of the written conditional expression\. It has a problem area\. Specifying a place in the analyzer's message can hardly help a user find 2 identical checks\. So let's use a red marker: ![0783_ONLYOFFICE_CommunityServer/image7.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image7.png) And here are the conditional expressions that the analyzer warned about\. In addition to fixing this place, I would recommend that the author formatted the code better to avoid such errors in the future\. **Warning 2** [V3030](https://pvs-studio.com/en/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] == '-') { .... } ``` The _user_ string is checked 2 times in a row in the same way\. Perhaps, this code can be slightly refactored\. Although on the other hand, maybe in one of the cases the programmer wanted to check the _absolute_ Boolean variable\. **Warning 3** [V3021](https://pvs-studio.com/en/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(":")) { .... } .... } ``` I'm sure you couldn't find the error here just by reviewing the fragment\. The analyzer detected a useless check, which turned out to be a copy of the code from above\. The _sLink_ variable must be checked instead of the _str_ variable\. **Warning 4** [V3004](https://pvs-studio.com/en/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(); } .... } ``` The analyzer found a very clear Copy\-Paste code\. Perhaps, the _paths_ variable has to be evaluated recursively, but this wasn't done\. **Warning 5** [V3009](https://pvs-studio.com/en/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; } ``` The size of this function is 135 lines\. Even the developers themselves left a comment that it should be simplified\. The function code definitely needs some tweaks, because it also returns the same value in all cases\. ## Useless function calls ![0783_ONLYOFFICE_CommunityServer/image8.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image8.png) **Warning 1** [V3010](https://pvs-studio.com/en/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(); .... } ``` The _Distinct_ method removes duplicates from the collection\. But in C\#, most of these extension methods do not change the object, but create a copy\. So in this example, the _result_ list remains the same as it was before the method was called\. You can also see the names _login_ and _passwordHash_\. This may be another security issue\. **Warning 2** [V3010](https://pvs-studio.com/en/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); } .... } ``` The _ToString_ method is standard here\. It returns a text representation of the object, but the return value is not used\. **Warning 3** [V3010](https://pvs-studio.com/en/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(" ", ""); .... } ``` Someone made a serious mistake\. It was necessary to remove all spaces from the _propertyField_ property, but this did not happen, because the _Replace_ function did not change the source object\. **Warning 4** [V3038](https://pvs-studio.com/en/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("'", "") ; } ``` Here, calls to _Replace_ functions are written correctly, but in one place it is done with strange identical arguments\. ## Potential NullReferenceException ![0783_ONLYOFFICE_CommunityServer/image9.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image9.png) **Warning 1** [V3022](https://pvs-studio.com/en/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_ won't have the _null_ value\. The check was made here in order to output the "NULL" value to the debug log if the date is not set\. However, since the _ToString_ method returns an empty string if there is no value, the error in the algorithm may be less noticeable in the logs\. The entire list of questionable logging places looks like this: * 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 **Warning 2** [V3095](https://pvs-studio.com/en/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"); })) { .... } .... } ``` When parsing Html or Xml, it is very dangerous to access attributes by name without checking\. This error is particularly stunning because the value of the _href_ attribute is first extracted and then checked to see if it is present at all\. **Warning 3** [V3146](https://pvs-studio.com/en/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; .... } ``` The analyzer detected unsafe use of the result of calling the _FirstOrDefault_ method\. This method returns the default value if there is no object in the list that matches the search predicate\. The default value for reference types is an empty reference \(null\)\. Accordingly, before using the resulting reference, one must check it, and not call the property immediately, as opposed to as it is here\. **Warning 4** [V3115](https://pvs-studio.com/en/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); } .... } ``` Object references in C\# are often compared with _null_\. Therefore, when overloading comparison methods, it is very important to anticipate such situations and add the appropriate check to the beginning of the function\. In this case the authors did not do it here\. ## Other errors ![0783_ONLYOFFICE_CommunityServer/image10.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image10.png) **Warning 1** [V3022](https://pvs-studio.com/en/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(); .... } ``` The call of the _CreateItem_ method will result in an _ArgumentException_\. The fact is that the very first conditional expression contains an error\. The condition always has the result _true_\. The error is in choosing a logical operator\. One should have used the && operator\. Most likely, this method has never been called before, as it is virtual and has always been overridden in derived classes until now\. To avoid such errors in future, I recommend reading my article: "[Logical expressions in C, C\+\+, C\#, and Java\. Mistakes made by professionals](https://pvs-studio.com/en/blog/posts/cpp/0390/)"\. Don't forget to save the link to it\. You can find review of all erroneous combinations from logical operators there\. **Warning 2** [V3052](https://pvs-studio.com/en/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; } } ``` The _GoogleApiException_ exception was converted to _SecurityException_ while losing information from the original exception that might be useful\. This small change will make the generated warning more informative: ```cpp throw new SecurityException(ex.Error.Message, ex); ``` Although it may well be possible that the _GoogleApiException_ exception was hidden intentionally\. **Warning 3** [V3118](https://pvs-studio.com/en/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; .... } ``` I used to think that diagnostics are of the precautionary nature\. As for code of my projects, it always gave false warnings\. In this case, I'm almost sure there was an error\. Most likely, one should have used the _TotalMinutes_ property instead of _Minutes_\. **Warning 4** [V3008](https://pvs-studio.com/en/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; } ``` The problem with this fragment is that when you enter a function, an array of bytes is always created, and then gets immediately reassigned\. In other words, there is constant allocation of memory, which does not make sense\. The best way would be to switch to C\#8 instead of C\#5 and write shorter code: ```cpp private byte[] GenerateKey() { using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....); return deriveBytes.GetBytes(keyLength); } ``` I can't say whether the project can be upgraded or not, but there are quite a few such places\. It is best to rewrite them somehow: * 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 As a last resort, one can refuse from allocating memory when declaring a variable\. ## Bug in PVS\-Studio ![0783_ONLYOFFICE_CommunityServer/image11.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image11.png) You might think we only write about other people's mistakes\. No, our team is self\-critical, admits its mistakes and doesn't hesitate to write about them too\. Everyone makes mistakes\. While working on the article, we found a rather stupid bug\. We recognize it and would like to share it with you\. Code from the same Community Server: ```cpp private bool IsPhrase(string searchText) { return searchText.Contains(" ") || searchText.Contains("\r\n") || searchText.Contains("\n"); } ``` Normally, I would cite a full analyzer warning before the code, as is done in the entire article, but that's the problem\. The warning looks like this: ![0783_ONLYOFFICE_CommunityServer/image12.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image12.png) The \\r and \\n control characters are not escaped before being output to the table\. ## Conclusion ![0783_ONLYOFFICE_CommunityServer/image13.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image13.png) It's been a long time since I came across such an interesting project to check\. Thanks to the authors of ONLYOFFCE\. We contacted you, but there was no feedback\. We regularly write such [articles](https://pvs-studio.com/en/blog/inspections/)\. This genre is more than ten years old\. Therefore, developers should not take criticism to heart\. We will be happy to share a full version of the report to improve the project or provide a temporary license to review the project\. This refers not only to the developers of CommunityServer project, but to everyone who wants to use the [\#onlyoffice](https://pvs-studio.com/en/pvs-studio/download/?promo=onlyoffice?promo=onlyoffice) promo code to be able to use the analyzer for ONE MONTH for free\. ![0783_ONLYOFFICE_CommunityServer/image14.png](https://import.viva64.com/docx/blog/0783_ONLYOFFICE_CommunityServer/image14.png) Security experts will also be interested to know that we are actively supporting the OWASP standard\. Some diagnostics are already available\. The analyzer interface will soon be updated to make it even more convenient to enable a particular standard for code analysis\.