# Unicorns on guard for your safety: Exploring the Bouncy Castle code 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\. ![0775_BouncyCastleProject/image1.png](https://import.viva64.com/docx/blog/0775_BouncyCastleProject/image1.png) ## Briefly about PVS\-Studio [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) 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](https://pvs-studio.com/en/blog/posts/java/0572/) about this unique union of Java and C\+\+\. Currently, there are [plugins](https://pvs-studio.com/en/docs/manual/6703/) 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](https://pvs-studio.com/en/docs/manual/0005/) of the analysis result\. ## Briefly about Bouncy Castle [Bouncy Castle](https://github.com/bcgit/bc-java) 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](https://pvs-studio.com/en/order/open-source-license/) 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](https://pvs-studio.com/en/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. XMSSTest\.java\(170\) ```cpp 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 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](https://pvs-studio.com/en/docs/warnings/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\) ```cpp 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\.SEKRET\_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](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. BcAsymmetricKeyUnwrapper\.java\(36\), BcAsymmetricKeyUnwrapper\.java\(40\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '\!\(nGroups < 8\)' is always false\. CBZip2OutputStream\.java\(753\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/v6033/) An item with the same key 'PKCSObjectIdentifiers\.pbeWithSHAAnd3\_KeyTripleDES\_CBC' has already been added\. PKCS12PBEUtils\.java\(50\), PKCS12PBEUtils\.java\(49\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/v6025/) Possibly index 'i' is out of bounds\. HSSTests\.java\(384\) ```cpp public void testVectorsFromReference() throws Exception { List lmsParameters = new ArrayList(); List lmOtsParameters = new ArrayList(); .... 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](https://pvs-studio.com/en/docs/warnings/v6060/) The 'params' reference was utilized before it was verified against null\. BCDSAPublicKey\.java\(54\), BCDSAPublicKey\.java\(53\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/v6014/) It's odd that this method always returns one and the same value\. XMSSSigner\.java\(129\) ```cpp 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 :\)