¿Desea ver un nuevo lote de errores encontrados por el analizador estático PVS-Studio para Java? ¡Entonces únete a la lectura del artículo! Esta vez, el proyecto del Castillo Hinchable se convirtió en objeto de verificación. Los fragmentos de código más interesantes, como de costumbre, te esperan a continuación.
Un poco sobre PVS-Studio
PVS-Studio es una herramienta para identificar errores y posibles vulnerabilidades en el código fuente de los programas. En el momento de escribir este artículo, el análisis estático se implementa para programas escritos en los lenguajes de programación C, C ++, C # y Java.
El analizador para Java es la línea más joven de PVS-Studio. A pesar de esto, no es inferior a sus hermanos mayores en encontrar defectos en el código. Esto se debe al hecho de que el analizador de Java utiliza toda la potencia de los mecanismos del analizador de C ++. Puede leer sobre esta unión única de Java y C ++ aquí .
Por el momento, existen complementos para Gradle, Maven e IntelliJ IDEA para un uso más conveniente . Si está familiarizado con la plataforma de control de calidad continuo SonarQube, es posible que le interese la idea de jugar conintegración del resultado del análisis.
Un poco sobre el castillo hinchable
Bouncy Castle es un paquete con la implementación de algoritmos criptográficos escritos en el lenguaje de programación Java (también hay una implementación en C #, pero este artículo no trata sobre eso). Esta biblioteca complementa la extensión criptográfica estándar (JCE) y contiene una API adecuada para su uso en cualquier entorno (incluido J2ME).
Los programadores son libres de utilizar todas las funciones de esta biblioteca. La implementación de una gran cantidad de algoritmos y protocolos hacen que este proyecto sea muy interesante para los desarrolladores de software que utilizan criptografía.
Empecemos a comprobar
Bouncy Castle es un proyecto bastante serio, porque cualquier error en dicha biblioteca puede reducir la confiabilidad del sistema de cifrado. Por lo tanto, al principio incluso dudamos si podríamos encontrar al menos algo interesante en esta biblioteca, o si todos los errores ya se habían encontrado y arreglado antes que nosotros. Digamos de inmediato que nuestro analizador de Java no nos decepcionó :)
Naturalmente, no podemos describir todas las advertencias del analizador en un artículo, pero tenemos una licencia gratuita para desarrolladores que desarrollan proyectos de código abierto. Si lo desea, puede solicitarnos esta licencia y analizar de forma independiente el proyecto utilizando PVS-Studio.
Y comenzaremos a ver los fragmentos de código más interesantes que se han descubierto.
Código inalcanzable
V6019 Se detectó un código inalcanzable. Es posible que exista un error. XMSSTest.java (170)
public void testSignSHA256CompleteEvenHeight2() {
....
int height = 10;
....
for (int i = 0; i < (1 << height); i++) {
byte[] signature = xmss.sign(new byte[1024]);
switch (i) {
case 0x005b:
assertEquals(signatures[0], Hex.toHexString(signature));
break;
case 0x0822:
assertEquals(signatures[1], Hex.toHexString(signature));
break;
....
}
}
}El valor de la variable de altura en el método no cambia, por lo que el contador i en el ciclo for no puede ser mayor que 1024 (1 << 10). Sin embargo, en la declaración del interruptor, las segundas de casos controles i contra el valor 0x0822 (2082). Naturalmente, el byte de firmas [1] nunca será verificado .
Dado que este es un código de método de prueba, no hay nada de qué preocuparse. Es solo que los desarrolladores deben prestar atención al hecho de que nunca se realiza la prueba de uno de los bytes.
Subexpresiones idénticas
V6001 Hay sub-expresiones idénticas 'etiqueta == PacketTags.SECRET_KEY' a la izquierda y a la derecha de '||' operador. PGPUtil.java (212), PGPUtil.java (212)
public static boolean isKeyRing(byte[] blob) throws IOException {
BCPGInputStream bIn = new BCPGInputStream(new ByteArrayInputStream(blob));
int tag = bIn.nextPacketTag();
return tag == PacketTags.PUBLIC_KEY || tag == PacketTags.PUBLIC_SUBKEY
|| tag == PacketTags.SECRET_KEY || tag == PacketTags.SECRET_KEY;
}En este fragmento de código, la declaración a cambio , etiqueta de doble verificación == PacketTags.SECRET_KEY . De manera similar a verificar la clave pública, la última verificación debe ser la igualdad entre la etiqueta y PacketTags.SECRET_SUBKEY .
Código idéntico en if / else
V6004 La instrucción 'then' es equivalente a la instrucción 'else'. BcAsymmetricKeyUnwrapper.java (36), BcAsymmetricKeyUnwrapper.java (40)
public GenericKey generateUnwrappedKey(....) throws OperatorException {
....
byte[] key = keyCipher.processBlock(....);
if (encryptedKeyAlgorithm.getAlgorithm().equals(....)) {
return new GenericKey(encryptedKeyAlgorithm, key);
} else {
return new GenericKey(encryptedKeyAlgorithm, key);
}
}En este ejemplo, el método devuelve las mismas instancias de la clase GenericKey , independientemente de si se cumple o no la condición en if . Está claro que el código en las ramas if / else debe ser diferente; de lo contrario, la verificación en if no tiene ningún sentido. Aquí el programador fue claramente decepcionado por copiar y pegar.
La expresión siempre es falsa
La expresión V6007 '! (NGroups <8)' es siempre falsa. CBZip2OutputStream.java (753)
private void sendMTFValues() throws IOException {
....
int nGroups;
....
if (nMTF < 200) {
nGroups = 2;
} else if (nMTF < 600) {
nGroups = 3;
} else if (nMTF < 1200) {
nGroups = 4;
} else if (nMTF < 2400) {
nGroups = 5;
} else {
nGroups = 6;
}
....
if (!(nGroups < 8)) {
panic();
}
}Aquí, a la variable nGroups en los bloques de código if / else se le asigna un valor que se usa pero que no cambia en ninguna parte. La expresión en la declaración if siempre será falsa, porque todos los valores posibles para nGroups : 2, 3, 4, 5 y 6 son menores que 8.
El analizador entiende que el método panic () nunca se ejecutará, y por lo tanto da la alarma. Pero aquí, muy probablemente, se utilizó "programación defensiva" y no hay nada de qué preocuparse.
Añadiendo los mismos elementos
V6033 Ya se ha agregado un elemento con la misma clave 'PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC'. PKCS12PBEUtils.java (50), PKCS12PBEUtils.java (49)
class PKCS12PBEUtils {
static {
....
keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC,
Integers.valueOf(192));
keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd2_KeyTripleDES_CBC,
Integers.valueOf(128));
....
desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);
desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);
}
}Este error se debe nuevamente a copiar y pegar. Se agregan dos elementos idénticos al contenedor desAlgs . El desarrollador copió la última línea del código, pero olvidó corregir el número 3 por 2 en el nombre del campo.
Índice fuera de rango
V6025 Posiblemente el índice 'i' esté fuera de los límites. HSSTests.java (384)
public void testVectorsFromReference() throws Exception {
List<LMSigParameters> lmsParameters = new ArrayList<LMSigParameters>();
List<LMOtsParameters> lmOtsParameters = new ArrayList<LMOtsParameters>();
....
for (String line : lines) {
....
if (line.startsWith("Depth:")) {
....
} else if (line.startsWith("LMType:")) {
....
lmsParameters.add(LMSigParameters.getParametersForType(typ));
} else if (line.startsWith("LMOtsType:")) {
....
lmOtsParameters.add(LMOtsParameters.getParametersForType(typ));
}
}
....
for (int i = 0; i != lmsParameters.size(); i++) {
lmsParams.add(new LMSParameters(lmsParameters.get(i),
lmOtsParameters.get(i)));
}
}La adición de elementos a la colección lmsParameters y lmOtsParameters se realiza en el primer ciclo for , en diferentes ramas de la instrucción if / else . Luego, en el segundo ciclo for, se accede a los elementos de la colección en el índice i . Solo verifica que el índice i sea menor que el tamaño de la primera colección, y el tamaño de la segunda colección no se verifica en el ciclo for . Si los tamaños de las colecciones resultan ser diferentes, entonces es probable que pueda obtener una IndexOutOfBoundsException... Sin embargo, debe tenerse en cuenta que este es un código de método de prueba y esta advertencia no representa ningún peligro particular, ya que las colecciones se llenan con datos de prueba de un archivo creado previamente y, naturalmente, después de agregar elementos, las colecciones tienen el mismo tamaño.
Uso antes de la comprobación nula
V6060 La referencia 'params' se utilizó antes de que se verificara con un valor nulo. BCDSAPublicKey.java (54), BCDSAPublicKey.java (53)
BCDSAPublicKey(DSAPublicKeyParameters params) {
this.y = params.getY();
if (params != null) {
this.dsaSpec = new DSAParameterSpec(params.getParameters().getP(),
params.getParameters().getQ(),
params.getParameters().getG());
} else {
this.dsaSpec = null;
}
this.lwKeyParams = params;
}La primera línea del método establece y en params.getY () . Inmediatamente después de la asignación, se comprueba que la variable params sea nula . Si se permitió que los parámetros pudieran ser nulos en un método dado , debería haber realizado esta verificación antes de usar la variable.
Registro de entrada redundante si / si no
V6003 Se detectó el uso del patrón 'if (A) {...} else if (A) {...}'. Existe una probabilidad de presencia de errores lógicos. EnrollExample.java (108), EnrollExample.java (113)
public EnrollExample(String[] args) throws Exception {
....
for (int t = 0; t < args.length; t++) {
String arg = args[t];
if (arg.equals("-r")) {
reEnroll = true;
} ....
else if (arg.equals("--keyStoreType")) {
keyStoreType = ExampleUtils.nextArgAsString
("Keystore type", args, t);
t += 1;
} else if (arg.equals("--keyStoreType")) {
keyStoreType = ExampleUtils.nextArgAsString
("Keystore type", args, t);
t += 1;
} ....
}
}La instrucción if / else comprueba el valor de la cadena de argumentos dos veces para que sea igual a la cadena "- keyStoreType ". Naturalmente, la segunda comprobación es redundante y no tiene sentido. Sin embargo, esto no parece un error, ya que no hay otros parámetros en el texto de ayuda del argumento de la línea de comandos que no se manejen en el bloque if / else . Lo más probable es que se trate de un código redundante que debería eliminarse.
El método devuelve el mismo valor
V6014 Es extraño que este método siempre devuelva el mismo valor. XMSSSigner.java (129)
public AsymmetricKeyParameter getUpdatedPrivateKey() {
// if we've generated a signature return the last private key generated
// if we've only initialised leave it in place
// and return the next one instead.
synchronized (privateKey) {
if (hasGenerated) {
XMSSPrivateKeyParameters privKey = privateKey;
privateKey = null;
return privKey;
} else {
XMSSPrivateKeyParameters privKey = privateKey;
if (privKey != null) {
privateKey = privateKey.getNextKey();
}
return privKey;
}
}
}El analizador emite una advertencia porque este método siempre devuelve lo mismo. El comentario del método dice que dependiendo de si la firma se genera o no, se debe devolver la última clave generada o la siguiente. Aparentemente, este método le pareció sospechoso al analizador por una razón.
Resumamos
Como puede ver, todavía logramos encontrar errores en el proyecto de Bouncy Castle, y esto solo confirma una vez más que ninguno de nosotros escribe código perfecto. Pueden funcionar varios factores: el desarrollador está cansado, desatento, alguien lo distrajo ... Mientras el código sea escrito por una persona, siempre se producirán errores.
A medida que los proyectos crecen, encontrar problemas en el código se vuelve cada vez más difícil. Por tanto, en el mundo moderno, el análisis de código estático no es una metodología más, sino una necesidad real. Incluso si usa revisión de código, TDD o análisis dinámico, esto no significa que pueda rechazar el análisis estático. Se trata de metodologías completamente diferentes que se complementan a la perfección.
Al hacer del análisis estático una de las etapas de desarrollo, tiene la oportunidad de encontrar errores casi de inmediato, en el momento de escribir el código. Como resultado, los desarrolladores no necesitarán dedicar horas a depurar estos errores. Los probadores tendrán que reproducir muchos menos errores elusivos. Los usuarios recibirán un programa más confiable y estable.
En general, asegúrese de utilizar el análisis estático en sus proyectos. Lo hacemos nosotros mismos y te lo recomendamos :)
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Irina Polynkina. Unicornios en guardia para su seguridad: Explorando el código del castillo hinchable .