Las aplicaciones de red del lado del servidor rara vez aparecen en nuestros informes de errores de código abierto. Probablemente esto se deba a su popularidad. Después de todo, tratamos de prestar atención a los proyectos que los propios lectores nos ofrecen. Y los servidores suelen realizar funciones muy importantes, pero sus actividades y beneficios permanecen invisibles para la mayoría de los usuarios. Entonces, por pura casualidad, se verificó el código del servidor de la comunidad de ONLYOFFICE. Resultó ser una reseña muy divertida.
Introducción
ONLYOFFICE Community Server es un sistema de colaboración de código abierto gratuito diseñado para administrar documentos, proyectos, interacciones con clientes y comunicaciones por correo electrónico en un solo lugar. En su sitio web, la empresa enfatiza la seguridad de sus soluciones con frases como "Ejecute su oficina privada con ONLYOFFICE" y "Aplicaciones seguras de oficina y productividad". Pero, aparentemente, no se utilizan herramientas para el control de calidad del código en el proceso de desarrollo.
Todo comenzó con el hecho de que revisé el código fuente de varias aplicaciones de red en busca de inspiración para la implementación de una de mis ideas de aplicación. El analizador PVS-Studio se estaba ejecutando en segundo plano. y arrojé errores graciosos en el chat corporativo general.
Entonces, algunos ejemplos fueron a Twitter :
Los representantes luego comentaron sobre el tweet e incluso más tarde publicaron una negación del problema:
Es muy probable que esto sea cierto. Pero esto no suma puntos a la calidad del proyecto. Veamos qué más se encontró allí.
Entrada de validación "asistente"
Algunos de los enfoques del desarrollador para validar los datos de entrada son sorprendentes por su originalidad.
Advertencia 1
La expresión V3022 'string.IsNullOrEmpty ("contraseña")' siempre es falsa. 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;
}
Como puede ver, este código marca el tono de todo el artículo. Se puede describir con la frase "El código es divertido, pero la situación es terrible". Probablemente tenga que cansarse mucho para confundir la variable de contraseña con la cadena "contraseña" . Este error le permite continuar ejecutando el código con una contraseña vacía. Según el autor del código, la contraseña también se verifica en la interfaz del programa. Pero el proceso de programación está diseñado de tal manera que las funciones previamente escritas a menudo se reutilizan. Por lo tanto, este error puede manifestarse en cualquier parte del futuro. Recuerde siempre la importancia de detectar errores en su código temprano.
Advertencia 2
V3022 La expresión 'String.IsNullOrEmpty ("nombre")' siempre es falsa. SendInterceptorSkeleton. cs 36
La expresión V3022 'String.IsNullOrEmpty ("sendInterceptor")' siempre es falsa. 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;
}
De repente, hubo una serie de errores similares en el código. Si al principio fue gracioso, ahora vale la pena pensar en las razones para escribir tal código. Quizás este hábito se mantuvo después de cambiar de otro lenguaje de programación. En C ++, los antiguos programadores de Python suelen cometer errores en nuestra experiencia de comprobación de proyectos de C ++.
Advertencia 3
V3022 La expresión 'id <0' siempre es falsa. El valor de tipo sin signo es siempre> = 0. UserFolderEngine.cs 173
public MailUserFolderData Update(uint id, string name, uint? parentId = null)
{
if (id < 0)
throw new ArgumentException("id");
....
}
La variable id es de tipo uint sin firmar . Por lo tanto, la verificación no tiene sentido aquí. Vale la pena prestar atención a llamar a esta función. Me pregunto qué se está pasando a esta función. Lo más probable es que el tipo int firmado se haya utilizado en todas partes , pero después de la refactorización, la comprobación se mantuvo.
Copiar y pegar código
Advertencia 1
V3001 Hay subexpresiones idénticas 'searchFilterData.WithCalendar == WithCalendar' a la izquierda y a la derecha del operador '&&'. MailSearchFilterData.cs 131
Este código tenía que presentarse en forma de imagen para transmitir la escala de la expresión condicional escrita. Hay un área problemática en ella. Especificar un lugar en la advertencia del analizador difícilmente puede ayudarlo a encontrar 2 comprobaciones idénticas. Por tanto, usaremos el marcador rojo:
Y aquí están los mismos condicionales sobre los que advirtió el analizador. Además de arreglar este punto, le recomendaría que diseñe mejor el código para que no contribuya a tales errores en el futuro.
Advertencia 2
V3030 Comprobación recurrente. La condición '! String.IsNullOrEmpty (user)' ya se verificó en la línea 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 cadena de usuario se comprueba 2 veces seguidas de la misma forma. Quizás este código se pueda refactorizar un poco. Aunque quién sabe, quizás en uno de los casos quisieron comprobar la variable booleana absoluta .
Advertencia 3
V3021 Hay dos declaraciones 'if' con expresiones condicionales idénticas. La primera instrucción 'if' contiene el método return. Esto significa que la segunda declaración 'si' no tiene sentido 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(":"))
{
....
}
....
}
Estoy seguro de que no puede encontrar un error aquí con sus ojos. El analizador encontró un cheque inútil, que resultó ser el código copiado de arriba. En lugar de la variable str , se debe verificar la variable sLink .
Advertencia 4
V3004 La instrucción 'then' es equivalente a la instrucción '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();
}
....
}
El analizador ha detectado un código Copiar-Pegar muy descriptivo. Quizás, en un caso, la variable de rutas debe calcularse de forma recursiva, pero esto no se ha hecho.
Advertencia 5
V3009 Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. 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;
}
El tamaño de esta función es de 135 líneas. Incluso los propios desarrolladores dejaron un comentario de que debería simplificarse. Definitivamente necesitas trabajar con el código de función, porque también devuelve un valor en todos los casos.
Llamadas a funciones inútiles
Advertencia 1
V3010 Se requiere que se utilice el valor de retorno de la función 'Distinto'. DbTenantService.cs 132
public IEnumerable<Tenant> GetTenants(string login, string passwordHash)
{
//new password
result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();
result.Distinct();
....
}
El método Distinct elimina los duplicados de la colección. Pero en C #, la mayoría de estos métodos de extensión no modifican el objeto, sino que crean una copia. Por tanto, en este ejemplo, la lista de resultados sigue siendo la misma que antes de la llamada al método. También puede ver los nombres de inicio de sesión y contraseñaHash aquí . Quizás este sea otro problema de seguridad.
Advertencia 2
V3010 Se requiere que se utilice el valor de retorno de la función '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);
}
....
}
El método ToString es estándar aquí. Devuelve la representación textual del objeto, pero no se utiliza el valor devuelto.
Advertencia 3
V3010 Se requiere que se utilice el valor de retorno de la función 'Reemplazar'. TextFileUserImporter.cs 252
private int GetFieldsMapping(....)
{
....
if (NameMapping != null && NameMapping.ContainsKey(propertyField))
{
propertyField = NameMapping[propertyField];
}
propertyField.Replace(" ", "");
....
}
Alguien cometió un grave error. Todos los espacios tuvieron que ser eliminados de la propiedad propertyField , pero esto no sucede, ya que la función Reemplazar no cambia el objeto original.
Advertencia 4
V3038 El argumento '"yy"' se pasó al método 'Reemplazar' varias veces. Es posible que en su lugar se deba pasar otro argumento. 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("'", "")
;
}
Aquí las llamadas a las funciones Reemplazar están escritas correctamente, pero en un lugar se hace con argumentos idénticos extraños.
Potential NullReferenceException
Advertencia 1
La expresión V3022 'portalUser.BirthDate.ToString ()' no siempre es nula. El operador '??' es excesivo. 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 no será nulo . La verificación se realizó aquí para generar el valor "NULL" en el registro de depuración si no se establece la fecha. Pero desde en ausencia de un valor, el método ToString devolverá una cadena vacía, entonces el error en el algoritmo puede ser menos notorio en los registros.
La lista completa de lugares de registro cuestionables se ve así:
- La expresión V3022 'ldapUser.BirthDate.ToString ()' no siempre es nula. El operador '??' es excesivo. LdapUserManager.cs 437
- La expresión V3022 'portalUser.Sex.ToString ()' no siempre es nula. El operador '??' es excesivo. LdapUserManager.cs 444
- La expresión V3022 'ldapUser.Sex.ToString ()' no siempre es nula. El operador '??' es excesivo. LdapUserManager.cs 445
Advertencia 2
V3095 El objeto 'r.Attributes ["href"]' se usó antes de que se verificara con un valor nulo. Compruebe las líneas: 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");
}))
{
....
}
....
}
Al analizar Html o Xml, es muy peligroso referirse a los atributos por su nombre sin validación. Este error es especialmente atractivo porque el valor del atributo href se recupera primero y luego se verifica para ver si está presente.
Advertencia 3
V3146 Posible desreferencia nula. 'ListTags.FirstOrDefault' puede devolver un valor nulo predeterminado. FileMarker.cs 299
public static void RemoveMarkAsNew(....)
{
....
var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();
valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;
....
}
El analizador ha detectado un uso inseguro del resultado de llamar al método FirstOrDefault . Este método devuelve un valor predeterminado si no hay objetos en la lista que satisfagan el predicado de búsqueda. El valor predeterminado para los tipos de referencia es una referencia nula. En consecuencia, antes de usar el enlace resultante, se debe verificar y no llamar inmediatamente a la propiedad, como aquí.
Advertencia 4
V3115 Pasar 'null' a 'Equals' no debería resultar en '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);
}
....
}
Las referencias a objetos en C # a menudo se comparan con nulos . Por lo tanto, cuando se sobrecargan los métodos de comparación, es muy importante anticipar tales situaciones y agregar las verificaciones apropiadas al comienzo de la función. Pero aquí no lo hicieron.
Otros errores
Advertencia 1
La expresión V3022 es siempre verdadera. Probablemente el operador '&&' debería usarse aquí. 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();
....
}
Llamar al método CreateItem arrojará una ArgumentException . El punto es que la primera expresión condicional contiene un error. La condición siempre se evalúa como verdadera . El error radica en la elección del operador lógico. Debería haber utilizado el operador &&.
Lo más probable es que este método aún no haya sido llamado. es virtual y siempre se ha redefinido en clases derivadas hasta ahora.
Para evitar este tipo de errores en el futuro, recomiendo leer mi artículo y guardar un enlace: " Expresiones lógicas. Cómo cometen errores los profesionales". Se proporcionan y analizan todas las combinaciones erróneas de operadores lógicos.
Advertencia 2
V3052 Se tragó el objeto de excepción original 'ex'. Se podría perder la pila de la excepción original. 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;
}
}
Aquí hemos convertido la GoogleApiException en una SecurityException , al tiempo que perdimos información útil de la excepción original.
Un pequeño cambio como este hará que la advertencia generada sea más informativa:
throw new SecurityException(ex.Error.Message, ex);
Aunque es muy posible que GoogleApiException se haya ocultado a propósito .
Advertencia 3 Se
utiliza el componente de minutos V3118 de TimeSpan, que no representa el intervalo de tiempo completo. Posiblemente en su lugar se pretendía el valor 'TotalMinutes'. NotifyClient.cs 281
public static void SendAutoReminderAboutTask(DateTime scheduleDate)
{
....
var deadlineReminderDate = deadline.AddMinutes(-alertValue);
if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;
....
}
Solía pensar que el diagnóstico era proactivo. En el código de mis proyectos, siempre daba falsas advertencias. Aquí, estoy bastante seguro de que hubo un error. Lo más probable es que deba utilizar la propiedad TotalMinutes en lugar de Minutes .
Advertencia 4
V3008 A la variable 'clave' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 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;
}
El problema con este fragmento es que al ingresar funciones, siempre se crea una matriz de bytes y luego se sobrescribe inmediatamente. Aquellos. hay una asignación constante de memoria que no tiene sentido.
Su mejor opción sería cambiar a C # 8 en lugar del C # 5 usado y escribir un código más corto:
private byte[] GenerateKey()
{
using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);
return deriveBytes.GetBytes(keyLength);
}
No puedo decir si el proyecto se puede actualizar o no, pero hay muchos lugares de este tipo. Es aconsejable reescribirlos de alguna manera:
- V3008 A la variable 'hmacKey' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 256, 252. Metadata.cs 256
- V3008 A la variable 'hmacHash' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 270, 264. Metadata.cs 270
- V3008 A la variable 'rutas' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 512, 508. RackspaceCloudStorage.cs 512
- V3008 A la variable 'b' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 265, 264. BookmarkingUserControl.ascx.cs 265
- V3008 A la variable 'taskIds' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 412, 391. TaskDao.cs 412
Como último recurso, no necesita asignar memoria al declarar una variable.
Error en PVS-Studio
Crees que solo escribimos sobre los errores de los demás. No, nuestro equipo es autocrítico, admite sus errores y no duda en escribir sobre ellos también. Todo el mundo está equivocado.
En el curso del trabajo en el artículo, encontramos un error bastante estúpido. Reconocemos y nos apresuramos a compartir.
Código del mismo servidor de comunidades:
private bool IsPhrase(string searchText)
{
return searchText.Contains(" ") || searchText.Contains("\r\n") ||
searchText.Contains("\n");
}
Tuve que dar una advertencia completa al analizador antes del código, como se hace a lo largo del artículo, pero este es el problema. La advertencia se ve así:
Los caracteres de control \ r y \ n no se escapan antes de imprimirse en la tabla.
Conclusión
No me he encontrado con un proyecto tan interesante desde hace mucho tiempo. Gracias a los colaboradores de ONLYOFFCE. Queríamos comunicarnos contigo, pero no hubo comentarios.
Escribimos artículos como este de forma regular . Este género tiene más de diez años. Por lo tanto, los desarrolladores no deben tomarse las críticas como algo personal. Estaremos encantados de emitir una versión completa del informe para mejorar el proyecto o proporcionar una licencia temporal para verificar el proyecto. Y no solo para el proyecto CommunityServer, sino para todos los que lo deseen durante UN MES utilizando el código de promoción #onlyoffice .
Además, los profesionales de la seguridad estarán interesados en saber que apoyamos activamente el estándar OWASP. Algunos diagnósticos ya están disponibles. Y pronto se mejorará la interfaz del analizador para que la inclusión de uno u otro estándar para el análisis de código sea aún más conveniente.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Svyatoslav Razmyslov. ONLYOFFICE Community Server: cómo los errores contribuyen a la aparición de problemas de seguridad .