NSA, Ghidra et licornes

NSA, Ghidra et licornes

Cette fois, les yeux de l'équipe PVS-Studio ont été attirés par Ghidra, un cadre large et diabolique pour la rétro-ingénierie avec lequel vous pouvez analyser divers fichiers binaires et faire toutes sortes de choses effrayantes avec eux. La chose la plus intéressante à ce sujet n'est pas qu'il est libre d'utiliser ou s'étend bien avec les plugins, mais qu'ils l'ont écrit dans la NSA et ont publié la source sur GitHub pour tout le monde. D'une part, la NSA semble disposer de suffisamment de ressources pour maintenir la base de code propre. Et d'autre part, de nouveaux contributeurs qui ne le connaissaient pas très bien auraient pu accidentellement ajouter des bogues non détectés récemment. Par conséquent, armés d'une analyse statique, nous avons décidé de rechercher des faiblesses dans ce projet.

Prélude


Au total, l'analyseur statique PVS-Studio a émis 651 avertissements élevés, 904 moyens et 909 faibles dans la partie Java du projet Ghidra ( version 9.1.2, commit 687ce7f ). Parmi eux, environ la moitié des réponses hautes et moyennes ont été déclenchées par le diagnostic V6022 .Le paramètre n'est pas utilisé à l'intérieur du corps de la méthode ", qui apparaît généralement après refactorisation, lorsqu'un paramètre n'était plus nécessaire ou qu'une fonctionnalité était temporairement désactivée par des commentaires. Un aperçu rapide de ces avertissements (il y en a trop pour les afficher chacun en tant qu'observateur externe ) dans ce projet n'a révélé aucun élément manifestement suspect. Il est probablement autorisé pour ce projet de désactiver temporairement ce diagnostic dans les paramètres de l'analyseur afin de ne pas être distrait par celui-ci. En pratique, vous pouvez souvent voir des fautes de frappe au nom du paramètre setter ou constructeur et, d'une manière générale, Je suis sûr que la plupart des lecteurs rencontrent au moins une fois un schéma désagréable similaire:

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

Plus de la moitié des avertissements faibles ont été émis par le diagnostic " Déréférence potentielle nulle V6008 de" variable "- par exemple, la valeur File.getParentFile () est souvent utilisée sans vérifier la valeur null . Si l'objet fichier sur lequel cette méthode a été appelée a été construit sans chemin absolu, null sera renvoyé et l'absence de vérification risque de supprimer l'application.

Par tradition, nous n'analyserons que les avertissements de niveaux élevés et moyens, car la majeure partie des erreurs réelles y sont contenues. Lorsque vous travaillez avec des rapports d'analyseur, nous recommandons toujours d'analyser les avertissements par ordre décroissant de leur fiabilité.

Ensuite, nous considérons quelques fragments indiqués par l'analyseur qui me paraissent suspects ou intéressants. La base de code du projet s'est avérée être d'une taille impressionnante, et il est presque impossible de trouver de tels endroits manuellement.

Fragment 1: validation interrompue


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;
}

PVS-Studio Warning: V6001 Il existe des sous-expressions identiques 'newDataType.getLength ()' à gauche et à droite de l'opérateur '>'. DataTypeSelectionEditor.java data66

Cette classe fournit un composant graphique pour sélectionner un type de données qui prend en charge la saisie semi -automatique. Le développeur utilisant ce composant peut définir la taille maximale autorisée du type de données sélectionné (via le champ maxSize ) ou la rendre illimitée en définissant une valeur négative. Il a été supposé que lors de la validation des données entrées, le dépassement de la limite lève une exception, qui est alors rattrapée dans la pile d'appels et l'utilisateur reçoit un message.

Il semble que l'auteur du composant ait été distrait au moment de la rédaction de ce test, ou peut-être qu'il ait pensé au sens de la vie, mais au final, la validation n'est tout simplement pas effectuée, car le nombre ne peut jamais être plus grand que lui-même et, en conséquence, nous obtenons d'ignorer cette condition. Cela signifie que ce composant peut fournir des données non valides.

Une autre erreur similaire a été trouvée dans deux autres classes: GuidUtil et 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;
  }
  ...
}

Avertissement PVS-Studio: l' expression V6007 'données [i]! = 0xFFFFFFFFL' est toujours vraie. GuidUtil.java:200

La boucle for de la méthode isOK vérifie que la même valeur n'est pas égale à deux nombres différents en même temps. Si tel est le cas, le GUID est immédiatement reconnu comme valide. Autrement dit, le GUID sera invalide uniquement si le tableau de données est vide, et cela ne se produit jamais, car la valeur de la variable correspondante n'est affectée qu'une seule fois - au début de la méthode parseLine . Corps de la

méthode IsOKdans les deux classes, cela coïncide complètement, ce qui suggère l'idée d'un autre copier-coller de code incorrect. Ce que l'auteur voulait exactement vérifier, je ne suis pas sûr, mais je peux supposer que cette méthode devrait être corrigée comme suit:

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;
}

Fragment 2: masquer les exceptions


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;
  }
}

PVS-Studio Warning: V6006 L'objet a été créé mais il n'est pas utilisé. Le mot clé 'throw' peut être manquant: 'new MemoryAccessException ("Cyclic Access")'. BitMappedSubMemoryBlock.java:99

Les objets d'exception eux-mêmes, comme vous le savez, ne font rien (ou du moins ne devraient rien faire). Presque toujours, leurs nouvelles instances sont jetées par jet , dans de rares cas - transférées quelque part ou placées dans une collection.

La classe contenant cette méthode est un wrapper sur un bloc de mémoire qui permet de lire et d'écrire des données. Ici, en raison du fait que les exceptions ne sont pas levées, la restriction imposée d'accès au bloc de mémoire actuel avec l'indicateur ioPending peut être violéeet, en outre, une exception AddressOverflowException est ignorée . Ainsi, les données peuvent être silencieusement corrompues et au lieu d'indiquer explicitement une erreur à un endroit particulier, le développeur recevra des artefacts étranges qui doivent être analysés par le débogueur.

Il y avait huit de ces exceptions perdues:

  • BitMappedSubMemoryBlock.java: lignes 77, 99, 106, 122
  • ByteMappedSubMemoryBlock.java: lignes 52, 73, 92, 114

Il est caractéristique que dans les mêmes fichiers, il existe des méthodes extrêmement similaires dans lesquelles la projection est présente. Très probablement, une méthode a été écrite à l'origine de la même manière que le fragment ci-dessus, après quoi elle a été copiée plusieurs fois, a en quelque sorte trouvé une erreur et l'a corrigée dans les endroits dont ils pouvaient se souvenir.

Fragment 3: champ de mines


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();
  ...
}

Avertissement PVS-Studio: V6008 Déréférence nulle de 'selectedNode' dans la fonction 'setViewPanel'. OptionsPanel.java:266

L'analyseur a menti un peu - pour le moment, appeler la méthode processSelection ne conduit pas à une exception NullPointerException , car cette méthode n'est appelée que deux fois et avant d'être appelée, selectedNode est explicitement vérifié pour null . Cela ne doit pas être fait, car un autre développeur peut voir que la méthode gère explicitement le cas selectedNode == null et décider qu'il s'agit d'une valeur valide, ce qui entraînera le blocage de l'application. De telles surprises sont particulièrement dangereuses uniquement dans les projets ouverts, car des personnes qui ne connaissent pas complètement la base de code y participent.

En général, je dois dire que toute la méthode processSelection semble plutôt étrange. Il est probable qu'il s'agit d'une erreur de copier-coller, car dans la même méthode, un bloc if avec le même corps est rencontré deux fois de plus, bien qu'avec des conditions différentes. Cependant, à ce stade, le selectedNode ne sera plus null et la chaîne d'appel setViewPanel-setHelpLocation n'entraînera pas d' exception NullPointerException .

Fragment 4: auto-complétion pour le 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);
  }
}

PVS-Studio Warning: V6053 La collection est modifiée pendant que l'itération est en cours. ConcurrentModificationException peut se produire. DWARFExpressionOpCodes.java:205

Dans ce cas, l'analyseur a de nouveau menti un peu - l'exception ne sera pas levée, car la collection UNSUPPORTED_OPCODES est toujours vide et la boucle ne s'exécutera tout simplement pas. De plus, il se trouve que la collection est une multitude, et l'ajout d'un élément déjà présent ne le changera pas. Très probablement, l'auteur a entré pour chaquele nom de la collection par auto-complétion et n'a pas remarqué que le mauvais champ a été proposé. Il est impossible de modifier la collection pendant l'itération, mais dans de bonnes circonstances, comme dans ce cas, l'application peut ne pas tomber. Ici, cette faute de frappe a un effet indirect: la machine qui analyse les fichiers DWARF s'appuie sur cette collection pour arrêter l'analyse lors de la recherche d'opcodes non pris en charge.

À partir de Java 9, il vaut la peine d'utiliser les méthodes d'usine de la bibliothèque standard pour les collections constantes: par exemple, Set.of (T ... elements) est non seulement beaucoup plus pratique, mais rend également immuable la collection créée, ce qui augmente la fiabilité du code.

Fragment 5: il y a tout


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;
}

Avertissements de PVS-Studio:

  • V6007 L' expression 'index> = 0' est toujours vraie. ExternalNamesTableModel.java:105
  • V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

L'auteur y a pensé, et dans la méthode indexOf, au lieu de "index" -1 pour une valeur inconnue retourne 0 - l'index du premier élément de la collection de chemins . Même si la collection est vide. Ou peut-être que la méthode a été générée, mais a oublié de modifier la valeur de retour par défaut. Par conséquent, la méthode setValueAt supprimera toute valeur qui lui sera transmise et affichera l'utilisateur avec une erreur «Le nom existe déjà», même s'il n'y a pas de nom existant.

Soit dit en passant, indexOf n'est utilisé nulle part ailleurs, et sa valeur n'est nécessaire que pour déterminer si l'élément que vous recherchez existe. Peut-être qu'au lieu d'une méthode distincte, écrivez for-each directement dans setValueAt et effectuez returnsur un élément correspondant au lieu de jeux avec des index.

Remarque: je n'ai pas pu reproduire l'erreur alléguée. La méthode setValueAt n'est probablement plus utilisée ou appelée uniquement sous certaines conditions.

Fragment 6: garder le silence


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");
  ...
}

PVS-Studio Warning: V6033 Un élément avec la même clé '@' a déjà été ajouté. FilterOptions.java:45

Ghidra prend en charge le filtrage des données dans divers contextes: par exemple, vous pouvez filtrer la liste des fichiers de projet par nom. De plus, le filtrage par plusieurs mots-clés à la fois est implémenté: «.java, .c» lorsque le mode «OU» est activé, affiche tous les fichiers dont le nom contient «.java» ou «.c». Il est entendu que n'importe quel caractère spécial peut être utilisé comme séparateur de mots (un séparateur spécifique est sélectionné dans les paramètres du filtre), mais en réalité le point d'exclamation n'était pas disponible.

Dans de telles feuilles d'initialisation, il est très facile de les sceller, car elles sont souvent écrites à l'aide de copier-coller, et lorsque vous regardez un tel code, vos yeux se brouillent rapidement. Et si la faute de frappe n'était pas sur deux lignes adjacentes, alors à la main, elle ne sera presque certainement pas vue.

Fragment 7: le reste de la division est toujours 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();
}

Avertissements de PVS-Studio:

  • V6007 L' expression '((i + 1)% defaultGroupSizeSpace) == 0' est toujours vraie. ByteViewerLayoutModel.java:66
  • V6048 Cette expression peut être simplifiée. L'opérande 'defaultGroupSizeSpace' dans l'opération est égal à 1. ByteViewerLayoutModel.java:66

Le visualiseur d'octets hexadécimal prend en charge le choix de la taille des groupes affichés: par exemple, vous pouvez configurer la sortie au format 'ffff ffff' ou 'ff ff ff ff'. La méthode setFactorys est responsable de l'emplacement de ces groupes dans l'interface utilisateur . Malgré le fait que la personnalisation et l'affichage fonctionnent correctement, le cycle de cette méthode semble extrêmement suspect: le reste de la division par un est toujours nul, ce qui signifie que la coordonnée x augmentera à chaque itération. La suspicion ajoute des propriétés et la disponibilité groupSize dans la définition de DataModel .

Reste des ordures après refactorisation? Ou peut-être que les calculs de la variable defaultGroupSizeSpace ont été perdus? Dans tous les cas, une tentative de remplacer sa valeur par dataModel.getGroupSize () a cassé la disposition, et peut-être que seul l'auteur de ce code peut donner une réponse non ambiguë.

Fragment 8: validation brisée, partie 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;
}

Avertissement PVS-Studio: L' expression V6007 'zeroLengthArray' est toujours fausse. PdbDataTypeParser.java:278

Cette méthode analyse la dimension d'un tableau multidimensionnel et renvoie le texte restant après l'analyse ou null pour les données non valides. Un commentaire à côté de l'une des vérifications de validation indique que seule la dernière taille de lecture peut être nulle. L'analyse va de droite à gauche, il est donc entendu que «[0] [1] [2]» est un texte d'entrée valide, et «[2] [1] [0]» ne l'est pas.

Mais, problème: personne n'a ajouté la vérification que la taille suivante est zéro et l'analyseur mange des données invalides sans poser de questions inutiles. Vous devriez probablement corriger le bloc try comme suit:

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;
}

Naturellement, il est possible que ce critère de validité dans le temps se soit avéré inutile, ou que le commentaire de l’auteur ait une signification différente et qu’il soit nécessaire de vérifier la première dimension lue. Dans tous les cas, la validation des données est un élément essentiel de toute application, qui doit être prise avec l'entière responsabilité. Des erreurs peuvent entraîner des plantages de l'application tout à fait inoffensifs, ainsi que des failles de sécurité, des fuites de données, une corruption ou une perte de données (par exemple, si vous ignorez l'injection SQL lors de la validation de la requête).

Un peu sur le reste des avertissements


Le lecteur peut remarquer que de nombreux avertissements ont été émis, mais peu ont été pris en compte. Le cloc pas très bien réglé dans le projet comptait environ 1,25 million de lignes de code Java (pas vides et pas de commentaires). Le fait est que presque tous les avertissements sont extrêmement similaires: ici, ils ont oublié de vérifier la valeur null , ils n'y ont pas supprimé le code hérité inutilisé. Je ne veux pas vraiment ennuyer le lecteur en énumérant la même chose, et j'ai mentionné une partie de ces cas au début de l'article.

Un autre exemple est les cinquante avertissements " La fonction V6009 reçoit un argument impair" dans le contexte d'une utilisation inexacte de la méthode de sous - chaîne(CParserUtils.java:280, ComplexName.java:48 et autres) pour obtenir le reste de la chaîne après tout séparateur. Les développeurs espèrent souvent que ce séparateur sera présent dans la chaîne et oublient que sinon indexOf renverra -1, ce qui est une valeur incorrecte pour la sous-chaîne . Naturellement, si les données ont été validées ou reçues non de l'extérieur, la probabilité de plantage de l'application est considérablement réduite. Cependant, en général, ce sont des endroits potentiellement dangereux dont nous voulons nous débarrasser.

Conclusion


En général, Ghidra était satisfait de la qualité du code - aucun cauchemar spécial n'est évident. Le code est bien formaté et a un style très cohérent: dans la plupart des cas, les variables, les méthodes et tout le reste reçoivent des noms clairs, des commentaires explicatifs sont trouvés et un grand nombre de tests sont présents.

Naturellement, il n'y a eu aucun problème, parmi lesquels:

  • Code mort, qui, très probablement, est resté après de nombreuses modifications;
  • De nombreux javadocs sont désespérément obsolètes et, par exemple, indiquent des paramètres inexistants;
  • Il n'y a aucune possibilité de développement pratique lors de l'utilisation d'IntelliJ IDEA ;
  • Un système modulaire construit autour de la réflexion rend la navigation dans un projet et la recherche de dépendances entre les composants beaucoup plus difficiles.

Veuillez ne pas négliger les outils de développement. L'analyse statique, comme les ceintures de sécurité, n'est pas une panacée, mais elle aidera à prévenir certaines catastrophes avant la libération. Et personne n'aime utiliser le logiciel connecté.

Vous pouvez lire sur d'autres projets éprouvés dans notre blog . Et nous avons également une licence d'essai et diverses options pour utiliser l'analyseur sans avoir à payer pour cela.



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Nikita Lazeba. NSA, Ghidra et Unicorns .

All Articles