🌥️ 🚾 👩🏾‍⚖️ NSA、ギドラ、ユニコーン 🌺 💼 #⃣

今回、PVS-Studioチームは、さまざまなバイナリファイルを分析してあらゆる種類の恐ろしいことを行うことができるリバースエンジニアリングのための大きくて邪悪なフレームワークであるGhidraに魅了されました。それについて最も興味深いことは、それがプラグインで自由に使用または拡張できるということではなく、NSAで作成され、誰もがGitHubに投稿したことです。一方で、NSAにはコードベースをクリーンに保つのに十分なリソースがあるようです。そして、その一方で、あまり知られていない新しい寄稿者が最近、検出されていないバグを誤って追加した可能性があります。したがって、静的分析を武器に、このプロジェクトの弱点を探すことにしました。

プレリュード

合計すると、PVS-Studio静的アナライザーは、ギドラプロジェクトのJava部分で、651高、904中、および909低の警告を発行しました（リリース9.1.2、コミット687ce7f）。その中で、高および中程度の応答の約半分はV6022診断によって引き起こされました。パラメータはメソッドの本体内では使用されません。これは通常、一部のパラメータが不要になったか、一部の機能がコメントによって一時的に無効にされたときに、リファクタリング後に表示されます。これらの警告を簡単に確認してください（警告が多すぎて、それぞれを外部オブザーバとして表示することができません）このプロジェクトでは、明らかに疑わしいものは何も明らかになりませんでした。おそらく、このプロジェクトがアナライザー設定でこの診断を一時的に無効にして、気が散らないようにすることは許容されます。実際には、セッターまたはコンストラクターパラメーターの名前にタイプミスが見られることがよくありますほとんどの読者は、少なくとも一度は同様の不愉快なパターンに出くわします。

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

低警告の半分以上が「V6008潜在的な「変数」のnull逆参照」診断によって生成されました。たとえば、値File.getParentFile（）はnullをチェックせずに使用されることがよくあります。このメソッドが呼び出されたファイルオブジェクトが絶対パスなしで作成された場合、nullが返され、検証が行われないとアプリケーションが削除される可能性があります。

伝統的に、高レベルと中レベルの警告のみを分析します。これは、実際のエラーの大部分が含まれているためです。アナライザーレポートを使用する場合は、信頼性の高い順に警告を分析することをお勧めします。

次に、アナライザーによって示された、疑わしいまたは興味深いと思われるいくつかのフラグメントを検討します。プロジェクトのコードベースは印象的なサイズであることが判明し、そのような場所を手動で見つけることはほとんど不可能です。

フラグメント1：無効な検証

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警告：V6001「>」演算子の左側と右側に同じサブ式「newDataType.getLength（）」があります。 DataTypeSelectionEditor.java data66

このクラスは、オートコンプリートをサポートするデータ型を選択するためのグラフィカルコンポーネントを提供します。このコンポーネントを使用する開発者は、選択されたデータ型の最大許容サイズを（maxSizeフィールドを介して）設定するか、負の値を設定することで無制限にすることができます。入力されたデータを検証するとき、制限を超えると例外がスローされ、それがコールスタックに追いつき、ユーザーにメッセージが表示されると想定されていました。

コンポーネントの作成者は、このテストを書いている時点で気が散っていたようです、あるいはおそらく彼は人生の意味について考えていましたが、最終的には検証が実行されません。数がそれ自体より大きくなることはないため、この条件は無視されます。つまり、このコンポーネントは無効なデータを提供する可能性があります。

別の同様のエラーがさらに2つのクラスで見つかりました：GuidUtilと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;
  }
  ...
}

PVS-Studio 警告：V6007式 'data [i]！= 0xFFFFFFFFL'は常にtrueです。 GuidUtil.java:200

ザ用のループISOKの方法の同じ値が同時に2つの異なる番号に等しくないことをチェックします。その場合、GUIDはすぐに有効であると認識されます。つまり、GUIDは、データ配列が空の場合にのみ無効になります。これは、対応する変数の値がparseLineメソッドの最初に1回だけ割り当てられるため、発生しません。IsOK

メソッド本体両方のクラスで完全に一致しています。これは、間違ったコードの別のコピーと貼り付けのアイデアを示唆しています。著者が正確に確認したかったことはわかりませんが、このメソッドは次のように修正する必要があると考えられます。

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

フラグメント2：例外を隠す

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警告：V6006オブジェクトは作成されましたが、使用されていません。「throw」キーワードが欠落している可能性があります：「new MemoryAccessException（ "Cyclic Access"）」。 BitMappedSubMemoryBlock.java:99

例外オブジェクト自体は、ご存じのとおり、何もしません（または、少なくとも何もすべきではありません）。ほとんどの場合、それらの新しいインスタンスはスローを通じてスローされますが、まれに、どこかに転送されるか、コレクションに配置される場合があります。

このメソッドを含むクラスは、データの読み取りと書き込みを可能にするメモリブロックのラッパーです。ここでは、例外がスローされないため、ioPendingフラグを使用して現在のメモリブロックへのアクセスに課せられた制限に違反する可能性がありますさらに、AddressOverflowExceptionは無視されます。したがって、データは静かに破損する可能性があり、特定の場所でエラーを明示的に示す代わりに、開発者はデバッガーで分析する必要のある奇妙なアーティファクトを受け取ります。

これらの失われた例外は8つありました。

BitMappedSubMemoryBlock.java：77、99、106、122行目
ByteMappedSubMemoryBlock.java：52、73、92、114行目

同じファイル内に、throwが存在する非常に類似したメソッドがあるのが特徴です。ほとんどの場合、1つのメソッドは上記のフラグメントと同様に最初に記述された後、数回コピーされ、何らかの理由でエラーが見つかり、覚えている場所で修正されました。

フラグメント3：地雷原

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

PVS-Studioの警告：V6008関数「setViewPanel」の「selectedNode」のnull逆参照。 OptionsPanel.java:266

アナライザーは少し嘘をつきました-現時点では、processSelectionメソッドを呼び出してもNullPointerExceptionは発生しません。このメソッドが呼び出されるのは2回だけであり、それを呼び出す前に、selectedNodeが明示的にnullについてチェックされているためです。別の開発者はメソッドがselectedNode == nullのケースを明示的に処理し、これが有効な値であると判断してアプリケーションがクラッシュする可能性があるため、これを行うべきではありません。コードベースを知らない人が徹底的に参加しているため、特に危険なのはオープンプロジェクトだけのような驚きです。

一般的に、processSelectionメソッド全体はかなり奇妙に見えます。同じメソッドで、条件は異なりますが同じボディのifブロックが2回以上検出されるため、これはコピーと貼り付けのエラーである可能性があります。ただし、この時点では、selectedNodeはnullでなくなり、setViewPanel-setHelpLocation呼び出しチェーンはNullPointerExceptionを引き起こしません。

フラグメント4：悪の自動補完

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警告：V6053反復の進行中にコレクションが変更されました。 ConcurrentModificationExceptionが発生する場合があります。 DWARFExpressionOpCodes.java:205

この場合、アナライザは再び少し嘘をつきました。UNSUPPORTED_OPCODES コレクションは常に空であり、ループは実行されないため、例外はスローされません。さらに、たまたまコレクションが多数であり、すでに存在する要素を追加しても変更されません。おそらく、著者はfor-eachオートコンプリートによるコレクションの名前。間違ったフィールドが提案されたことに気づかなかった。反復中にコレクションを変更することは不可能ですが、この場合のように、アプリケーションが適切に動作しない場合があります。ここで、このタイプミスには間接的な影響があります。DWARFファイルを解析するマシンは、このコレクションに依存して、サポートされていないオペコードを見つけると分析を停止します。

Java 9以降、定数コレクションの標準ライブラリのファクトリメソッドを使用する価値があります。たとえば、Set.of（T ...要素）ははるかに便利であるだけでなく、作成されたコレクションをすぐに不変にして、コードの信頼性を高めます。

フラグメント5：すべてがあります

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

PVS-Studioの警告：

V6007式 'index> = 0'は常にtrueです。ExternalNamesTableModel.java:105
V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

著者はそれについて考え、indexOfメソッドでは、 "index"の代わりに、不明な値に対して-1は0- パスコレクションの最初の要素のインデックスを返します。コレクションが空であっても。または、メソッドが生成されたものの、デフォルトの戻り値を変更するのを忘れていました。その結果、既存の名前がない場合でも、setValueAtメソッドは渡された値を破棄し、「名前は既に存在します」エラーでユーザーを表示します。

ちなみに、indexOfは他では使用されておらず、その値は、探している要素が存在するかどうかを判断するためにのみ必要です。おそらく、個別のメソッドの代わりに、set - ValueAtにfor-eachを直接記述して、returnします。インデックス付きのゲームではなく、一致するアイテムで。

注：申し立てられたエラーを再現できませんでした。setValueAtメソッドはおそらく使用されないか、特定の条件下でのみ呼び出されます。

フラグメント6：沈黙を守る

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警告：V6033同じキー「@」のアイテムはすでに追加されています。 FilterOptions.java:45 Ghidra

は、さまざまなコンテキストでのデータのフィルタリングをサポートしています。たとえば、プロジェクトファイルのリストを名前でフィルタリングできます。さらに、一度に複数のキーワードによるフィルタリングが実装されています。「OR」モードがオンの場合、「。java、.c」は、名前に「.java」または「.c」のいずれかを含むすべてのファイルを表示します。単語の区切り文字として特殊文字を使用できることは理解されていますが（特定の区切り文字はフィルター設定で選択されています）、実際には感嘆符は使用できませんでした。

このような初期化シートでは、コピーと貼り付けを使用して記述されることが多く、そのようなコードを見ると目がすぐにぼやけます。そして、タイプミスが2つの隣接する行になかった場合は、手作業ではほとんど間違いなく誰も見なくなります。

フラグメント7：除算の残りは常に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();
}

PVS-Studioの警告：

V6007式 '（（i + 1）％defaultGroupSizeSpace）== 0'は常にtrueです。ByteViewerLayoutModel.java:66
V6048この式は簡略化できます。操作のオペランド「defaultGroupSizeSpace」は1です。ByteViewerLayoutModel.java:66

16進バイトビューアは、表示されるグループのサイズの選択をサポートします。たとえば、出力を「ffff ffff」または「ff ff ff ff」の形式で構成できます。setFactorysメソッドは、ユーザーインターフェイスでのこれらのグループの場所を管理します。カスタマイズと表示が正しく機能するという事実にもかかわらず、この方法のサイクルは非常に疑わしく見えます。1による除算の残りは常に0であり、これは各反復でx座標が増加することを意味します。 Suspicionは、DataModelの設定にプロパティと可用性groupSizeを追加します。リファクタリング後に残ったゴミ？または、defaultGroupSizeSpace変数の計算が失われた可能性があります

？いずれの場合でも、その値をdataModel.getGroupSize（）で置き換えようとするとレイアウトが壊れ、おそらくこのコードの作成者だけが明確な答えを出すことができます。

フラグメント8：検証の失敗、パート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;
}

PVS-Studio警告：V6007式「zeroLengthArray」は常にfalseです。 PdbDataTypeParser.java:278

このメソッドは、多次元配列の次元を解析し、解析後に残っているテキスト、または無効なデータの場合はnullを返します。検証チェックのいずれかの隣のコメントは、最後の読み取りサイズのみがゼロになることができることを示しています。分析は右から左に進むため、「[0] [1] [2]」は有効な入力テキストであり、「[2] [1] [0]」は有効ではないことがわかります。

しかし、問題は次のサイズがゼロであるというチェックを誰も追加しなかったため、パーサーは不要な質問なしに無効なデータを食べてしまいました。おそらく次のようにtryブロックを修正する必要があります。

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

当然のことながら、この有効性の基準は時間の経過に伴って不要になる可能性があります。または、著者のコメントの意味が異なり、最初に読んだ次元を確認する必要があります。いずれの場合でも、データの検証はすべてのアプリケーションの重要な部分であり、すべての責任を負う必要があります。エラーが発生すると、アプリケーションの非常に無害なクラッシュのほか、セキュリティホール、データの漏洩、データの破損または損失につながる可能性があります（たとえば、クエリ検証中にSQLインジェクションをスキップした場合）。

残りの警告について少し

読者は多くの警告が出されたことに気づくかもしれませんが、いくつかは考慮されませんでした。プロジェクトであまりきれいに調整されていないclocは、約125万行のJavaコードをカウントしました（空でもコメントでもない）。実際、ほとんどすべての警告は非常によく似ています。ここでは、nullを確認するのを忘れており、未使用のレガシーコードを削除していません。同じことを挙げて読者を飽きさせたくないので、記事の冒頭でそのようなケースの一部について述べました。

別の例は、サブストリングメソッドの不正確な使用のコンテキストでの50の警告「V6009関数が奇数の引数を受け取ります」です。（CParserUtils.java:280、ComplexName.java:48など）を使用して、区切り文字の後の残りの文字列を取得します。多くの場合、開発者はこのセパレーターが文字列に存在することを望み、そうでない場合はindexOfが-1を返すことを忘れます。これはsubstringの不正な値です。当然、データが外部からではなく検証または受信された場合、アプリケーションがクラッシュする可能性は大幅に減少します。ただし、一般的に、これらは私たちが取り除くのを手助けしたい潜在的に危険な場所です。

結論

一般的に、ギドラはコードの品質に満足しています-特別な悪夢は明白ではありません。コードは適切にフォーマットされており、非常に一貫したスタイルを持っています。ほとんどの場合、変数、メソッド、その他すべてに明確な名前が付けられ、説明コメントが見つかり、多数のテストが存在します。

当然、問題はありませんでした。

ほとんどの場合、多数のリファクタリング後に残ったデッドコード。
多くのjavadocは絶望的に古くなっており、たとえば、存在しないパラメータを示しています。
IntelliJ IDEAを使用する場合、便利な開発の可能性はありません。
リフレクションを中心に構築されたモジュラーシステムは、プロジェクトのナビゲートとコンポーネント間の依存関係の検出をはるかに困難にします。

開発者ツールをおろそかにしないでください。シートベルトのような静的分析は万能薬ではありませんが、リリース前にいくつかの災害を防ぐのに役立ちます。また、ログインしたソフトウェアを使いたくない人もいます。

あなたは私たちのブログで他の証明されたプロジェクトについて読むことができます。また、試用版ライセンスと、アナライザーを使用するために支払う必要のないさまざまなオプションもあります。

この記事を英語を話す視聴者と共有したい場合は、翻訳へのリンクを使用してください：Nikita Lazeba。NSA、ギドラ、ユニコーン。

NSA、ギドラ、ユニコーン