# Единороги на страже вашей безопасности: исследуем код Bouncy Castle Хотите увидеть новую порцию ошибок, найденных статическим анализатором PVS\-Studio для Java? Тогда присоединяйтесь к прочтению статьи\! В этот раз объектом проверки стал проект Bouncy Castle\. Самые интересные фрагменты кода, как обычно, ждут вас ниже\. ![0775_BouncyCastleProject_ru/image1.png](https://import.viva64.com/docx/blog/0775_BouncyCastleProject_ru/image1.png) ## Немного о PVS\-Studio [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) – инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ\. На момент написания статьи статический анализ реализован для программ, написанных на языках программирования C, C\+\+, C\# и Java\. Анализатор для Java самое молодое направление PVS\-Studio\. Несмотря на это, в поиске дефектов в коде он не уступает своим старшим братьям\. Это связано с тем, что Java анализатор использует всю мощь механизмов из C\+\+ анализатора\. Об этом уникальном союзе Java и С\+\+ можно почитать [здесь](https://pvs-studio.ru/ru/blog/posts/java/0572/)\. На данный момент для более удобного использования [существуют](https://pvs-studio.ru/ru/docs/manual/6703/) плагины для Gradle, Maven и IntelliJ IDEA\. Если вы знакомы с платформой непрерывного контроля качества SonarQube, то, возможно, вам будет интересна идея поиграться с [интеграцией](https://pvs-studio.ru/ru/docs/manual/0005/) результата анализа\. ## Немного о Bouncy Castle [Bouncy Castle](https://github.com/bcgit/bc-java) – это пакет с реализацией криптографических алгоритмов, написанный на языке программирования Java \(также существует реализация на C\#, но в данной статье речь пойдет не об этом\)\. Данная библиотека дополняет стандартное криптографическое расширение \(JCE\) и содержит API, подходящий для использования в любой среде \(включая J2ME\)\. Программисты могут свободно использовать все возможности данной библиотеки\. Реализация большого количества алгоритмов и протоколов делают данный проект очень интересным для разработчиков ПО, использующих криптографию\. ## Приступаем к проверке Bouncy Castle – довольно серьезный проект, ведь любая ошибка в такой библиотеке может снизить надежность системы шифрования\. Поэтому сначала мы даже сомневались, сможем ли мы найти в данной библиотеке хоть что\-то интересное, или же все ошибки уже были найдены и исправлены до нас\. Скажем сразу, что наш Java анализатор нас не подвел :\) Естественно, мы не можем описать все предупреждения анализатора в одной статье, но у нас есть бесплатная лицензия для разработчиков, которые развивают [open source](https://pvs-studio.ru/ru/order/open-source-license/) проекты\. При желании у нас можно запросить данную лицензию и самостоятельно проанализировать проект с помощью PVS\-Studio\. А мы приступим к рассмотрению самых интересных фрагментов кода, которые были обнаружены\. ### Недостижимый код [V6019](https://pvs-studio.ru/ru/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; .... } } } ``` Значение переменной _height_ в методе не меняется, поэтому счетчик _i_ в цикле _for_ не может быть больше, чем 1024 \(1 << 10\)\. Однако, в операторе _switch_ второй _case_ проверяет _i_ на соответствие значению 0x0822 \(2082\)\. Естественно, проверка байта _signatures\[1\]_ никогда не будет выполнена\. Так как это код тестового метода, ничего страшного здесь нет\. Просто разработчикам нужно обратить внимание на то, что тестирование одного из байтов никогда не производится\. ### Идентичные подвыражения [V6001](https://pvs-studio.ru/ru/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; } ``` В данном фрагменте кода в операторе _return_ дважды производится проверка _tag_ \=\= _PacketTags\.SEKRET\_KEY_\. По аналогии с проверкой публичного ключа, последняя проверка должна быть на равенство _tag_ и _PacketTags\.SECRET\_SUBKEY_\. ### Идентичный код в if / else [V6004](https://pvs-studio.ru/ru/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); } } ``` В данном примере метод возвращает одинаковые экземпляры класса _GenericKey_ вне зависимости от того, выполнено условие в _if_ или нет\. Понятно, что код в ветвях _if / else_ должен отличаться, иначе проверка в _if_ вообще не имеет смысла\. Здесь программиста явно подвел копипаст\. ### Выражение всегда ложно [V6007](https://pvs-studio.ru/ru/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(); } } ``` Здесь переменной _nGroups_ в блоках кода _if / else_ присваивается значение, которое используется, но нигде не меняется\. Выражение в операторе_ if_ всегда будет ложным, т\.к\. все возможные значения для _nGroups_: 2, 3, 4, 5 и 6 – меньше 8\. Анализатор понимает, что метод _panic\(\)_ никогда не будет выполнен, и поэтому бьет тревогу\. Но здесь, скорее всего, было использовано "защитное программирование", и беспокоиться не о чем\. ### Добавление одинаковых элементов [V6033](https://pvs-studio.ru/ru/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); } } ``` Эта ошибка снова из\-за копипаста\. В контейнер _desAlgs_ добавляются два одинаковых элемента\. Разработчик скопировал последнюю строчку кода, но исправить цифру 3 на 2 в имени поля забыл\. ### Индекс за пределами диапазона [V6025](https://pvs-studio.ru/ru/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))); } } ``` Добавление элементов в коллекции _lmsParameters_ и _lmOtsParameters_ производится в первом цикле _for_, в разных ветках оператора _if / else_\. Затем, во втором цикле _for,_ осуществляется доступ к элементам коллекций по индексу _i_\. При этом проверяется только то, что индекс _i_ меньше, чем размер первой коллекции, а размер второй коллекции в цикле _for_ не проверяется\. Если размеры коллекций окажутся разными, то, вполне вероятно, можно получить _IndexOutOfBoundsException_\. Правда, стоит отметить, что это код тестового метода, и особой опасности данное предупреждение не представляет, т\.к\. коллекции заполняются тестовыми данными из заранее созданного файла и, естественно, по окончанию добавления элементов, коллекции имеют одинаковый размер\. ### Использование до проверки на null [V6060](https://pvs-studio.ru/ru/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; } ``` В первой строке метода переменной _y_ присваивается значение _params\.getY\(\)_\. Сразу же после присваивания переменная _params_ проверяется на _null_\. Если допускается, что в данном методе _params_ может быть _null_, следовало сделать данную проверку перед тем, как использовать переменную\. ### Избыточная проверка в if / else [V6003](https://pvs-studio.ru/ru/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; } .... } } ``` В операторе _if / else_ значение строки _args_ дважды проверяется на равенство со строкой "\-\-_keyStoreType_"\. Естественно, вторая проверка избыточна, и никакого смысла в ней нет\. Однако, на ошибку это не похоже, т\.к\. в тексте справки по аргументам командной строки нет других параметров, которые бы не были обработаны в блоке _if / else_\. Скорее всего, это избыточный код, который стоит удалить\. ### Метод возвращает одно и то же значение [V6014](https://pvs-studio.ru/ru/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; } } } ``` Анализатор выдает предупреждение из\-за того, что данный метод всегда возвращает одно и то же\. В комментарии к методу говорится, что в зависимости от того, сгенерирована подпись или нет, должен возвращаться либо последний сгенерированный ключ, либо следующий\. По всей видимости, анализатору данный метод показался подозрительным не просто так\. ## Подведем итоги Как вы видите, ошибки в проекте Bouncy Castle нам все же удалось обнаружить, и это только лишний раз подтверждает, что никто из нас не пишет идеальный код\. Разные факторы могут сработать: разработчик устал, невнимателен, его кто\-то отвлек\.\.\. Пока код пишет человек – ошибки будут встречаться всегда\. По мере роста проектов находить проблемы в коде становится все сложнее и сложнее\. Поэтому в современном мире статический анализ кода – это не просто очередная методология, а реальная необходимость\. Даже если вы используете code review, TDD или динамический анализ – это не значит, что можно отказаться от статического анализа\. Это совершенно разные методологии, которые прекрасно дополняют друг друга\. Сделав статический анализ одним из этапов разработки, вы получаете возможность находить ошибки практически сразу, в момент написания кода\. Как следствие, разработчикам не нужно будет тратить на эти ошибки долгие часы отладки\. Тестировщикам придется воспроизводить гораздо меньше неуловимых багов\. Пользователи получат программу, которая работает надежнее и стабильнее\. В общем, обязательно используйте статический анализ в своих проектах\! Мы сами это делаем, и вам рекомендуем :\)