# Немного про OpenSSL Небольшая заметка о результатах проверки проекта OpenSSL с помощью анализатора PVS\-Studio\. Я анализировал версию openssl\-0\.9\.8\-stable\-SNAP\-20121208\. ## О проверке OpenSSL Недавно я написал заметку "[Безопасность, безопасность\! А вы её тестируете?](https://pvs-studio.ru/ru/blog/posts/cpp/0178/)" о проверке проекта TOR\. За компанию я упомянул библиотеку OpenSSL, которая входит в проект TOR\. Статья активно обсуждалась на некоторых программистских ресурсах\. Оказалось, что программистов весьма заботит качество библиотеки [OpenSSL](https://ru.wikipedia.org/wiki/OpenSSL)\. Однако, я не смог ответить на некоторые вопросы касающиеся этой библиотеки\. Также меня обвинили в том, что я не уведомил разработчиков библиотеки OpenSSL обо всех подозрительных местах\. Хочу пояснить эту ситуацию\. Дело в том, что изучая TOR, я не планировал проверять библиотеку OpenSSL и изучать результаты её анализа\. Эта библиотека просто случайно попалась под руку\. В проект TOR была включена, какая то версия OpenSSL\. Она и была обработана анализатором PVS\-Studio за компанию\. Теперь я решил исправить ситуацию и специально скачал и проверил свежую версию библиотек OpenSSL\. Если честно, рассказывать мне особенно нечего\. Мне не удалось обнаружить почти ничего подозрительного\. Те ошибки, которые я описывал в статье, уже исправлены\. OpenSSL – это качественный проект\. Библиотека уже проверена множеством инструментов \(Clang, Cppcheck, Coverity, DoubleCheck, Coccinelle, Klocwork и другими\)\. В общем, библиотека хорошо вылизана\. И найти в ней хотя бы одну ошибку, это будет уже подвиг\. Итак, расскажу, что подозрительного я нашел в коде библиотеки OpenSSL\. Скорее всего, это несущественные недочеты, а не ошибки\. Но надо хоть про что\-то написать\. :\) ## Подозрительный фрагмент N1 ```cpp 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\): * V501 There are identical sub\-expressions '\!object\-\>data\.key' to the left and to the right of the '\|\|' operator\. str\_lib\.c 616 * V501 There are identical sub\-expressions '\!object\-\>data\.key' to the left and to the right of the '\|\|' operator\. str\_lib\.c 670 * V501 There are identical sub\-expressions '\!object\-\>data\.key' to the left and to the right of the '\|\|' operator\. str\_lib\.c 811 ## Подозрительный фрагмент N2 Есть несколько мест, где указатель в начале используется, а затем проверяется на нулевое значение\. Но достаточно подозрительным мне показался только один фрагмент: ```cpp 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\)", расположенная чуть ниже в коде\. ## Подозрительный фрагмент N3 В следующем фрагменте кода, анализатору PVS\-Studio кажется всё\-таки удалось найти настоящую полновесную ошибку\. ```cpp 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"\. ## Подозрительный фрагмент N4 Есть несколько бессмысленных проверок следующего вида: ```cpp 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" всегда истинно и не имеет смысла\. Рядом есть код следующего вида: ```cpp if (rsa_c[i][0] == 0) { rsa_c[i][0]=1; rsa_c[i][1]=20; } ``` Возможно и с массивом 'dsa\_c' следует работать таким же образом\. Тогда код должен выглядеть так: ```cpp if (dsa_c[i][0] == 0) { dsa_c[i][0]=1; dsa_c[i][1]=1; } ``` Такая странная проверка есть ещё в нескольких местах: * V600 Consider inspecting the condition\. The 'ecdsa\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1506 * V600 Consider inspecting the condition\. The 'ecdsa\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1523 * V600 Consider inspecting the condition\. The 'ecdsa\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1540 * V600 Consider inspecting the condition\. The 'ecdh\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1560 * V600 Consider inspecting the condition\. The 'ecdh\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1577 * V600 Consider inspecting the condition\. The 'ecdh\_c\[i\]' pointer is always not equal to NULL\. speed\.c 1594 ## Неподозрительный момент Есть ряд мелких недочетов\. Это точно не ошибки\. Просто лишний код\. Вот пример избыточности в условии: ```cpp 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\-типа](https://pvs-studio.ru/ru/blog/terms/0030/) к 32\-битному типу, а затем обратно к memsize\-типу\. Вот одно их таких мест: ```cpp 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 написан некрасиво\. ```cpp void *CRYPTO_malloc(int num, const char *file, int line); #define OPENSSL_malloc(num) CRYPTO_malloc((int)num,__FILE__,__LINE__) ``` В результате, после препроцессирования получается следующий код: ```cpp heap = CRYPTO_malloc ((int)pow2 * sizeof heap[0], __FILE__,__LINE__); ``` В результате начинается бестолковые приведения типов\. В начале переменная 'pow2' типа [size\_t](https://pvs-studio.ru/ru/blog/terms/0044/) явно приводится к типу 'int'\. Затем при умножении на 'sizeof\(\)' тип выражения становится опять size\_t\. После этого при вызове функции CRYPTO\_malloc\(\) вновь происходит приведение к типу 'int'\. На этот раз приведение уже неявное\. В общем, от этого приведения типа, нет никакого толку\. Только потенциальное повод допустить ошибку\. Например, можно написать что\-то такое: ```cpp int *p1, *p2; int x, y; .... p = OPENSSL_malloc(p1 == p2 ? x : y); ``` На 64\-битной системе указатель 'p1' потеряет старшие биты, и результат сравнения может дать неверный результат\. Это конечно выдуманный пример, но всё равно не хорошо создавать такие макросы\. Как минимум, следует переписать макрос так: ```cpp #define OPENSSL_malloc(num) CRYPTO_malloc((int)(num), __FILE__,__LINE__) ``` А ещё лучше вообще не использовать здесь тип 'int'\. Размер выделяемой памяти должен передаваться в переменной memsize\-типа\. Например, это тип 'size\_t'\. ## Заключение Спасибо всем за внимание\. Буду рад, если эта заметка поможет как\-то улучшить библиотеку OpenSSL\. Как всегда прошу авторов не ограничиваться подозрительными местами, описанными здесь\. Лучше ещё раз проверить библиотеку и самостоятельно изучить отчёт\. Мы бесплатно предоставляем разработчикам open\-source библиотек ключ на некоторое время\. Возможно, удастся заметить опасные места, которым я не придал значения\.