Небольшая заметка о результатах проверки проекта OpenSSL с помощью анализатора PVS-Studio. Я анализировал версию openssl-0.9.8-stable-SNAP-20121208.
Недавно я написал заметку "Безопасность, безопасность! А вы её тестируете?" о проверке проекта TOR. За компанию я упомянул библиотеку OpenSSL, которая входит в проект TOR.
Статья активно обсуждалась на некоторых программистских ресурсах. Оказалось, что программистов весьма заботит качество библиотеки OpenSSL. Однако, я не смог ответить на некоторые вопросы касающиеся этой библиотеки. Также меня обвинили в том, что я не уведомил разработчиков библиотеки OpenSSL обо всех подозрительных местах.
Хочу пояснить эту ситуацию. Дело в том, что изучая TOR, я не планировал проверять библиотеку OpenSSL и изучать результаты её анализа. Эта библиотека просто случайно попалась под руку. В проект TOR была включена, какая то версия OpenSSL. Она и была обработана анализатором PVS-Studio за компанию.
Теперь я решил исправить ситуацию и специально скачал и проверил свежую версию библиотек OpenSSL.
Если честно, рассказывать мне особенно нечего. Мне не удалось обнаружить почти ничего подозрительного. Те ошибки, которые я описывал в статье, уже исправлены. OpenSSL – это качественный проект. Библиотека уже проверена множеством инструментов (Clang, Cppcheck, Coverity, DoubleCheck, Coccinelle, Klocwork и другими). В общем, библиотека хорошо вылизана. И найти в ней хотя бы одну ошибку, это будет уже подвиг.
Итак, расскажу, что подозрительного я нашел в коде библиотеки OpenSSL. Скорее всего, это несущественные недочеты, а не ошибки. Но надо хоть про что-то написать. :)
EVP_PKEY *STORE_get_private_key(....)
{
STORE_OBJECT *object;
....
if (!object || !object->data.key || !object->data.key)
{
STOREerr(STORE_F_STORE_GET_PRIVATE_KEY,
STORE_R_FAILED_GETTING_KEY);
return 0;
}
....
}Диагностическое сообщение PVS-Studio: V501 There are identical sub-expressions '!object->data.key' to the left and to the right of the '||' operator. str_lib.c 475
Два раза повторяется условие "!object->data.key". Скорее всего, вторая проверка просто лишняя и ничего страшного тут нет. Если вдруг окажется, что здесь хотели проверить другой член класса, тогда это конечно беда.
Эту странную проверку можно встретить ещё в трех местах файла str_lib.c (видимо это Copy-Paste):
Есть несколько мест, где указатель в начале используется, а затем проверяется на нулевое значение. Но достаточно подозрительным мне показался только один фрагмент:
int OBJ_obj2txt(char *buf, int buf_len,
const ASN1_OBJECT *a, int no_name)
{
....
if ((a == NULL) || (a->data == NULL)) {
buf[0]='\0';
return(0);
}
....
if (buf)
....
}Диагностическое сообщение PVS-Studio: V595 The 'buf' pointer was utilized before it was verified against nullptr. Check lines: 448, 461. obj_dat.c 448
В начале проверяется, что 'a' или 'a->data' равны нулю. Если это так, то используется указатель 'buf'. Но ведь и указатель 'buf' тоже может быть равен нулю. На это намекает проверка '"if (buf)", расположенная чуть ниже в коде.
В следующем фрагменте кода, анализатору PVS-Studio кажется всё-таки удалось найти настоящую полновесную ошибку.
int ssl3_get_cert_verify(SSL *s)
{
int type=0,i,j;
....
if ((peer != NULL) && (type | EVP_PKT_SIGN))
{
al=SSL_AD_UNEXPECTED_MESSAGE;
SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
SSL_R_MISSING_VERIFY_MESSAGE);
goto f_err;
}
....
}Диагностическое сообщение PVS-Studio: V617 Consider inspecting the condition. The '0x0010' argument of the '|' bitwise operation contains a non-zero value. s3_srvr.c 2394
Выражение "(type | EVP_PKT_SIGN)" всегда истинно. Возможно, здесь должно было быть написано: "type & EVP_PKT_SIGN".
Есть несколько бессмысленных проверок следующего вида:
int MAIN(int argc, char **argv)
{
....
long dsa_c[DSA_NUM][2];
....
if (dsa_c[i] == 0)
{
dsa_c[i][0]=1;
dsa_c[i][1]=1;
}
....
}Диагностическое сообщение PVS-Studio: V600 Consider inspecting the condition. The 'dsa_c[i]' pointer is always not equal to NULL. speed.c 1486
Здесь 'dsa_c' это двумерный массив. Поэтому выражение "dsa_c[i] == 0" всегда истинно и не имеет смысла. Рядом есть код следующего вида:
if (rsa_c[i][0] == 0)
{
rsa_c[i][0]=1;
rsa_c[i][1]=20;
}Возможно и с массивом 'dsa_c' следует работать таким же образом. Тогда код должен выглядеть так:
if (dsa_c[i][0] == 0)
{
dsa_c[i][0]=1;
dsa_c[i][1]=1;
}Такая странная проверка есть ещё в нескольких местах:
Есть ряд мелких недочетов. Это точно не ошибки. Просто лишний код. Вот пример избыточности в условии:
int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
int c;
....
c= *(s++);
if (!( ((c >= 'a') && (c <= 'z')) ||
((c >= 'A') && (c <= 'Z')) ||
(c == ' ') ||
((c >= '0') && (c <= '9')) ||
(c == ' ') || (c == '\'') ||
(c == '(') || (c == ')') ||
(c == '+') || (c == ',') ||
(c == '-') || (c == '.') ||
(c == '/') || (c == ':') ||
(c == '=') || (c == '?')))
ia5=1;
....
}Диагностическое сообщение PVS-Studio: V501 There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 76
То, что символ является пробелом, повторяется два раза. Просто по невнимательности проверок написали два раза. Ничего страшного. Видел ещё подобные места, но писать про них не интересно.
Я долго не мог понять, в некоторых местах анализатор нашел приведение memsize-типа к 32-битному типу, а затем обратно к memsize-типу. Вот одно их таких мест:
int ec_GFp_simple_points_make_affine(const EC_GROUP *group,
size_t num, EC_POINT *points[], BN_CTX *ctx)
{
BIGNUM **heap = NULL;
size_t pow2 = 0;
....
heap = OPENSSL_malloc(pow2 * sizeof heap[0]);
....
}Диагностическое сообщение PVS-Studio: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'pow2'. ecp_smpl.c 1576
Оказывается, макрос OPENSSL_malloc написан некрасиво.
void *CRYPTO_malloc(int num, const char *file, int line);
#define OPENSSL_malloc(num) CRYPTO_malloc((int)num,__FILE__,__LINE__)В результате, после препроцессирования получается следующий код:
heap = CRYPTO_malloc ((int)pow2 * sizeof heap[0], __FILE__,__LINE__);В результате начинается бестолковые приведения типов. В начале переменная 'pow2' типа size_t явно приводится к типу 'int'. Затем при умножении на 'sizeof()' тип выражения становится опять size_t. После этого при вызове функции CRYPTO_malloc() вновь происходит приведение к типу 'int'. На этот раз приведение уже неявное.
В общем, от этого приведения типа, нет никакого толку. Только потенциальное повод допустить ошибку. Например, можно написать что-то такое:
int *p1, *p2;
int x, y;
....
p = OPENSSL_malloc(p1 == p2 ? x : y);На 64-битной системе указатель 'p1' потеряет старшие биты, и результат сравнения может дать неверный результат.
Это конечно выдуманный пример, но всё равно не хорошо создавать такие макросы. Как минимум, следует переписать макрос так:
#define OPENSSL_malloc(num) CRYPTO_malloc((int)(num),
__FILE__,__LINE__)А ещё лучше вообще не использовать здесь тип 'int'. Размер выделяемой памяти должен передаваться в переменной memsize-типа. Например, это тип 'size_t'.
Спасибо всем за внимание. Буду рад, если эта заметка поможет как-то улучшить библиотеку OpenSSL. Как всегда прошу авторов не ограничиваться подозрительными местами, описанными здесь. Лучше ещё раз проверить библиотеку и самостоятельно изучить отчёт. Мы бесплатно предоставляем разработчикам open-source библиотек ключ на некоторое время. Возможно, удастся заметить опасные места, которым я не придал значения.