Немного про OpenSSL

Андрей Карпов
Статей: 356



Небольшая заметка о результатах проверки проекта OpenSSL с помощью анализатора PVS-Studio. Я анализировал версию openssl-0.9.8-stable-SNAP-20121208.

О проверке OpenSSL

Недавно я написал заметку "Безопасность, безопасность! А вы её тестируете?" о проверке проекта TOR. За компанию я упомянул библиотеку OpenSSL, которая входит в проект TOR.

Статья активно обсуждалась на некоторых программистских ресурсах. Оказалось, что программистов весьма заботит качество библиотеки OpenSSL. Однако, я не смог ответить на некоторые вопросы касающиеся этой библиотеки. Также меня обвинили в том, что я не уведомил разработчиков библиотеки OpenSSL обо всех подозрительных местах.

Хочу пояснить эту ситуацию. Дело в том, что изучая TOR, я не планировал проверять библиотеку OpenSSL и изучать результаты её анализа. Эта библиотека просто случайно попалась под руку. В проект TOR была включена, какая то версия OpenSSL. Она и была обработана анализатором PVS-Studio за компанию.

Теперь я решил исправить ситуацию и специально скачал и проверил свежую версию библиотек OpenSSL.

Если честно, рассказывать мне особенно нечего. Мне не удалось обнаружить почти ничего подозрительного. Те ошибки, которые я описывал в статье, уже исправлены. OpenSSL – это качественный проект. Библиотека уже проверена множеством инструментов (Clang, Cppcheck, Coverity, DoubleCheck, Coccinelle, Klocwork и другими). В общем, библиотека хорошо вылизана. И найти в ней хотя бы одну ошибку, это будет уже подвиг.

Итак, расскажу, что подозрительного я нашел в коде библиотеки OpenSSL. Скорее всего, это несущественные недочеты, а не ошибки. Но надо хоть про что-то написать. :)

Подозрительный фрагмент N1

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

Есть несколько мест, где указатель в начале используется, а затем проверяется на нулевое значение. Но достаточно подозрительным мне показался только один фрагмент:

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 кажется всё-таки удалось найти настоящую полновесную ошибку.

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

Есть несколько бессмысленных проверок следующего вида:

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;
}

Такая странная проверка есть ещё в нескольких местах:

  • 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

Неподозрительный момент

Есть ряд мелких недочетов. Это точно не ошибки. Просто лишний код. Вот пример избыточности в условии:

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 библиотек ключ на некоторое время. Возможно, удастся заметить опасные места, которым я не придал значения.



Используйте PVS-Studio для поиска ошибок в C, C++ и C# коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;

Андрей Карпов
Статей: 356


А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++ и C#

goto PVS-Studio;