Do you want to see a new batch of errors found by the PVS-Studio for Java static analyzer? Then join the reading of the article! This time, the Bouncy Castle project became the object of verification. The most interesting code snippets, as usual, are waiting for you below.
A little about PVS-Studio
PVS-Studio is a tool for identifying errors and potential vulnerabilities in the source code of programs. At the time of this writing, static analysis is implemented for programs written in the C, C ++, C # and Java programming languages.
The analyzer for Java is the youngest line of PVS-Studio. Despite this, he is not inferior to his older brothers in finding defects in the code. This is due to the fact that the Java analyzer uses the full power of the mechanisms from the C ++ analyzer. You can read about this unique union of Java and C ++ here .
At the moment, there are plugins for Gradle, Maven and IntelliJ IDEA for more convenient use . If you are familiar with the SonarQube continuous quality control platform, then you might be interested in the idea of playing withintegration of the analysis result.
A little about Bouncy Castle
Bouncy Castle is a package with the implementation of cryptographic algorithms written in the Java programming language (there is also an implementation in C #, but this article is not about that). This library complements the Standard Cryptographic Extension (JCE) and contains an API suitable for use in any environment (including J2ME).
Programmers are free to use all the features of this library. The implementation of a large number of algorithms and protocols make this project very interesting for software developers using cryptography.
Let's start checking
Bouncy Castle is a rather serious project, because any mistake in such a library can reduce the reliability of the encryption system. Therefore, at first we even doubted whether we could find at least something interesting in this library, or whether all the errors had already been found and fixed before us. Let's say right away that our Java analyzer did not disappoint us :)
Naturally, we cannot describe all the analyzer warnings in one article, but we have a free license for developers who develop open source projects. If you wish, you can request this license from us and independently analyze the project using PVS-Studio.
And we will start looking at the most interesting code snippets that have been discovered.
Unreachable code
V6019 Unreachable code detected. It is possible that an error is present. 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;
....
}
}
}The value of the height variable in the method does not change, so the i counter in the for loop cannot be greater than 1024 (1 << 10). However, in the switch statement, the second case checks i against the value 0x0822 (2082). Naturally, the signatures [1] byte will never be verified .
Since this is a test method code, there is nothing to worry about. It's just that developers need to pay attention to the fact that testing of one of the bytes is never done.
Identical subexpressions
V6001 There are identical sub-expressions 'tag == PacketTags.SECRET_KEY' to the left and to the right of the '||' operator. 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;
}In this code snippet, the statement in return , double-checked tag == PacketTags.SECRET_KEY . Similar to checking the public key, the last check should be for equality between tag and PacketTags.SECRET_SUBKEY .
Identical code in if / else
V6004 The 'then' statement is equivalent to the 'else' statement. 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);
}
}In this example, the method returns the same instances of the GenericKey class , regardless of whether the condition in the if is satisfied or not. It is clear that the code in the if / else branches must be different, otherwise the check in the if does not make sense at all. Here the programmer was clearly let down by copy-paste.
Expression is always false
V6007 Expression '! (NGroups <8)' is always false. 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();
}
}Here, the nGroups variable in the if / else code blocks is assigned a value that is used but does not change anywhere. The expression in the if statement will always be false, because all possible values for nGroups : 2, 3, 4, 5 and 6 are less than 8.
The analyzer understands that the panic () method will never be executed, and therefore raises the alarm. But here, most likely, "defensive programming" was used, and there is nothing to worry about.
Adding the same elements
V6033 An item with the same key 'PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC' has already been added. 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);
}
}This error is again due to copy-paste. Two identical elements are added to the desAlgs container . The developer copied the last line of the code, but forgot to correct the number 3 by 2 in the field name.
Index out of range
V6025 Possibly index 'i' is out of bounds. 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)));
}
}Adding elements to the lmsParameters and lmOtsParameters collection is done in the first for loop , in different branches of the if / else statement . Then, in the second for loop , the collection items are accessed at index i . It only checks that the index i is less than the size of the first collection, and the size of the second collection is not checked in the for loop . If the sizes of the collections turn out to be different, then it is likely that you can get an IndexOutOfBoundsException... However, it should be noted that this is a test method code, and this warning does not pose any particular danger, since the collections are filled with test data from a pre-created file and, of course, after adding elements, the collections have the same size.
Usage before null checking
V6060 The 'params' reference was utilized before it was verified against null. 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;
}The first line of the method sets y to params.getY () . Immediately after the assignment, the params variable is checked for null . If it is allowed that params can be null in this method , you should have done this check before using the variable.
Redundant check in if / else
V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. 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;
} ....
}
}In the if / else statement, the value of the args string is checked twice for equality with the string "- keyStoreType ". Naturally, the second check is redundant and makes no sense. However, this does not look like an error, since there are no other parameters in the command line argument help text that are not processed in the if / else block . This is most likely redundant code that should be removed.
The method returns the same value
V6014 It's odd that this method always returns one and the same value. 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;
}
}
}The analyzer issues a warning because this method always returns the same. The method comment says that depending on whether the signature was generated or not, either the last generated key should be returned or the next one. Apparently, this method seemed suspicious to the analyzer for a reason.
Let's sum up
As you can see, we still managed to find errors in the Bouncy Castle project, and this only confirms once again that none of us writes perfect code. Various factors can work: the developer is tired, inattentive, someone distracted him ... As long as the code is written by a person, errors will always occur.
As projects grow, finding problems in the code becomes more and more difficult. Therefore, in the modern world, static code analysis is not just another methodology, but a real necessity. Even if you use code review, TDD or dynamic analysis, this does not mean that you can refuse static analysis. These are completely different methodologies that complement each other perfectly.
By making static analysis one of the development stages, you get the opportunity to find errors almost immediately, at the time of writing the code. As a result, developers will not need to spend hours of debugging on these errors. Testers will have to reproduce far fewer elusive bugs. Users will get a program that works more reliably and more stable.
In general, be sure to use static analysis in your projects! We do it ourselves, and we recommend it to you :)
If you want to share this article with an English-speaking audience, please use the translation link: Irina Polynkina. Unicorns on Guard for Your Safety: Exploring the Bouncy Castle Code .