NSA, Ghidra e Unicórnios

NSA, Ghidra e Unicórnios

Desta vez, a equipe do PVS-Studio foi atraída por Ghidra, uma estrutura grande e maligna para engenharia reversa com a qual você pode analisar vários arquivos binários e fazer todo tipo de coisa assustadora com eles. A coisa mais interessante sobre isso não é que ele é gratuito para uso ou se estende bem com plug-ins, mas que foi escrito na NSA e publicado no GitHub para todos. Por um lado, a NSA parece ter recursos suficientes para manter a base de código limpa. Por outro lado, novos colaboradores que não estavam muito familiarizados com ele poderiam ter acidentalmente adicionado erros não detectados recentemente. Portanto, armados com uma análise estática, decidimos procurar pontos fracos neste projeto.

Prelúdio


No total, o analisador estático PVS-Studio emitiu 651 avisos altos, 904 médios e 909 baixos na parte Java do projeto Ghidra ( release 9.1.2, commit 687ce7f ). Entre eles, cerca de metade das respostas altas e médias foram desencadeadas pelo diagnóstico V6022 .O parâmetro não é usado dentro do corpo do método ", que geralmente aparece após a refatoração, quando algum parâmetro não era mais necessário ou alguma funcionalidade foi temporariamente desabilitada pelos comentários. Uma rápida olhada nesses avisos (há muitos deles para exibir cada um como um observador externo ) neste projeto não revelou nada obviamente suspeito. Provavelmente, é permitido que esse projeto desative temporariamente esse diagnóstico nas configurações do analisador para não ser distraído por ele. Na prática, é possível ver erros de digitação no nome do parâmetro setter ou construtor e, de um modo geral, não deve Tenho certeza de que a maioria dos leitores, pelo menos uma vez, encontra um padrão desagradável semelhante:

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

Mais da metade dos avisos baixos foram feitos pelo diagnóstico " Dereferência nula potencial do V6008 de 'variável'" - por exemplo, o valor File.getParentFile () geralmente é usado sem a verificação de nulo . Se o objeto de arquivo no qual esse método foi chamado foi construído sem um caminho absoluto, nulo será retornado e a ausência de verificação poderá interromper o aplicativo.

Por tradição, analisaremos apenas avisos de níveis alto e médio, uma vez que a maior parte dos erros reais está contida neles. Ao trabalhar com relatórios do analisador, sempre recomendamos a análise de avisos em ordem decrescente de confiabilidade.

A seguir, consideramos alguns fragmentos indicados pelo analisador que pareciam suspeitos ou interessantes para mim. A base de código do projeto acabou sendo de tamanho impressionante, e é quase impossível encontrar esses locais manualmente.

Fragmento 1: validação interrompida


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 Existem sub-expressões idênticas 'newDataType.getLength ()' à esquerda e à direita do operador '>'. DataTypeSelectionEditor.java data66

Esta classe fornece um componente gráfico para selecionar um tipo de dados que suporta o preenchimento automático. O desenvolvedor que usa esse componente pode definir o tamanho máximo permitido do tipo de dados selecionado (por meio do campo maxSize ) ou torná-lo ilimitado, definindo um valor negativo. Supunha-se que, ao validar os dados inseridos, exceder o limite gera uma exceção, que é capturada pela pilha de chamadas e o usuário recebe uma mensagem.

Parece que o autor do componente estava distraído no momento em que escrevia esse teste, ou talvez ele pensasse no significado da vida, mas no final, a validação simplesmente não é realizada, pois o número nunca pode ser maior que ele e, consequentemente, ignoramos essa condição. Isso significa que este componente pode fornecer dados inválidos.

Outro erro semelhante foi encontrado em mais duas classes: GuidUtil e 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;
  }
  ...
}

Aviso do PVS-Studio: A expressão V6007 'data [i]! = 0xFFFFFFFFL' é sempre verdadeira. GuidUtil.java:200

O loop for do método isOK verifica se o mesmo valor não é igual a dois números diferentes ao mesmo tempo. Nesse caso, o GUID é imediatamente reconhecido como válido. Ou seja, o GUID será inválido apenas se a matriz de dados estiver vazia, e isso nunca acontece, pois o valor da variável correspondente é atribuído apenas uma vez - no início do método parseLine . Corpo do

método IsOKem ambas as classes coincide completamente, o que sugere a idéia de outra cópia e colagem de código incorreto. O que exatamente o autor queria verificar, não tenho certeza, mas posso assumir que esse método deve ser corrigido da seguinte maneira:

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 exceções


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 O objeto foi criado, mas não está sendo usado. A palavra-chave 'throw' pode estar ausente: 'new MemoryAccessException ("Cyclic Access")'. BitMappedSubMemoryBlock.java:99

Os próprios objetos de exceção, como você sabe, não fazem nada (ou pelo menos não devem fazer nada). Quase sempre, as suas novas instâncias são jogados através de lance , em alguns casos raros - transferido em algum lugar ou talvez colocado em uma coleção.

A classe que contém esse método é um invólucro sobre um bloco de memória que permite ler e gravar dados. Aqui, devido ao fato de que não são lançadas exceções, a restrição imposta de acesso ao bloco de memória atual com o sinalizador ioPending pode ser violadae, além disso, um AddressOverflowException é ignorado . Assim, os dados podem ser corrompidos silenciosamente e, em vez de indicar explicitamente um erro em um local específico, o desenvolvedor receberá artefatos estranhos que precisam ser analisados ​​pelo depurador.

Havia oito dessas exceções perdidas:

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

É característico que nos mesmos arquivos existam métodos extremamente semelhantes nos quais o throw está presente. Provavelmente, um método foi originalmente escrito de forma semelhante ao fragmento acima, após o qual foi copiado várias vezes, de alguma forma encontrou um erro e o corrigiu nos locais em que eles conseguiam se lembrar.

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

Aviso do PVS-Studio: V6008 Desreferência nula de 'selectedNode' na função 'setViewPanel'. OptionsPanel.java:266

O analisador mentiu um pouco - no momento, chamar o método processSelection não leva a uma NullPointerException , já que esse método é chamado apenas duas vezes e, antes de chamá-lo, selectedNode é verificado explicitamente como nulo . Isso não deve ser feito, pois outro desenvolvedor pode ver que o método lida explicitamente com o caso selectedNode == null e decidir que esse é um valor válido, o que resultará na falha do aplicativo. Tais surpresas são especialmente perigosas apenas em projetos abertos, já que pessoas que não conhecem a base de código participam completamente delas.

Em geral, devo dizer que todo o método processSelection parece bastante estranho. É provável que este seja um erro de copiar e colar, pois no mesmo método um bloco if com o mesmo corpo é encontrado duas vezes mais, embora com condições diferentes. No entanto, nesse momento, o selectedNode não será mais nulo e a cadeia de chamadas setViewPanel-setHelpLocation não resultará em uma NullPointerException .

Fragmento 4: auto-completar para o 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 A coleção é modificada enquanto a iteração está em andamento. ConcurrentModificationException pode ocorrer. DWARFExpressionOpCodes.java:205

Nesse caso, o analisador mentiu novamente um pouco - a exceção não será lançada, pois a coleção UNSUPPORTED_OPCODES está sempre vazia e o loop simplesmente não é executado. Além disso, aconteceu que a coleção é uma multidão e a adição de um elemento já presente não a alterará. Muito provavelmente, o autor digitou para cadao nome da coleção por meio do preenchimento automático e não percebeu que o campo errado foi proposto. A modificação da coleção durante a iteração é impossível, mas em boas circunstâncias, como neste caso, o aplicativo pode não cair. Aqui, esse erro de digitação tem um efeito indireto: a máquina que analisa arquivos DWARF depende dessa coleção para interromper a análise ao encontrar códigos de operação não suportados.

A partir do Java 9, vale a pena usar os métodos de fábrica da biblioteca padrão para coleções constantes: por exemplo, Set.of (elementos T ...) não é apenas muito mais conveniente, mas também torna imutável a coleção criada imediatamente, o que aumenta a confiabilidade do código.

Fragmento 5: há tudo


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

Avisos do PVS-Studio:

  • A expressão V6007 'index> = 0' é sempre verdadeira. ExternalNamesTableModel.java:105
  • V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

O autor pensou sobre isso e, no método indexOf, em vez de "index" -1 para um valor desconhecido, retorna 0 - o índice do primeiro elemento da coleção de caminhos . Mesmo se a coleção estiver vazia. Ou talvez o método tenha sido gerado, mas esqueceu de alterar o valor de retorno padrão. Como resultado, o método setValueAt descartará qualquer valor passado para ele e exibirá o usuário com um erro "O nome já existe", mesmo se não houver um nome existente.

A propósito, indexOf não é usado em nenhum outro lugar, e seu valor é necessário apenas para determinar se o elemento que você está procurando existe. Talvez, em vez de um método separado, escreva para cada um diretamente em setValueAt e faça retornoem um item correspondente em vez de jogos com índices.

Nota: Não foi possível reproduzir o alegado erro. O método setValueAt provavelmente não é mais usado ou chamado apenas sob determinadas condições.

Fragmento 6: mantenha o silêncio


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 Um item com a mesma chave '@' já foi adicionado. FilterOptions.java:45 O

Ghidra suporta a filtragem de dados em vários contextos: por exemplo, você pode filtrar a lista de arquivos do projeto por nome. Além disso, a filtragem por várias palavras-chave ao mesmo tempo é implementada: '.java, .c' quando o modo 'OR' está ativado, exibe todos os arquivos cujo nome contém '.java' ou '.c'. Entende-se que qualquer caractere especial pode ser usado como um separador de palavras (um separador específico é selecionado nas configurações de filtro), mas, na realidade, o ponto de exclamação não estava disponível.

Nessas folhas de inicialização, é muito fácil selar, pois elas geralmente são escritas usando copiar e colar e, quando você olha para esse código, seus olhos ficam borrados rapidamente. E se o erro de digitação não estava em duas linhas adjacentes, então à mão quase certamente ninguém verá.

Fragmento 7: o restante da divisão é sempre 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();
}

Avisos do PVS-Studio:

  • A expressão V6007 '((i + 1)% defaultGroupSizeSpace) == 0' sempre é verdadeira. ByteViewerLayoutModel.java:66
  • V6048 Esta expressão pode ser simplificada. O operando 'defaultGroupSizeSpace' na operação é igual a 1. ByteViewerLayoutModel.java:66

O visualizador de byte hexadecimal suporta a escolha do tamanho dos grupos exibidos: por exemplo, você pode configurar a saída no formato 'ffff ffff' ou 'ff ff ff ff'. O método setFactorys é responsável pela localização desses grupos na interface do usuário . Apesar de a personalização e a exibição funcionarem corretamente, o ciclo neste método parece extremamente suspeito: o restante da divisão por um é sempre zero, o que significa que a coordenada x aumentará a cada iteração. A suspeita adiciona propriedades e disponibilidade groupSize na configuração do DataModel .

Lixo restante após a refatoração? Ou talvez os cálculos da variável defaultGroupSizeSpace tenham sido perdidos? De qualquer forma, uma tentativa de substituir seu valor por dataModel.getGroupSize () interrompeu o layout, e talvez apenas o autor desse código possa dar uma resposta inequívoca.

Fragmento 8: validação interrompida, 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;
}

Aviso do PVS-Studio: A expressão V6007 'zeroLengthArray' é sempre falsa. PdbDataTypeParser.java:278

Esse método analisa a dimensão de uma matriz multidimensional e retorna o texto restante após a análise ou nulo para dados inválidos. Um comentário ao lado de uma das verificações de validação afirma que apenas o último tamanho de leitura pode ser zero. A análise vai da direita para a esquerda, portanto, entende-se que '[0] [1] [2]' é um texto de entrada válido e '[2] [1] [0]' não.

Mas, problema: ninguém adicionou a verificação de que o próximo tamanho é zero e o analisador consome dados inválidos sem perguntas desnecessárias. Você provavelmente deve corrigir o bloco try da seguinte maneira:

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 a possibilidade de que esse critério de validade ao longo do tempo seja desnecessário, ou o comentário do autor tenha um significado diferente e é necessário verificar a leitura da primeira dimensão. De qualquer forma, a validação de dados é uma parte crítica de qualquer aplicativo, que deve ser assumida com total responsabilidade. Os erros podem levar a falhas inofensivas do aplicativo, além de falhas de segurança, vazamento de dados, corrupção ou perda de dados (por exemplo, se você pular a injeção de SQL durante a validação de consulta).

Um pouco sobre o resto dos avisos


O leitor pode perceber que muitos avisos foram emitidos, mas poucos foram considerados. O cloc não muito bem ajustado no projeto contou cerca de 1,25 milhão de linhas de código Java (não vazias e sem comentários). O fato é que quase todos os avisos são extremamente semelhantes: aqui eles se esqueceram de verificar se há nulo , não excluíram o código legado não utilizado lá. Eu realmente não quero aborrecer o leitor listando a mesma coisa, e mencionei parte desses casos no início do artigo.

Outro exemplo são os cinquenta avisos " A função V6009 recebe um argumento estranho" no contexto do uso impreciso do método de substring(CParserUtils.java:280, ComplexName.java:48 e outros) para obter o restante da sequência após qualquer separador. Os desenvolvedores geralmente esperam que esse separador esteja presente na string e esquecem que, caso contrário, o indexOf retornará -1, que é um valor incorreto para a substring . Naturalmente, se os dados foram validados ou recebidos não de fora, a probabilidade de falha do aplicativo é significativamente reduzida. No entanto, em geral, esses são lugares potencialmente perigosos dos quais queremos ajudar a nos livrar.

Conclusão


Em geral, Ghidra está satisfeito com a qualidade do código - nenhum pesadelo especial é evidente. O código é bem formatado e tem um estilo muito consistente: na maioria dos casos, variáveis, métodos e tudo o mais recebem nomes claros, comentários explicativos são encontrados, um grande número de testes está presente.

Naturalmente, não houve problemas, entre os quais:

  • Código morto, que, provavelmente, permaneceu após inúmeras refatorações;
  • Muitos javadocs estão irremediavelmente desatualizados e, por exemplo, indicam parâmetros inexistentes;
  • Não há possibilidade de desenvolvimento conveniente ao usar o IntelliJ IDEA ;
  • Um sistema modular construído em torno da reflexão dificulta a navegação em um projeto e a localização de dependências entre os componentes.

Por favor, não negligencie as ferramentas do desenvolvedor. A análise estática, como os cintos de segurança, não é uma panacéia, mas ajudará a evitar alguns desastres antes do lançamento. E ninguém gosta de usar o software conectado.

Você pode ler sobre outros projetos comprovados em nosso blog . E também temos uma licença de teste e várias opções para usar o analisador sem a necessidade de pagar por isso.



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Nikita Lazeba. NSA, Ghidra e Unicórnios .

All Articles