Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Хотите увидеть новую порцию ошибок, найденных статическим анализатором PVS-Studio для Java? Тогда присоединяйтесь к прочтению статьи! В этот раз объектом проверки стал проект Bouncy Castle. Самые интересные фрагменты кода, как обычно, ждут вас ниже.
Немного о PVS-Studio
PVS-Studio – инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ. На момент написания статьи статический анализ реализован для программ, написанных на языках программирования C, C++, C# и Java.
Анализатор для Java самое молодое направление PVS-Studio. Несмотря на это, в поиске дефектов в коде он не уступает своим старшим братьям. Это связано с тем, что Java анализатор использует всю мощь механизмов из C++ анализатора. Об этом уникальном союзе Java и С++ можно почитать здесь.
На данный момент для более удобного использования существуют плагины для Gradle, Maven и IntelliJ IDEA. Если вы знакомы с платформой непрерывного контроля качества SonarQube, то, возможно, вам будет интересна идея поиграться с интеграцией результата анализа.
Немного о Bouncy Castle
Bouncy Castle – это пакет с реализацией криптографических алгоритмов, написанный на языке программирования Java (также существует реализация на C#, но в данной статье речь пойдет не об этом). Данная библиотека дополняет стандартное криптографическое расширение (JCE) и содержит API, подходящий для использования в любой среде (включая J2ME).
Программисты могут свободно использовать все возможности данной библиотеки. Реализация большого количества алгоритмов и протоколов делают данный проект очень интересным для разработчиков ПО, использующих криптографию.
Приступаем к проверке
Bouncy Castle – довольно серьезный проект, ведь любая ошибка в такой библиотеке может снизить надежность системы шифрования. Поэтому сначала мы даже сомневались, сможем ли мы найти в данной библиотеке хоть что-то интересное, или же все ошибки уже были найдены и исправлены до нас. Скажем сразу, что наш Java анализатор нас не подвел :)
Естественно, мы не можем описать все предупреждения анализатора в одной статье, но у нас есть бесплатная лицензия для разработчиков, которые развивают open source проекты. При желании у нас можно запросить данную лицензию и самостоятельно проанализировать проект с помощью PVS-Studio.
А мы приступим к рассмотрению самых интересных фрагментов кода, которые были обнаружены.
Недостижимый код
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;
....
}
}
}Значение переменной height в методе не меняется, поэтому счетчик i в цикле for не может быть больше, чем 1024 (1 << 10). Однако, в операторе switch второй case проверяет i на соответствие значению 0x0822 (2082). Естественно, проверка байта signatures[1] никогда не будет выполнена.
Так как это код тестового метода, ничего страшного здесь нет. Просто разработчикам нужно обратить внимание на то, что тестирование одного из байтов никогда не производится.
Идентичные подвыражения
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;
}В данном фрагменте кода в операторе return дважды производится проверка tag == PacketTags.SEKRET_KEY. По аналогии с проверкой публичного ключа, последняя проверка должна быть на равенство tag и PacketTags.SECRET_SUBKEY.
Идентичный код в 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);
}
}В данном примере метод возвращает одинаковые экземпляры класса GenericKey вне зависимости от того, выполнено условие в if или нет. Понятно, что код в ветвях if / else должен отличаться, иначе проверка в if вообще не имеет смысла. Здесь программиста явно подвел копипаст.
Выражение всегда ложно
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();
}
}Здесь переменной nGroups в блоках кода if / else присваивается значение, которое используется, но нигде не меняется. Выражение в операторе if всегда будет ложным, т.к. все возможные значения для nGroups: 2, 3, 4, 5 и 6 – меньше 8.
Анализатор понимает, что метод panic() никогда не будет выполнен, и поэтому бьет тревогу. Но здесь, скорее всего, было использовано "защитное программирование", и беспокоиться не о чем.
Добавление одинаковых элементов
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);
}
}Эта ошибка снова из-за копипаста. В контейнер desAlgs добавляются два одинаковых элемента. Разработчик скопировал последнюю строчку кода, но исправить цифру 3 на 2 в имени поля забыл.
Индекс за пределами диапазона
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)));
}
}Добавление элементов в коллекции lmsParameters и lmOtsParameters производится в первом цикле for, в разных ветках оператора if / else. Затем, во втором цикле for, осуществляется доступ к элементам коллекций по индексу i. При этом проверяется только то, что индекс i меньше, чем размер первой коллекции, а размер второй коллекции в цикле for не проверяется. Если размеры коллекций окажутся разными, то, вполне вероятно, можно получить IndexOutOfBoundsException. Правда, стоит отметить, что это код тестового метода, и особой опасности данное предупреждение не представляет, т.к. коллекции заполняются тестовыми данными из заранее созданного файла и, естественно, по окончанию добавления элементов, коллекции имеют одинаковый размер.
Использование до проверки на 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;
}В первой строке метода переменной y присваивается значение params.getY(). Сразу же после присваивания переменная params проверяется на null. Если допускается, что в данном методе params может быть null, следовало сделать данную проверку перед тем, как использовать переменную.
Избыточная проверка в 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;
} ....
}
}В операторе if / else значение строки args дважды проверяется на равенство со строкой "--keyStoreType". Естественно, вторая проверка избыточна, и никакого смысла в ней нет. Однако, на ошибку это не похоже, т.к. в тексте справки по аргументам командной строки нет других параметров, которые бы не были обработаны в блоке if / else. Скорее всего, это избыточный код, который стоит удалить.
Метод возвращает одно и то же значение
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;
}
}
}Анализатор выдает предупреждение из-за того, что данный метод всегда возвращает одно и то же. В комментарии к методу говорится, что в зависимости от того, сгенерирована подпись или нет, должен возвращаться либо последний сгенерированный ключ, либо следующий. По всей видимости, анализатору данный метод показался подозрительным не просто так.
Подведем итоги
Как вы видите, ошибки в проекте Bouncy Castle нам все же удалось обнаружить, и это только лишний раз подтверждает, что никто из нас не пишет идеальный код. Разные факторы могут сработать: разработчик устал, невнимателен, его кто-то отвлек… Пока код пишет человек – ошибки будут встречаться всегда.
По мере роста проектов находить проблемы в коде становится все сложнее и сложнее. Поэтому в современном мире статический анализ кода – это не просто очередная методология, а реальная необходимость. Даже если вы используете code review, TDD или динамический анализ – это не значит, что можно отказаться от статического анализа. Это совершенно разные методологии, которые прекрасно дополняют друг друга.
Сделав статический анализ одним из этапов разработки, вы получаете возможность находить ошибки практически сразу, в момент написания кода. Как следствие, разработчикам не нужно будет тратить на эти ошибки долгие часы отладки. Тестировщикам придется воспроизводить гораздо меньше неуловимых багов. Пользователи получат программу, которая работает надежнее и стабильнее.
В общем, обязательно используйте статический анализ в своих проектах! Мы сами это делаем, и вам рекомендуем :)
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Irina Polynkina. Unicorns on Guard for Your Safety: Exploring the Bouncy Castle Code.
