Would you like to see a new batch of errors found by the PVS-Studio static analyzer for Java? Then keep reading the article! This time the Bouncy Castle project is to be checked. The most interesting code snippets, as usual, are waiting for you below.
Briefly about PVS-Studio
PVS-Studio is a tool designed to detect errors and potential vulnerabilities in the source code of programs. At the time of writing, static analysis is implemented for programs written in the C, C++, C#, and Java programming languages.
The analyzer for Java is the youngest direction of PVS-Studio. Despite this, it is not inferior to its older brothers in the search for defects in code. This is due to the fact that the Java analyzer uses all the power of the mechanisms from the C++ analyzer. You can read here about this unique union of Java and C++.
Currently, there are plugins for Gradle, Maven, and IntelliJ IDEA for more convenient use. If you are familiar with the SonarQube continuous quality control platform, you might be interested in playing around with the integration of the analysis result.
Briefly about Bouncy Castle
Bouncy Castle is a package implementing 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 many algorithms and protocols makes this project very interesting for software developers who use cryptography.
Getting started with the check
Bouncy Castle is quite a responsible project, because any error in such a library can reduce the reliability of the encryption system. Therefore, at first we even doubted whether we would be able to find anything interesting in this library, or whether all the errors had already been found and fixed before us. Let me tell you right away — our Java analyzer hasn't let us down :)
Sure, we can't 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 analyze the project yourself using PVS-Studio.
So, let's start reviewing the most interesting found code fragments.
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 icounter in the for loop can't be greater than 1024 (1 << 10). However, in the switch statement, the second case checks i for the value 0x0822 (2082). Obviously, the signatures[1] byte will never be checked.
Since this is the code of the test method, there is nothing wrong here. Developers just need to pay attention to the fact that one of bytes is never tested.
Identical sub-expressions
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, tag == PacketTags.SECRET_KEY is checked twice in the return statement. Similar to the public key check, the last check should be for equality of 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 identical instances of the GenericKey class, regardless of whether the if condition is met or not. Clearly, the code in the if / else branches must be different, otherwise checking in if does not make sense at all. It is copy-paste that got the developer into trouble here.
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 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 perceives that the panic() method will never be executed, and therefore gives the alarm. But most likely "defensive programming" was applied here, and there is nothing to worry about.
Adding identical 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 because of copy-paste. Two identical elements are added to the desAlgs container. The developer copied the last line of code but forgot to change the number 3 to 2 in the field name.
Index outside the 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)));
}
}Elements are added to the lmsParameters and lmOtsParameters collections in the first for loop, in different branches of the if / else statement. Then, in the second for loop, the collection elements are accessed by the index i. This only checks if the i index is smaller than the size of the first collection, whereas the size of the second collection is not checked in the for loop. If the collection sizes are different, it is likely that you can get the IndexOutOfBoundsException. However, it is worth noting that this is the code of a test method, and this warning is not particularly dangerous, because collections are filled with test data from a pre-created file and, of course, when the elements are added, the collections have the same size.
Using before checking for null
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;
}In the first line of the method, the y variable is assigned the params.getY() value. Immediately after assignment, the params variable is checked for null. If it is allowed that params can be null in the method, the author should have placed 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". Of course, the second check is redundant, and there is no point in it. Still it does not look like an error, because there are no other parameters in the help text for command-line arguments that are not processed in the if / else block. Most likely, this is redundant code that should be deleted.
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 this warning as this method always returns one and the same thing. The method's comment states that depending on whether the signature is generated or not, either the last generated key or the next one should be returned. Apparently, the analyzer found this method suspicious for a reason.
Let's recap
As you can see, we still managed to find errors in the Bouncy Castle project. This confirms once again that none of us writes perfect code. Various reasons play their part: a developer is tired, inattentive, or distracted by someone… As long as the code is written by a human, errors will always occur.
As projects grow, it becomes more and more difficult to find problems in the code. 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 opt out of static analysis. These are completely different methodologies that perfectly complement each other.
By making static analysis one of the development stages, you can find errors almost immediately, at the time of writing the code. As a result, developers will not need to spend long hours debugging these errors. Q&A engineers will have to reproduce much fewer elusive bugs. Users will get a program that works more reliably and stably.
Anyway, be sure to use static analysis in your projects! We do this ourselves, and recommend the same to you :)