NSA, Ghidra y Unicornios

NSA, Ghidra y Unicornios

Esta vez, el equipo de PVS-Studio se sintió atraído por Ghidra, un marco grande y malvado para la ingeniería inversa con el que puede analizar varios archivos binarios y hacer todo tipo de cosas aterradoras con ellos. Lo más interesante de esto no es que sea de uso gratuito o se extienda bien con complementos, sino que fue escrito en la NSA y publicado en GitHub para todos. Por un lado, la NSA parece tener suficientes recursos para mantener limpia la base del código. Y, por otro lado, los nuevos contribuyentes que no estaban muy familiarizados con él podrían haber agregado accidentalmente errores no detectados últimamente. Por lo tanto, armados con un análisis estático, decidimos buscar debilidades en este proyecto.

Preludio


En total, el analizador estático PVS-Studio emitió 651 advertencias altas, 904 medias y 909 bajas en la parte Java del proyecto Ghidra ( versión 9.1.2, commit 687ce7f ). Entre ellos, aproximadamente la mitad de las respuestas altas y medias fueron desencadenadas por el diagnóstico V6022 .El parámetro no se usa dentro del cuerpo del método ", que generalmente aparece después de la refactorización, cuando algún parámetro ya no era necesario o alguna funcionalidad estaba temporalmente desactivada por los comentarios. Un vistazo rápido a estas advertencias (hay demasiadas para verlas como un observador externo) ) en este proyecto no reveló nada obviamente sospechoso. Probablemente esté permitido que este proyecto desactive temporalmente este diagnóstico en la configuración del analizador para no distraerse con él. En la práctica, a menudo puede ver errores tipográficos en el nombre del setter o parámetro del constructor y, en general, no debería Estoy seguro de que la mayoría de los lectores al menos una vez se encuentran con un patrón desagradable similar:

public class A {
  private String value;
  public A(String val) { // V6022
    this.value = value;
  }
  public int hashCode() {
    return value.hashCode(); // NullPointerException
  }
}

Más de la mitad de las advertencias bajas fueron hechas por el diagnóstico " V6008 Desreferencia potencial nula del diagnóstico 'variable'", por ejemplo, el valor File.getParentFile () se usa a menudo sin verificar nulo . Si el objeto de archivo en el que se llamó a este método se construyó sin una ruta absoluta, se devolverá nulo y la ausencia de verificación puede descartar la aplicación.

Por tradición, analizaremos solo las advertencias de niveles altos y medios, ya que la mayor parte de los errores reales están contenidos en ellos. Al trabajar con informes del analizador, siempre recomendamos analizar las advertencias en orden descendente de su fiabilidad.

A continuación, consideramos algunos fragmentos indicados por el analizador que me parecieron sospechosos o interesantes. La base del código del proyecto resultó ser de un tamaño impresionante, y es casi imposible encontrar dichos lugares manualmente.

Fragmento 1: validación rota


private boolean parseDataTypeTextEntry()
throws InvalidDataTypeException {
  ...
  try {
    newDataType = parser.parse(selectionField.getText(),
                               getDataTypeRootForCurrentText());
  }
  catch (CancelledException e) {
    return false;
  }
  if (newDataType != null) {
    if (maxSize >= 0
        && newDataType.getLength() > newDataType.getLength()) { // <=
      throw new InvalidDataTypeException("data-type larger than "
                                         + maxSize + " bytes");
    }
    selectionField.setSelectedValue(newDataType);
    return true;
  }
  return false;
}

Advertencia de PVS-Studio: V6001 Hay subexpresiones idénticas 'newDataType.getLength ()' a la izquierda y a la derecha del operador '>'. DataTypeSelectionEditor.java data66

Esta clase proporciona un componente gráfico para seleccionar un tipo de datos que admita la finalización automática. El desarrollador que usa este componente puede establecer el tamaño máximo permitido del tipo de datos seleccionado (a través del campo maxSize ) o hacerlo ilimitado estableciendo un valor negativo. Se asumió que al validar los datos ingresados, al exceder el límite se produce una excepción, que luego se captura en la pila de llamadas y se le muestra un mensaje al usuario.

Parece que el autor del componente estaba distraído justo en el momento de escribir esta prueba, o tal vez pensó en el significado de la vida, pero al final, la validación simplemente no funciona, ya que el número nunca puede ser mayor que él mismo y, en consecuencia, nos ignoramos de esta condición. Esto significa que este componente puede proporcionar datos no válidos.

Otro error similar se encontró en dos clases más: GuidUtil y NewGuid .

public class GuidUtil {
  ...
  public static GuidInfo parseLine(...) {
    ...
    long[] data = new long[4];
    ...
    if (isOK(data)) {
      if (!hasVersion) {
        return new GuidInfo(guidString, name, guidType);
      }
      return new VersionedGuidInfo(guidString, version, name, guidType);
    }
    return null;
  }
  ...
  private static boolean isOK(long[] data) {
    for (int i = 0; i < data.length; i++) {
      if ((data[i] != 0) || (data[i] != 0xFFFFFFFFL)) { // <=
        return true;
      }
    }
    return false;
  }
  ...
}

Advertencia PVS-Studio: V6007 La expresión 'datos [i]! = 0xFFFFFFFFL' siempre es verdadera. GuidUtil.java:200

El ciclo for del método isOK verifica que el mismo valor no sea igual a dos números diferentes al mismo tiempo. Si es así, el GUID se reconoce inmediatamente como válido. Es decir, el GUID no será válido solo si la matriz de datos está vacía, y esto nunca sucede, ya que el valor de la variable correspondiente se asigna solo una vez, al comienzo del método parseLine . Cuerpo del

método IsOKen ambas clases coincide por completo, lo que sugiere la idea de otra copia y pega de código incorrecto. No estoy seguro de qué es exactamente lo que el autor quería verificar, pero puedo suponer que este método debería solucionarse de la siguiente manera:

private static boolean isOK(long[] data) {
  for (int i = 0; i < data.length; i++) {
    if ((data[i] == 0) || (data[i] == 0xFFFFFFFFL)) {
      return false;
    }
  }
  return true;
}

Fragmento 2: ocultando excepciones


public void putByte(long offsetInMemBlock, byte b)
throws MemoryAccessException, IOException {
  long offsetInSubBlock = offsetInMemBlock - subBlockOffset;
  try {
    if (ioPending) {
      new MemoryAccessException("Cyclic Access"); // <=
    }
    ioPending = true;
    doPutByte(mappedAddress.addNoWrap(offsetInSubBlock / 8),
              (int) (offsetInSubBlock % 8), b);
  }
  catch (AddressOverflowException e) {
    new MemoryAccessException("No memory at address"); // <=
  }
  finally {
    ioPending = false;
  }
}

Advertencia de PVS-Studio: V6006 El objeto se creó pero no se está utilizando. Podría faltar la palabra clave 'throw': 'new MemoryAccessException ("Cyclic Access")'. BitMappedSubMemoryBlock.java:99

Los objetos de excepción en sí mismos, como saben, no hacen nada (o al menos no deberían hacer nada). Casi siempre, sus nuevas instancias se lanzan a través del lanzamiento , en algunos casos raros, se transfieren a algún lugar o tal vez se colocan en una colección.

La clase que contiene este método es un contenedor sobre un bloque de memoria que permite leer y escribir datos. Aquí, debido al hecho de que no se lanzan excepciones, se puede violar la restricción impuesta de acceso al bloque de memoria actual con el indicador ioPendingy, además, se ignora una AddressOverflowException . Por lo tanto, los datos se pueden corromper silenciosamente y, en lugar de indicar explícitamente un error en un lugar en particular, el desarrollador recibirá artefactos extraños que el depurador debe analizar.

Hubo ocho de estas excepciones perdidas:

  • BitMappedSubMemoryBlock.java: líneas 77, 99, 106, 122
  • ByteMappedSubMemoryBlock.java: líneas 52, 73, 92, 114

Es característico que en los mismos archivos existen métodos extremadamente similares en los que throw está presente. Lo más probable es que un método se haya escrito originalmente de manera similar al fragmento anterior, después de lo cual se copió varias veces, de alguna manera encontró un error y lo corrigió en aquellos lugares que podían recordar.

Fragmento 3: campo minado


private void processSelection(OptionsTreeNode selectedNode) {
  if (selectedNode == null) {
    setViewPanel(defaultPanel, selectedNode); // <=
    return;
  }
  ...
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
  ...
  setHelpLocation(component, selectedNode);
  ...
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
  Options options = node.getOptions();
  ...
}

Advertencia de PVS-Studio: V6008 Desreferencia nula de 'selectedNode' en la función 'setViewPanel'. OptionsPanel.java:266

El analizador mentido un poco - por el momento, llamando a la processSelection método no no conduce a una NullPointerException , ya que este método se llama sólo dos veces, y antes de que se llama, SelectedNode se comprobó explícitamente nula . Esto no debe hacerse, ya que otro desarrollador puede ver que el método maneja explícitamente el caso selectedNode == null , y decide que este es un valor válido, lo que provocará el bloqueo de la aplicación. Tales sorpresas son especialmente peligrosas solo en proyectos abiertos, ya que las personas que no conocen la base del código participan a fondo en ellos.

En general, debo decir que todo el método processSelection parece bastante extraño. Es probable que se trate de un error de copiar y pegar, ya que en el mismo método se encuentra un bloque if con el mismo cuerpo dos veces más, aunque con diferentes condiciones. Sin embargo, en este punto, el Nodo seleccionado ya no será nulo y la cadena de llamada setViewPanel-setHelpLocation no dará como resultado una NullPointerException .

Fragmento 4: autocompletado para el mal


public static final int[] UNSUPPORTED_OPCODES_LIST = { ... };
public static final Set<Integer> UNSUPPORTED_OPCODES = new HashSet<>();

static {
  for (int opcode : UNSUPPORTED_OPCODES) {
    UNSUPPORTED_OPCODES.add(opcode);
  }
}

Advertencia de PVS-Studio: V6053 La colección se modifica mientras la iteración está en progreso. ConcurrentModificationException puede ocurrir. DWARFExpressionOpCodes.java:205

En este caso, el analizador volvió a mentir un poco: la excepción no se lanzará, ya que la colección UNSUPPORTED_OPCODES siempre está vacía y el bucle simplemente no se ejecutará. Además, sucedió que la colección es una multitud, y agregar un elemento ya presente no lo cambiará. Lo más probable es que el autor ingresó para cada unoel nombre de la colección mediante autocompletado y no se dio cuenta de que se propuso un campo incorrecto. La modificación de la colección durante la iteración es imposible, pero en buenas circunstancias, como en este caso, la aplicación puede no caer. Aquí, este error tiene un efecto indirecto: la máquina que analiza los archivos DWARF se basa en esta colección para detener el análisis al encontrar códigos de operación no compatibles.

Comenzando con Java 9, vale la pena usar los métodos de fábrica de la biblioteca estándar para colecciones constantes: por ejemplo, Set.of (T ... elements) no solo es mucho más conveniente, sino que también hace que la colección creada sea inmutable, lo que aumenta la confiabilidad del código.

Fragmento 5: hay de todo


public void setValueAt(Object aValue, int row, int column) {
  ...
  int index = indexOf(newName);
  if (index >= 0) {                  // <=
    Window window = tool.getActiveWindow();
    Msg.showInfo(getClass(), window, "Duplicate Name",
                 "Name already exists: " + newName);
    return;
  }

  ExternalPath path = paths.get(row); // <=
  ...
}
private int indexOf(String name) {
  for (int i = 0; i < paths.size(); i++) {
    ExternalPath path = paths.get(i);
    if (path.getName().equals(name)) {
      return i;
    }
  }
  return 0;
}

Advertencias de PVS-Studio:

  • V6007 La expresión 'index> = 0' siempre es verdadera. ExternalNamesTableModel.java:105
  • V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

El autor lo pensó y en el método indexOf en lugar de "index" -1 para un valor no detectado devuelve 0: el índice del primer elemento de la colección de rutas . Incluso si la colección está vacía. O tal vez se generó el método, pero olvidó cambiar el valor de retorno predeterminado. Como resultado, el método setValueAt descartará cualquier valor que se le pase y mostrará al usuario un error "El nombre ya existe", incluso si no hay un nombre existente.

Por cierto, indexOf no se usa en ningún otro lugar, y su valor solo se necesita para determinar si el elemento que está buscando existe. Quizás, en lugar de un método separado, escriba para cada uno directamente en setValueAt y haga una devoluciónen un elemento coincidente en lugar de juegos con índices.

Nota: No pude reproducir el supuesto error. El método setValueAt probablemente ya no se use o se llame solo bajo ciertas condiciones.

Fragmento 6: guarda silencio


final static Map<Character, String> DELIMITER_NAME_MAP = new HashMap<>(20);
// Any non-alphanumeric char can be used as a delimiter.
static {
  DELIMITER_NAME_MAP.put(' ', "Space");
  DELIMITER_NAME_MAP.put('~', "Tilde");
  DELIMITER_NAME_MAP.put('`', "Back quote");
  DELIMITER_NAME_MAP.put('@', "Exclamation point");
  DELIMITER_NAME_MAP.put('@', "At sign");
  DELIMITER_NAME_MAP.put('#', "Pound sign");
  DELIMITER_NAME_MAP.put('$', "Dollar sign");
  DELIMITER_NAME_MAP.put('%', "Percent sign");
  ...
}

Advertencia de PVS-Studio: V6033 Ya se ha agregado un elemento con la misma clave '@'. FilterOptions.java:45

Ghidra admite el filtrado de datos en varios contextos: por ejemplo, puede filtrar la lista de archivos de proyecto por nombre. Además, se implementa el filtrado por varias palabras clave a la vez: '.java, .c' cuando el modo 'OR' está activado, muestra todos los archivos cuyo nombre contiene '.java' o '.c'. Se entiende que cualquier carácter especial se puede utilizar como separador de palabras (se selecciona un separador específico en la configuración del filtro), pero en realidad el signo de exclamación no estaba disponible.

En tales hojas de inicialización, es muy fácil sellar, ya que a menudo se escriben usando copiar y pegar, y cuando mira dicho código, sus ojos se vuelven borrosos rápidamente. Y si el error tipográfico no estaba en dos líneas adyacentes, entonces a mano seguramente casi nadie lo verá.

Fragmento 7: el resto de la división siempre es 0


void setFactorys(FieldFactory[] fieldFactorys,
                 DataFormatModel dataModel, int margin) {
  factorys = new FieldFactory[fieldFactorys.length];

  int x = margin;
  int defaultGroupSizeSpace = 1;
  for (int i = 0; i < factorys.length; i++) {
    factorys[i] = fieldFactorys[i];
    factorys[i].setStartX(x);
    x += factorys[i].getWidth();
    // add in space between groups
    if (((i + 1) % defaultGroupSizeSpace) == 0) { // <=
      x += margin * dataModel.getUnitDelimiterSize();
    }
  }
  width = x - margin * dataModel.getUnitDelimiterSize() + margin;
  layoutChanged();
}

Advertencias de PVS-Studio:

  • V6007 La expresión '((i + 1)% defaultGroupSizeSpace) == 0' siempre es verdadera. ByteViewerLayoutModel.java:66
  • V6048 Esta expresión se puede simplificar. El operando 'defaultGroupSizeSpace' en la operación es igual a 1. ByteViewerLayoutModel.java:66

El visor de bytes hexadecimales admite la elección del tamaño de los grupos mostrados: por ejemplo, puede configurar la salida en el formato 'ffff ffff' o 'ff ff ff ff'. El método setFactorys es responsable de la ubicación de estos grupos en la interfaz de usuario . A pesar de que la personalización y la visualización funcionan correctamente, el ciclo en este método parece extremadamente sospechoso: el resto de la división por uno siempre es cero, lo que significa que la coordenada x aumentará en cada iteración. La sospecha agrega propiedades y disponibilidad groupSize en la configuración de DataModel .

¿Basura restante después de refactorizar? O tal vez se perdieron los cálculos de la variable DefaultGroupSizeSpace? En cualquier caso, un intento de reemplazar su valor con dataModel.getGroupSize () rompió el diseño, y quizás solo el autor de este código pueda dar una respuesta inequívoca.

Fragmento 8: validación rota, parte 2


private String parseArrayDimensions(String datatype,
                                    List<Integer> arrayDimensions) {
  String dataTypeName = datatype;
  boolean zeroLengthArray = false;
  while (dataTypeName.endsWith("]")) {
    if (zeroLengthArray) {                   // <=
      return null; // only last dimension may be 0
    }
    int rBracketPos = dataTypeName.lastIndexOf(']');
    int lBracketPos = dataTypeName.lastIndexOf('[');
    if (lBracketPos < 0) {
      return null;
    }
    int dimension;
    try {
      dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                          rBracketPos));
      if (dimension < 0) {
        return null; // invalid dimension
      }
    }
    catch (NumberFormatException e) {
      return null;
    }
    dataTypeName = dataTypeName.substring(0, lBracketPos).trim();
    arrayDimensions.add(dimension);
  }
  return dataTypeName;
}

Advertencia de PVS-Studio: V6007 La expresión 'zeroLengthArray' siempre es falsa. PdbDataTypeParser.java:278

Este método analiza la dimensión de una matriz multidimensional y devuelve el texto restante después del análisis o nulo para datos no válidos. Un comentario al lado de una de las comprobaciones de validación indica que solo el último tamaño de lectura puede ser cero. El análisis va de derecha a izquierda, por lo que se entiende que '[0] [1] [2]' es un texto de entrada válido y '[2] [1] [0]' no lo es.

Pero, problema: nadie agregó la verificación de que el siguiente tamaño es cero, y el analizador come datos no válidos sin preguntas innecesarias. Probablemente deberías arreglar el bloque try de la siguiente manera:

try {
  dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                      rBracketPos));
  if (dimension < 0) {
    return null; // invalid dimension
  } else if (dimension == 0) {
    zeroLengthArray = true;
  }
}
catch (NumberFormatException e) {
  return null;
}

Naturalmente, existe la posibilidad de que este criterio de validez en el tiempo resulte innecesario, o el comentario del autor tiene un significado diferente y es necesario verificar la lectura de la primera dimensión. En cualquier caso, la validación de datos es una parte crítica de cualquier aplicación, que debe tomarse con total responsabilidad. Los errores pueden provocar bloqueos bastante inofensivos de la aplicación, así como agujeros de seguridad, fuga de datos, corrupción o pérdida de datos (por ejemplo, si omite la inyección de SQL durante la validación de la consulta).

Un poco sobre el resto de las advertencias


El lector puede notar que se emitieron muchas advertencias, pero se consideraron pocas. El cloc no muy bien ajustado en el proyecto contó aproximadamente 1,25 millones de líneas de código Java (no está vacío y no hay comentarios). El hecho es que casi todas las advertencias son extremadamente similares: aquí se olvidaron de buscar nulos , no eliminaron el código heredado no utilizado allí. Realmente no quiero aburrir al lector enumerando lo mismo, y mencioné parte de tales casos al comienzo del artículo.

Otro ejemplo son las cincuenta advertencias " La función V6009 recibe un argumento extraño" en el contexto del uso incorrecto del método de subcadena(CParserUtils.java:280, ComplexName.java:48 y otros) para obtener el resto de la cadena después de cualquier separador. Los desarrolladores a menudo esperan que este separador esté presente en la cadena y olvidan que de lo contrario indexOf devolverá -1, que es un valor incorrecto para la subcadena . Naturalmente, si los datos se validaron o no se recibieron desde el exterior, la probabilidad de que la aplicación se bloquee se reduce significativamente. Sin embargo, en general, estos son lugares potencialmente peligrosos de los que queremos ayudar a deshacernos.

Conclusión


En general, Ghidra está satisfecho con la calidad del código: no hay pesadillas especiales evidentes. El código está bien formateado y tiene un estilo muy consistente: en la mayoría de los casos, las variables, los métodos y todo lo demás reciben nombres claros, se encuentran comentarios explicativos y hay una gran cantidad de pruebas.

Naturalmente, no hubo problemas, entre los cuales:

  • Código muerto, que, muy probablemente, permaneció después de numerosas refactorizaciones
  • Muchos javadocs están irremediablemente desactualizados y, por ejemplo, indican parámetros inexistentes;
  • No hay posibilidad de desarrollo conveniente cuando se usa IntelliJ IDEA ;
  • Un sistema modular construido alrededor de la reflexión hace que navegar por un proyecto y encontrar dependencias entre componentes sea mucho más difícil.

No descuides las herramientas de desarrollador. El análisis estático, como los cinturones de seguridad, no es una panacea, pero ayudará a prevenir algunos desastres antes del lanzamiento. Y a nadie le gusta usar el software conectado.

Puedes leer sobre otros proyectos probados en nuestro blog . Y también tenemos una licencia de prueba y varias opciones para usar el analizador sin la necesidad de pagarlo.



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Nikita Lazeba. NSA, Ghidra y Unicornios .

All Articles