Les applications réseau côté serveur apparaissent rarement dans nos rapports de bogues open source. Cela est probablement dû à leur popularité. Après tout, nous essayons de prêter attention aux projets que les lecteurs eux-mêmes nous proposent. Et les serveurs remplissent souvent des fonctions très importantes, mais leurs activités et avantages restent invisibles pour la plupart des utilisateurs. Donc, par pur hasard, le code ONLYOFFICE Community Server a été vérifié. Cela s'est avéré être une critique très amusante.
introduction
ONLYOFFICE Community Server est un système de collaboration open source gratuit conçu pour gérer les documents, les projets, les interactions clients et les e-mails en un seul endroit. Sur son site Internet, l' entreprise met l'accent sur la sécurité de ses solutions avec des phrases telles que «Gérez votre bureau privé avec ONLYOFFICE» et «Applications de bureau et de productivité sécurisées». Mais, apparemment, aucun outil de contrôle de qualité du code n'est utilisé dans le processus de développement.
Tout a commencé par le fait que j'ai parcouru le code source de plusieurs applications réseau à la recherche d'inspiration pour la mise en œuvre d'une de mes idées d'application. L'analyseur PVS-Studio fonctionnait en arrière-plan et j'ai jeté des erreurs amusantes dans le chat général de l'entreprise.
Ainsi, quelques exemples sont allés sur Twitter :
Les représentants ont ensuite commenté le tweet et ont même plus tard publié un déni du problème:
C'est probablement vrai. Mais cela n'ajoute pas de points à la qualité du projet. Voyons ce qui a été trouvé là-bas.
Assistant de validation d'entrée
Certaines des approches du développeur pour valider les données d'entrée sont frappantes par leur originalité.
Avertissement 1
V3022 Expression 'string.IsNullOrEmpty ("password")' est toujours faux. 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;
}
Comme vous pouvez le voir, ce morceau de code donne le ton à tout l'article. Cela peut être décrit par la phrase "Le code est drôle, mais la situation est terrible". Vous devez probablement être très fatigué pour confondre la variable de mot de passe avec la chaîne "password" . Cette erreur vous permet de continuer à exécuter le code avec un mot de passe vide. Selon l'auteur du code, le mot de passe est en outre vérifié dans l'interface du programme. Mais le processus de programmation est conçu de telle manière que les fonctions précédemment écrites sont souvent réutilisées. Par conséquent, cette erreur peut se manifester n'importe où dans le futur. Souvenez-vous toujours de l'importance d'attraper les bogues dans votre code tôt.
Avertissement 2
L' expression V3022 'String.IsNullOrEmpty ("name")' est toujours fausse. SendInterceptorSkeleton. cs 36
L' expression V3022 'String.IsNullOrEmpty ("sendInterceptor")' est toujours fausse. 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;
}
Tout à coup, il y a eu un certain nombre d'erreurs similaires dans le code. Si au début c'était drôle, cela vaut la peine de réfléchir aux raisons d'écrire un tel code. Peut-être que cette habitude est restée après le passage d'un autre langage de programmation. En C ++, des erreurs sont souvent apportées par d'anciens programmeurs Python dans notre expérience de vérification de projets C ++.
Avertissement 3
V3022 L' expression «id <0» est toujours fausse. La valeur de type non signé est toujours> = 0. UserFolderEngine.cs 173
public MailUserFolderData Update(uint id, string name, uint? parentId = null)
{
if (id < 0)
throw new ArgumentException("id");
....
}
La variable id est de type uint non signé . Par conséquent, la vérification est inutile ici. Il convient de prêter attention à l'appel de cette fonction. Je me demande ce qui est passé à cette fonction. Très probablement, le type signé int a été utilisé partout , mais après la refactorisation, la vérification est restée.
Copier-coller du code
Avertissement 1
V3001 Il existe des sous-expressions identiques 'searchFilterData.WithCalendar == WithCalendar' à gauche et à droite de l'opérateur '&&'. MailSearchFilterData.cs 131
Ce morceau de code devait être présenté sous la forme d'une image pour véhiculer l'échelle de l'expression conditionnelle écrite. Il y a un problème là-dedans. Spécifier une place dans l'avertissement de l'analyseur peut difficilement vous aider à trouver 2 vérifications identiques. Par conséquent, nous utiliserons le marqueur rouge:
Et voici les mêmes conditions que l'analyseur a mis en garde. En plus de corriger ce point, je vous recommande de mieux styliser le code afin qu'il ne contribue pas à de telles erreurs à l'avenir.
Avertissement 2
V3030 Contrôle récurrent. La condition '! String.IsNullOrEmpty (user)' a déjà été vérifiée à la ligne 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] == '-')
{
....
}
La chaîne utilisateur est vérifiée 2 fois de suite de la même manière. Peut-être que ce code peut être un peu remanié. Bien que qui sait, peut-être dans l'un des cas, ils voulaient vérifier la variable booléenne absolue .
Avertissement 3
V3021 Il existe deux instructions «if» avec des expressions conditionnelles identiques. La première instruction «if» contient un retour de méthode. Cela signifie que la deuxième instruction 'if' est insensée 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(":"))
{
....
}
....
}
Je suis sûr que vous ne pouvez pas trouver une erreur ici avec vos yeux. L'analyseur a trouvé une vérification inutile, qui s'est avérée être le code copié d'en haut. Au lieu de la variable str , la variable sLink doit être vérifiée .
Avertissement 4
V3004 L' instruction 'then' est équivalente à l'instruction '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();
}
....
}
L'analyseur a détecté un code Copy-Paste très descriptif. Peut-être, dans un cas, la variable de chemins doit être calculée de manière récursive, mais cela n'a pas été fait.
Attention 5
V3009 Il est étrange que cette méthode retourne toujours une seule et même valeur de «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;
}
La taille de cette fonction est de 135 lignes. Même les développeurs eux-mêmes ont laissé un commentaire selon lequel cela devrait être simplifié. Vous devez absolument travailler avec le code de fonction, car il renvoie également une valeur dans tous les cas.
Appels de fonction inutiles
Avertissement 1
V3010 La valeur de retour de la fonction «Distinct» doit être utilisée. DbTenantService.cs 132
public IEnumerable<Tenant> GetTenants(string login, string passwordHash)
{
//new password
result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();
result.Distinct();
....
}
La méthode Distinct supprime les doublons de la collection. Mais en C #, la plupart de ces méthodes d'extension ne modifient pas l'objet, mais créent une copie. Ainsi, dans cet exemple, la liste de résultats reste la même qu'avant l'appel de méthode. Vous pouvez également voir les noms login et passwordHash ici . C'est peut-être un autre problème de sécurité.
Avertissement 2
V3010 La valeur de retour de la fonction 'ToString' doit être utilisée. 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);
}
....
}
La méthode ToString est standard ici. Il renvoie la représentation textuelle de l'objet, mais la valeur de retour n'est pas utilisée.
Avertissement 3
V3010 La valeur de retour de la fonction «Remplacer» doit être utilisée. TextFileUserImporter.cs 252
private int GetFieldsMapping(....)
{
....
if (NameMapping != null && NameMapping.ContainsKey(propertyField))
{
propertyField = NameMapping[propertyField];
}
propertyField.Replace(" ", "");
....
}
Quelqu'un a commis une grave erreur. Tous les espaces ont dû être supprimés de la propriété propertyField , mais cela ne se produit pas, car la fonction Remplacer ne modifie pas l'objet d'origine.
Avertissement 4
V3038 L' argument '"yy"' a été passé plusieurs fois à la méthode 'Replace'. Il est possible qu'un autre argument soit passé à la place. 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("'", "")
;
}
Ici, les appels aux fonctions Replace sont écrits correctement, mais à un endroit, cela se fait avec d'étranges arguments identiques.
Potential NullReferenceException
Avertissement 1
L' expression V3022 'portalUser.BirthDate.ToString ()' n'est toujours pas nulle. L'opérateur '??' est excessif. 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 ne sera pas nul . La vérification a été effectuée ici afin de sortir la valeur "NULL" dans le journal de débogage si la date n'est pas définie. Mais depuis en l'absence de valeur, la méthode ToString renverra une chaîne vide, puis l'erreur dans l'algorithme peut être moins perceptible dans les journaux.
La liste complète des lieux de journalisation douteux ressemble à ceci:
- L'expression V3022 'ldapUser.BirthDate.ToString ()' n'est toujours pas nulle. L'opérateur '??' est excessif. LdapUserManager.cs 437
- L'expression V3022 'portalUser.Sex.ToString ()' n'est toujours pas nulle. L'opérateur '??' est excessif. LdapUserManager.cs 444
- L'expression V3022 'ldapUser.Sex.ToString ()' n'est toujours pas nulle. L'opérateur '??' est excessif. LdapUserManager.cs 445
Avertissement 2
V3095 L'objet 'r.Attributes ["href"]' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 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");
}))
{
....
}
....
}
Lors de l'analyse de Html ou Xml, il est très dangereux de se référer aux attributs par leur nom sans validation. Ce bogue est particulièrement intéressant car la valeur de l'attribut href est d' abord récupérée puis vérifiée pour voir si elle est présente.
Avertissement 3
V3146 Déréférence nulle possible. Le 'listTags.FirstOrDefault' peut renvoyer une valeur nulle par défaut. FileMarker.cs 299
public static void RemoveMarkAsNew(....)
{
....
var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();
valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;
....
}
L'analyseur a détecté une utilisation non sécurisée du résultat de l'appel de la méthode FirstOrDefault . Cette méthode renvoie une valeur par défaut s'il n'y a aucun objet dans la liste qui satisfait le prédicat de recherche. La valeur par défaut des types référence est une référence nulle. Par conséquent, avant d'utiliser le lien résultant, il doit être vérifié et pas immédiatement appelé la propriété, comme ici.
Avertissement 4
V3115 Le passage de la méthode «null» à «Equals» ne doit pas entraîner «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);
}
....
}
Les références d'objets en C # sont souvent comparées à null . Par conséquent, lors de la surcharge des méthodes de comparaison, il est très important d'anticiper de telles situations et d'ajouter la vérification appropriée au début de la fonction. Mais ici, ils ne l'ont pas fait.
Autres bugs
Avertissement 1
V3022 L' expression est toujours vraie. L'opérateur '&&' devrait probablement être utilisé ici. 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();
....
}
L'appel de la méthode CreateItem lèvera une ArgumentException . Le fait est que la toute première expression conditionnelle contient une erreur. La condition est toujours évaluée à vrai . L'erreur réside dans le choix de l'opérateur logique. Vous devriez avoir utilisé l'opérateur &&.
Très probablement, cette méthode n'a encore jamais été appelée. il est virtuel et a toujours été redéfini dans les classes dérivées jusqu'à présent.
Afin d'éviter de telles erreurs à l'avenir, je recommande de lire mon article et d'enregistrer un lien vers celui-ci: " Expressions logiques. Comment les professionnels font des erreurs". Toutes les combinaisons erronées d'opérateurs logiques sont données et analysées.
Avertissement 2
V3052 L'objet d'exception d'origine 'ex' a été avalé. La pile de l'exception d'origine pourrait être perdue. 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;
}
}
Ici, nous avons converti une GoogleApiException en SecurityException , tout en perdant des informations de l'exception d'origine qui pourraient être utiles.
Un petit changement comme celui-ci rendra l'avertissement généré plus informatif:
throw new SecurityException(ex.Error.Message, ex);
Bien qu'il soit tout à fait possible que GoogleApiException ait été masqué exprès .
Avertissement 3
Le composant V3118 Minutes de TimeSpan est utilisé, ce qui ne représente pas l'intervalle de temps complet. Peut-être que la valeur «TotalMinutes» était destinée à la place. NotifyClient.cs 281
public static void SendAutoReminderAboutTask(DateTime scheduleDate)
{
....
var deadlineReminderDate = deadline.AddMinutes(-alertValue);
if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;
....
}
J'avais l'habitude de penser que les diagnostics étaient préventifs. Dans le code de mes projets, il donnait toujours de faux avertissements. Ici, je suis à peu près sûr qu'il y a eu un bug. Très probablement, vous devez utiliser la propriété TotalMinutes au lieu de Minutes .
Avertissement 4
V3008 La variable «clé» reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 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;
}
Le problème avec ce fragment est que lors de la saisie d'une fonction, un tableau d'octets est toujours créé, puis immédiatement écrasé. Ceux. il y a une allocation constante de mémoire qui n'a pas de sens.
Votre meilleur pari serait de passer en C # 8 au lieu du C # 5 utilisé et d'écrire un code plus court:
private byte[] GenerateKey()
{
using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
return deriveBytes.GetBytes(keyLength);
}
Je ne peux pas dire si le projet peut être amélioré ou non, mais il existe de nombreux endroits de ce type. Il est conseillé de les réécrire en quelque sorte:
- V3008 La variable 'hmacKey' reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 256, 252. Metadata.cs 256
- V3008 La variable 'hmacHash' reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 270, 264. Metadata.cs 270
- V3008 La variable «chemins» reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 512, 508. RackspaceCloudStorage.cs 512
- V3008 La variable «b» reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 265, 264. BookmarkingUserControl.ascx.cs 265
- V3008 La variable 'taskIds' reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 412, 391. TaskDao.cs 412
En dernier recours, vous n'avez pas besoin d'allouer de mémoire lors de la déclaration d'une variable.
Bug dans PVS-Studio
Vous pensez que nous n'écrivons que sur les erreurs des autres. Non, notre équipe est autocritique, admet ses erreurs et n'hésite pas à en parler aussi. Tout le monde a tort.
Au cours du travail sur l'article, nous avons trouvé un bug assez stupide. Nous reconnaissons et nous pressons de partager.
Code du même serveur de communauté:
private bool IsPhrase(string searchText)
{
return searchText.Contains(" ") || searchText.Contains("\r\n") ||
searchText.Contains("\n");
}
J'ai dû donner un avertissement complet à l'analyseur avant le code, comme cela se fait tout au long de l'article, mais c'est le problème. L'avertissement ressemble à ceci:
Les caractères de contrôle \ r et \ n ne sont pas échappés avant d'être imprimés dans la table.
Conclusion
Je n'ai pas rencontré de projet aussi intéressant depuis longtemps. Merci aux contributeurs ONLYOFFCE. Nous voulions vous contacter, mais il n'y a eu aucun retour.
Nous écrivons régulièrement des articles comme celui-ci . Ce genre a plus de dix ans. Par conséquent, les développeurs ne doivent pas prendre les critiques personnellement. Nous serons heureux de publier une version complète du rapport pour améliorer le projet ou de fournir une licence temporaire pour vérifier le projet. Et pas seulement au projet CommunityServer, mais à tous ceux qui le souhaitent pendant UN MOIS en utilisant le code promotionnel #onlyoffice .
De plus, les professionnels de la sécurité seront intéressés de savoir que nous soutenons activement la norme OWASP. Certains diagnostics sont déjà disponibles. Et bientôt, l'interface de l'analyseur sera améliorée pour rendre l'inclusion de l'une ou l'autre norme d'analyse de code encore plus pratique.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Svyatoslav Razmyslov. ONLYOFFICE Community Server: comment les bogues contribuent à l'émergence de problèmes de sécurité .