Serverseitige Netzwerkanwendungen erscheinen selten in unseren Open Source-Fehlerberichten. Dies ist wahrscheinlich auf ihre Popularität zurückzuführen. Schließlich versuchen wir, auf die Projekte zu achten, die uns die Leser selbst anbieten. Und Server führen oft sehr wichtige Funktionen aus, aber ihre Aktivitäten und Vorteile bleiben für die meisten Benutzer unsichtbar. Also wurde rein zufällig der ONLYOFFICE Community Server-Code überprüft. Es stellte sich heraus, dass es eine sehr lustige Bewertung war.
Einführung
ONLYOFFICE Community Server ist ein kostenloses Open-Source-Kollaborationssystem, mit dem Dokumente, Projekte, Kundeninteraktionen und E-Mail-Kommunikation an einem Ort verwaltet werden können. Auf seiner Website betont das Unternehmen die Sicherheit seiner Lösungen mit Begriffen wie "Führen Sie Ihr privates Büro mit dem ONLYOFFICE" und "Sichere Büro- und Produktivitäts-Apps". Offensichtlich werden im Entwicklungsprozess jedoch keine Tools zur Qualitätskontrolle des Codes verwendet.
Alles begann damit, dass ich den Quellcode mehrerer Netzwerkanwendungen durchgesehen habe, um Inspiration für die Umsetzung einer meiner Anwendungsideen zu finden. Der PVS-Studio- Analysator wurde im Hintergrund ausgeführt und ich warf lustige Fehler in den allgemeinen Unternehmenschat.
Einige Beispiele gingen an Twitter :
Vertreter kommentierten später den Tweet und posteten sogar später eine Ablehnung des Problems:
Dies ist höchstwahrscheinlich wahr. Dies trägt jedoch nicht zur Qualität des Projekts bei. Mal sehen, was dort noch gefunden wurde.
Eingabevalidierungsassistent
Einige der Ansätze des Entwicklers zur Validierung von Eingabedaten sind in ihrer Originalität bemerkenswert.
Warnung 1
V3022 Ausdruck 'string.IsNullOrEmpty ("password")' ist immer falsch. 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;
}
Wie Sie sehen können, gibt dieser Code den Ton für den gesamten Artikel an. Es kann mit dem Satz "Der Code ist lustig, aber die Situation ist schrecklich." Beschrieben werden. Sie müssen wahrscheinlich sehr müde werden, um die Kennwortvariable mit der Zeichenfolge "Kennwort" zu verwechseln . Mit diesem Fehler können Sie den Code mit einem leeren Kennwort weiter ausführen. Laut Autor des Codes wird das Passwort zusätzlich in der Programmoberfläche überprüft. Der Programmierprozess ist jedoch so konzipiert, dass zuvor geschriebene Funktionen häufig wiederverwendet werden. Daher kann sich dieser Fehler überall in der Zukunft manifestieren. Denken Sie immer daran, wie wichtig es ist, Fehler in Ihrem Code frühzeitig zu erkennen.
Warnung 2
V3022 Ausdruck 'String.IsNullOrEmpty ("name")' ist immer falsch. SendInterceptorSkeleton. cs 36
V3022 Ausdruck 'String.IsNullOrEmpty ("sendInterceptor")' ist immer falsch. 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;
}
Plötzlich gab es eine Reihe ähnlicher Fehler im Code. Wenn es zuerst lustig war, lohnt es sich jetzt, über die Gründe für das Schreiben eines solchen Codes nachzudenken. Vielleicht blieb diese Gewohnheit nach dem Wechsel von einer anderen Programmiersprache bestehen. In C ++ werden Fehler von ehemaligen Python-Programmierern bei der Überprüfung von C ++ - Projekten häufig vorgebracht.
Warnung 3
V3022 Der Ausdruck 'id <0' ist immer falsch. Der vorzeichenlose Typwert ist immer> = 0. UserFolderEngine.cs 173
public MailUserFolderData Update(uint id, string name, uint? parentId = null)
{
if (id < 0)
throw new ArgumentException("id");
....
}
Die ID- Variable ist vom Typ uint ohne Vorzeichen . Daher ist eine Überprüfung hier sinnlos. Es lohnt sich, diese Funktion aufzurufen. Ich frage mich, was an diese Funktion übergeben wird. Höchstwahrscheinlich wurde der signierte Typ int überall verwendet , aber nach dem Refactoring blieb die Prüfung bestehen.
Code kopieren und einfügen
Warnung 1
V3001 Links und rechts vom Operator '&&' befinden sich identische Unterausdrücke 'searchFilterData.WithCalendar == WithCalendar'. MailSearchFilterData.cs 131
Dieser Code musste in Form eines Bildes dargestellt werden, um den Maßstab des schriftlichen bedingten Ausdrucks zu vermitteln. Es gibt einen Problembereich. Wenn Sie einen Platz in der Warnung des Analysators angeben, können Sie kaum zwei identische Prüfungen finden. Daher verwenden wir die rote Markierung:
Und hier sind die gleichen Bedingungen, vor denen der Analysator gewarnt hat. Zusätzlich zur Behebung dieses Punktes würde ich empfehlen, dass Sie den Code besser formatieren, damit er in Zukunft nicht zu solchen Fehlern beiträgt.
Warnung 2
V3030 Wiederkehrende Prüfung. Die Bedingung '! String.IsNullOrEmpty (user)' wurde bereits in Zeile 173 überprüft. 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] == '-')
{
....
}
Die Benutzerzeichenfolge wird auf dieselbe Weise zweimal hintereinander überprüft. Vielleicht kann dieser Code ein wenig überarbeitet werden. Obwohl wer weiß, wollten sie vielleicht in einem der Fälle die boolesche Variable absolut überprüfen .
Warnung 3
V3021 Es gibt zwei ' if' -Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Methodenrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. 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(":"))
{
....
}
....
}
Ich bin sicher, dass Sie hier mit Ihren Augen keinen Fehler finden können. Der Analysator fand eine nutzlose Prüfung, die sich als kopierter Code von oben herausstellte. Anstelle der Variablen str sollte die Variable sLink überprüft werden .
Warnung 4
V3004 Die Anweisung 'then' entspricht der Anweisung '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();
}
....
}
Der Analysator hat einen sehr beschreibenden Code zum Kopieren und Einfügen erkannt. In einem Fall muss die Pfadvariable möglicherweise rekursiv berechnet werden, dies wurde jedoch nicht durchgeführt.
Warnung 5
V3009 Es ist seltsam, dass diese Methode immer ein und denselben Wert von 'true' zurückgibt. 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;
}
Die Größe dieser Funktion beträgt 135 Zeilen. Sogar die Entwickler selbst haben einen Kommentar hinterlassen, dass es vereinfacht werden sollte. Sie müssen auf jeden Fall mit dem Funktionscode arbeiten, weil In allen Fällen wird auch ein Wert zurückgegeben.
Nutzlose Funktionsaufrufe
Warnung 1
V3010 Der Rückgabewert der Funktion 'Distinct' muss verwendet werden. DbTenantService.cs 132
public IEnumerable<Tenant> GetTenants(string login, string passwordHash)
{
//new password
result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();
result.Distinct();
....
}
Die Distinct- Methode entfernt Duplikate aus der Sammlung. In C # ändern die meisten dieser Erweiterungsmethoden das Objekt jedoch nicht, sondern erstellen eine Kopie. In diesem Beispiel bleibt die Ergebnisliste also dieselbe wie vor dem Methodenaufruf. Sie können hier auch die Namen login und passwordHash sehen . Vielleicht ist dies ein weiteres Sicherheitsproblem.
Warnung 2
V3010 Der Rückgabewert der Funktion 'ToString' muss verwendet werden. 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);
}
....
}
Die ToString- Methode ist hier Standard. Es gibt die Textdarstellung des Objekts zurück, aber der Rückgabewert wird nicht verwendet.
Warnung 3
V3010 Der Rückgabewert der Funktion 'Ersetzen' muss verwendet werden. TextFileUserImporter.cs 252
private int GetFieldsMapping(....)
{
....
if (NameMapping != null && NameMapping.ContainsKey(propertyField))
{
propertyField = NameMapping[propertyField];
}
propertyField.Replace(" ", "");
....
}
Jemand hat einen schweren Fehler gemacht. Alle Leerzeichen mussten aus der Eigenschaft propertyField entfernt werden , dies geschieht jedoch nicht, da Die Funktion Ersetzen ändert das ursprüngliche Objekt nicht.
Warnung 4
V3038 Das Argument '"yy"' wurde mehrmals an die Methode 'Replace' übergeben. Es ist möglich, dass stattdessen ein anderes Argument übergeben wird. 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("'", "")
;
}
Hier sind die Aufrufe der Replace- Funktionen korrekt geschrieben, aber an einer Stelle erfolgt dies mit seltsam identischen Argumenten.
Mögliche NullReferenceException
Warnung 1
V3022 Der Ausdruck 'portalUser.BirthDate.ToString ()' ist immer nicht null. Der Betreiber '??' ist übertrieben. 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 ist nicht null . Die Überprüfung wurde hier durchgeführt, um den Wert "NULL" in das Debug-Protokoll auszugeben, wenn das Datum nicht festgelegt ist. Aber seit Wenn kein Wert vorhanden ist, gibt die ToString- Methode eine leere Zeichenfolge zurück. In den Protokollen ist der Fehler im Algorithmus möglicherweise weniger auffällig.
Die gesamte Liste der fragwürdigen Protokollierungsorte sieht folgendermaßen aus:
- V3022 Der Ausdruck 'ldapUser.BirthDate.ToString ()' ist immer nicht null. Der Betreiber '??' ist übertrieben. LdapUserManager.cs 437
- V3022 Der Ausdruck 'portalUser.Sex.ToString ()' ist immer nicht null. Der Betreiber '??' ist übertrieben. LdapUserManager.cs 444
- V3022 Der Ausdruck 'ldapUser.Sex.ToString ()' ist immer nicht null. Der Betreiber '??' ist übertrieben. LdapUserManager.cs 445
Warnung 2
V3095 Das Objekt 'r.Attributes ["href"]' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen: 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");
}))
{
....
}
....
}
Beim Parsen von HTML oder XML ist es sehr gefährlich, Attribute ohne Validierung namentlich zu referenzieren. Dieser Fehler ist besonders attraktiv, da der Wert des href- Attributs zuerst abgerufen und dann überprüft wird, um festzustellen, ob er überhaupt vorhanden ist.
Warnung 3
V3146 Mögliche Null-Dereferenzierung. Der 'listTags.FirstOrDefault' kann den Standardwert Null zurückgeben. FileMarker.cs 299
public static void RemoveMarkAsNew(....)
{
....
var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();
valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;
....
}
Der Analysator hat eine unsichere Verwendung des Ergebnisses des Aufrufs der FirstOrDefault- Methode festgestellt . Diese Methode gibt einen Standardwert zurück, wenn die Liste keine Objekte enthält, die das Suchprädikat erfüllen. Der Standardwert für Referenztypen ist eine Nullreferenz. Dementsprechend sollte vor der Verwendung des resultierenden Links dieser überprüft und nicht wie hier sofort als Eigenschaft bezeichnet werden.
Warnung 4
V3115 Die Übergabe von 'null' an die Methode 'Equals' sollte nicht zu 'NullReferenceException' führen. 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);
}
....
}
Objektreferenzen in C # werden häufig mit null verglichen . Daher ist es beim Überladen von Vergleichsmethoden sehr wichtig, solche Situationen zu antizipieren und zu Beginn der Funktion geeignete Überprüfungen hinzuzufügen. Aber hier haben sie es nicht getan.
Andere Fehler
Warnung 1
V3022 Ausdruck ist immer wahr. Wahrscheinlich sollte hier der Operator '&&' verwendet werden. 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();
....
}
Durch Aufrufen der CreateItem- Methode wird eine ArgumentException ausgelöst . Der Punkt ist, dass der allererste bedingte Ausdruck einen Fehler enthält. Die Bedingung wird immer als wahr ausgewertet . Der Fehler liegt in der Wahl des logischen Operators. Sie sollten den Operator && verwendet haben.
Höchstwahrscheinlich wurde diese Methode noch nie aufgerufen. Es ist virtuell und wurde bisher in abgeleiteten Klassen immer neu definiert.
Um solche Fehler in Zukunft zu vermeiden, empfehle ich, meinen Artikel zu lesen und einen Link dazu zu führen: " Logische Ausdrücke. Wie Profis Fehler machen". Alle fehlerhaften Kombinationen logischer Operatoren werden angegeben und analysiert.
Warnung 2
V3052 Das ursprüngliche Ausnahmeobjekt 'ex' wurde verschluckt. Der Stapel der ursprünglichen Ausnahme kann verloren gehen. 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;
}
}
Hier haben wir eine GoogleApiException in eine SecurityException konvertiert und dabei Informationen aus der ursprünglichen Ausnahme verloren, die möglicherweise nützlich sind.
Eine kleine Änderung wie diese macht die generierte Warnung informativer:
throw new SecurityException(ex.Error.Message, ex);
Obwohl es durchaus möglich ist, dass die GoogleApiException absichtlich ausgeblendet wurde .
Warnung 3 Es wird die
V3118- Minuten-Komponente von TimeSpan verwendet, die kein vollständiges Zeitintervall darstellt. Möglicherweise war stattdessen der Wert 'TotalMinutes' vorgesehen. NotifyClient.cs 281
public static void SendAutoReminderAboutTask(DateTime scheduleDate)
{
....
var deadlineReminderDate = deadline.AddMinutes(-alertValue);
if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;
....
}
Früher dachte ich, die Diagnose sei proaktiv. Im Code meiner Projekte gab es immer falsche Warnungen. Hier bin ich mir ziemlich sicher, dass es einen Fehler gab. Höchstwahrscheinlich sollten Sie die TotalMinutes- Eigenschaft anstelle von Minutes verwenden .
Warnung 4
V3008 Der Variablen 'key' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 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;
}
Das Problem mit diesem Fragment besteht darin, dass bei der Eingabe von Funktionen immer ein Array von Bytes erstellt und dann sofort überschrieben wird. Jene. Es gibt eine konstante Speicherzuweisung, die keinen Sinn ergibt.
Am besten wechseln Sie zu C # 8 anstelle des verwendeten C # 5 und schreiben kürzeren Code:
private byte[] GenerateKey()
{
using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
return deriveBytes.GetBytes(keyLength);
}
Ich kann nicht sagen, ob das Projekt aktualisiert werden kann oder nicht, aber es gibt viele solcher Orte. Es ist ratsam, sie irgendwie umzuschreiben:
- V3008 Der Variablen 'hmacKey' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 256, 252. Metadata.cs 256
- V3008 Der Variablen 'hmacHash' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 270, 264. Metadata.cs 270
- V3008 Der Variablen 'Pfade' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 512, 508. RackspaceCloudStorage.cs 512
- V3008 Der Variablen 'b' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 265, 264. BookmarkingUserControl.ascx.cs 265
- V3008 Der Variablen 'taskIds' werden zweimal nacheinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 412, 391. TaskDao.cs 412
Als letzten Ausweg müssen Sie beim Deklarieren einer Variablen keinen Speicher zuweisen.
Fehler in PVS-Studio
Sie denken, wir schreiben nur über die Fehler anderer. Nein, unser Team ist selbstkritisch, gibt seine Fehler zu und zögert nicht, auch darüber zu schreiben. Jeder ist falsch.
Während der Arbeit an dem Artikel haben wir einen ziemlich dummen Fehler gefunden. Wir erkennen an und beeilen uns zu teilen.
Code vom selben Community Server:
private bool IsPhrase(string searchText)
{
return searchText.Contains(" ") || searchText.Contains("\r\n") ||
searchText.Contains("\n");
}
Ich musste vor dem Code eine vollständige Analysewarnung geben, wie im gesamten Artikel beschrieben, aber das ist das Problem. Die Warnung sieht folgendermaßen aus:
Die Steuerzeichen \ r und \ n werden vor dem Drucken in die Tabelle nicht maskiert.
Fazit
Ich bin schon lange nicht mehr auf ein so interessantes Projekt gestoßen. Vielen Dank an die ONLYOFFCE-Mitwirkenden. Wir wollten Sie kontaktieren, aber es gab kein Feedback.
Wir schreiben regelmäßig solche Artikel . Dieses Genre ist über zehn Jahre alt. Entwickler sollten Kritik daher nicht persönlich nehmen. Gerne veröffentlichen wir eine Vollversion des Berichts, um das Projekt zu verbessern, oder stellen eine temporäre Lizenz zur Überprüfung des Projekts zur Verfügung. Und nicht nur für das CommunityServer-Projekt, sondern für alle, die es für EINEN MONAT mit dem Promo-Code #onlyoffice wünschen .
Auch Sicherheitsexperten werden interessiert sein zu wissen, dass wir den OWASP-Standard aktiv unterstützen. Einige Diagnosen sind bereits verfügbar. Und bald wird die Analysatorschnittstelle verbessert, um die Aufnahme des einen oder anderen Standards für die Codeanalyse noch komfortabler zu machen.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Svyatoslav Razmyslov. ONLYOFFICE Community Server: Wie Fehler zur Entstehung von Sicherheitsproblemen beitragen .