Server-side network applications rarely appear in our open source bug reports. This is probably due to their popularity. After all, we try to pay attention to the projects that the readers themselves offer us. And servers often perform very important functions, but their activities and benefits remain invisible to most users. So, purely by chance, the ONLYOFFICE Community Server code was checked. It turned out to be a very funny review.
Introduction
ONLYOFFICE Community Server is a free open source collaboration system designed to manage documents, projects, customer interactions and email communications in one place. On its website, 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". But, apparently, no tools for quality control of the code are used in the development process.
It all started with the fact that I looked through the source code of several network applications in search of inspiration for the implementation of one of my application ideas. The PVS-Studio analyzer was running in the background , and I threw funny mistakes into the general corporate chat.
So, a few examples went to Twitter :
Representatives later commented on the tweet, and even later posted a denial of the problem:
This is most likely true. But this does not add points to the quality of the project. Let's see what else was found there.
Input Validation Wizard
Some of the developers' approaches to validating input data are striking in their originality.
Warning 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;
}
As you can see, this piece of code sets the tone for the entire article. It can be described by the phrase "The code is funny, but the situation is terrible." You probably have to get very tired to confuse the password variable with the "password" string . This error allows you to continue executing the code with an empty password. According to the author of the code, the password is additionally checked in the program interface. But the programming process is designed in such a way that previously written functions are often reused. Therefore, this error can manifest itself anywhere in the future. Always remember the importance of catching bugs in your code early.
Warning 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;
}
All of a sudden, a whole bunch of similar errors appeared in the code. If at first it was funny, now it is worth thinking about the reasons for writing such code. Maybe this habit remained after switching from another programming language. In C ++, mistakes are often brought by former Python programmers in our experience of checking C ++ projects.
Warning 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");
....
}
The id variable is of unsigned uint type . Therefore, checking is pointless here. It is worth paying attention to calling this function. I wonder what is being passed to this function. Most likely, the signed type int was used everywhere , but after refactoring the check remained.
Copy-Paste code
Warning 1
V3001 There are identical sub-expressions 'searchFilterData.WithCalendar == WithCalendar' to the left and to the right of the '&&' operator. MailSearchFilterData.cs 131
This piece of code had to be shown as a picture to convey the scale of the written conditional expression. There is a problem area in it. Specifying a place in the analyzer warning can hardly help you find 2 identical checks. Therefore, we will use the red marker:
And here are the same conditionals that the analyzer warned about. In addition to fixing this point, I would recommend that you style the code better so that it does not contribute to such errors in the future.
Warning 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] == '-')
{
....
}
The user string is checked 2 times in a row in the same way. Perhaps this code can be refactored a little. Although who knows, maybe in one of the cases they wanted to check the boolean variable absolute .
Warning 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(":"))
{
....
}
....
}
I am sure that you cannot find a mistake here with your eyes. The analyzer found a useless check, which turned out to be the copied code from above. Instead of the str variable, the sLink variable should be checked .
Warning 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();
}
....
}
The analyzer has detected a very descriptive Copy-Paste code. Perhaps, in one of the cases, the paths variable needs to be calculated recursively, but this was not done.
Warning 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;
}
The size of this function is 135 lines. Even the developers themselves left a comment that it should be simplified. You definitely need to work with the function code, because it also returns one value in all cases.
Useless function calls
Warning 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();
....
}
The Distinct method removes duplicates from the collection. But in C #, most of these extension methods do not modify the object, but create a copy. Thus, in this example, the result list remains the same as before the method call. You can also see the names login and passwordHash here . Perhaps this is another security problem.
Warning 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);
}
....
}
The ToString method is standard here. It returns a textual representation of the object, but the return value is not used.
Warning 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(" ", "");
....
}
Someone made a serious mistake. All spaces had to be removed from the propertyField property , but this does not happen, since the Replace function does not change the original object.
Warning 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("'", "")
;
}
Here the calls to the Replace functions are written correctly, but in one place it is done with strange identical arguments.
Potential NullReferenceException
Warning 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 will not be null . The check was done here in order to output the "NULL" value to the debug log if the date is not set. But since in the absence of a value, the ToString method will return an empty string, then 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 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");
}))
{
....
}
....
}
When parsing Html or Xml, it is very dangerous to refer to attributes by name without validation. This bug is especially appealing in that the value of the href attribute is first retrieved and then checked to see if it is present at all.
Warning 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;
....
}
The analyzer detected an unsafe use of the result of calling the FirstOrDefault method . This method returns a default value if there are no objects in the list that satisfy the search predicate. The default for reference types is a null reference. Accordingly, before using the resulting link, it should be checked, and not immediately called the property, as here.
Warning 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);
}
....
}
Object references in C # are often compared to null . Therefore, when overloading comparison methods, it is very important to anticipate such situations and add the appropriate check at the beginning of the function. But here they didn't.
Other bugs
Warning 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();
....
}
Calling the CreateItem method will throw an ArgumentException . The point is that the very first conditional expression contains an error. The condition always evaluates to true . The error lies in the choice of the logical operator. You should have used the && operator.
Most likely, this method has never been called yet. it is virtual and has always been redefined in derived classes until now.
In order to avoid such mistakes in the future, I recommend reading my article and saving a link to it: " Logical expressions. How professionals make mistakes". All erroneous combinations of logical operators are given and analyzed.
Warning 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;
}
}
Here we have converted a GoogleApiException to a SecurityException , while losing information from the original exception that might be useful.
A small change like this will make the generated warning more informative:
throw new SecurityException(ex.Error.Message, ex);
Although it is entirely possible that the GoogleApiException was hidden on purpose .
Warning 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;
....
}
I used to think that diagnostics were preventive. In the code of my projects, it always gave false warnings. Here, I'm pretty sure there was a bug. Most likely, you should use the TotalMinutes property instead of Minutes .
Warning 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;
}
The problem with this fragment is that when entering functions, an array of bytes is always created, and then immediately overwritten. Those. there is a constant allocation of memory that does not make sense.
Your best bet would be to switch to C # 8 instead of the used C # 5 and write shorter code:
private byte[] GenerateKey()
{
using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
return deriveBytes.GetBytes(keyLength);
}
I can't say if the project can be upgraded or not, but there are many such places. It is advisable 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, you do not need to allocate memory when declaring a variable.
Bug in PVS-Studio
You think we only write about the mistakes of others. No, our team is self-critical, admits its mistakes and does not hesitate to write about them too. Everyone is wrong.
In the course of work on the article, we found a rather stupid bug. We acknowledge and hurry to share.
Code from the same Community Server:
private bool IsPhrase(string searchText)
{
return searchText.Contains(" ") || searchText.Contains("\r\n") ||
searchText.Contains("\n");
}
I had to give a full warning from the analyzer before the code, as is done throughout the article, but this is the problem. The warning looks like this:
The control characters \ r and \ n are not escaped before being printed to the table.
Conclusion
I haven't come across such an interesting project for a long time. Thanks to the ONLYOFFCE contributors. We wanted to contact you, but there was no feedback.
We write articles like this on a regular basis . This genre is over ten years old. Therefore, developers should not take criticism personally. We will be happy to issue a full version of the report to improve the project or provide a temporary license to check the project. And not only to the CommunityServer project, but to everyone who wants it for ONE MONTH using the #onlyoffice promo code .
Also, security professionals will be interested to know that we are actively supporting the OWASP standard. Some diagnostics are already available. And soon the analyzer interface will be improved to make the inclusion of one or another standard for code analysis even more convenient.
If you want to share this article with an English-speaking audience, please use the translation link: Svyatoslav Razmyslov. ONLYOFFICE Community Server: how bugs contribute to the emergence of security problems .