Андрей Карпов

16 Апр 2014

Теги:

#Cpp

Скучная статья про проверку OpenSSL

16 Апр 2014

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

Проверка OpenSSL
Результаты анализа
Заключение

Не так давно в OpenSSL была обнаружена уязвимость, о которой не говорит только ленивый. Я знаю, что PVS-Studio не способен найти ошибку, которая приводит к этой уязвимости. Поэтому я решил, что нет повода писать какую-либо статью про OpenSSL. В последние дни и так слишком много стало статей на эту тему. Однако, я получил шквал писем с просьбой рассказать, может ли PVS-Studio найти эту ошибку. Я сдался и написал эту статью.

Проверка OpenSSL

Я думаю, уже все знают, что в OpenSSL обнаружена серьёзная уязвимость. Если всё-таки кто-то пропустил это или хочет узнать больше подробностей, предлагаю познакомиться с несколькими статьями на эту тему:

Если быть кратким, то уязвимость, позволяющая осуществить доступ к чужим данным, просуществовала в коде около 2 лет. За это время её не обнаружил ни один анализатор кода, хотя эту библиотеку не тестировал только ленивый.

Тестировали OpenSSL и мы. Вот заметка на эту тему: "Немного про OpenSSL". Кое-что мы нашли, но кажется ничего серьёзного. Сейчас эти ошибки исправлены. Так что не зря проверили.

Я не уточнял, проверяли мы OpenSSL, когда там уже был Heartbleed баг или нет. В любом случае, я знаю, что PVS-Studio не умеет обнаруживать такой баг. Его вообще сложно обнаружить. Проект OpenSSL проверяли и проверяют различными инструментами, и ни один из них не нашёл эту ошибку. Например, не нашёл ошибку, лидер среди анализаторов кода Coverity Scan. Заметки про это: "Heartbleed and Static Analysis", "Heartbleed and static analysis (2)".

Дело в том, что такую ошибку очень сложно найти с помощью статического анализа. Код слишком запутанный. Нужно учитывать значения, хранящиеся в памяти, нужно понять, что скрывается за явными приведениями типов и так далее. Даже человеку сложно понять, в чём состоит проблема. Статические анализаторы тут пасуют. Это не недостаток методологии статического анализа. Просто ошибка действительно сложная. Наверное, нет инструмента, который может найти такой дефект, если предварительно его не натренировать на поиск подобных конструкций.

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

Моё личное мнение. Это просто ошибка, а никакая не закладка. Инструменты статического анализа не умеют её обнаруживать, так как она сложная. Вот и всё.

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

Почему находится так мало? Да потому, что OpenSSL качественный проект. То, что в нём найдена серьёзная уязвимость, вовсе не означат, что код ужасен. Думаю, во многих приложениях есть куда большие дыры, просто они никому не нужны. Плюс, проект OpenSSL проверяется многими инструментами.

Результаты анализа

Ещё раз хочу повторить, что никаких особенных ошибок я не нашёл. Будет лучше, если рассматривать текст ниже не как описание ошибок, а как комментарии к коду, который мне показался неаккуратным. Не хочу потом видеть комментарии, что я раздуваю из мухи слона.

Подозрительное сравнение

typedef struct ok_struct
{
  ....
  size_t buf_len_save;
  size_t buf_off_save;
  ....
} BIO_OK_CTX;

static int ok_read(BIO *b, char *out, int outl)
{ 
  .... 
  BIO_OK_CTX *ctx;
  ....
  /* copy start of the next block into proper place */
  if(ctx->buf_len_save - ctx->buf_off_save > 0)
  ....
}

Предупреждение PVS-Studio: V555 The expression of the 'A - B > 0' kind will work as 'A != B'. bio_ok.c 243

Выражение (ctx->buf_len_save - ctx->buf_off_save > 0) работает не так, как может показаться на первый взгляд.

Кажется, здесь хотят проверить условие (ctx->buf_len_save > ctx->buf_off_save). Это не так. Дело в том, что сравниваемые переменные имеют беззнаковый тип. Вычитание одной беззнаковой переменной из другой беззнаковой переменной, даёт результат беззнакового типа.

Условие (ctx->buf_len_save - ctx->buf_off_save > 0) выполнится всегда, если переменные не совпадают. Другими словами, следующие два выражения эквивалентны:

(ctx->buf_len_save - ctx->buf_off_save > 0)
(ctx->buf_len_save != ctx->buf_off_save)

Пояснение для тех, кто не очень хорошо знаком с языком Си. Опытные разработчики могут не читать.

Пусть у нас есть две 32-битные переменные типа unsigned:

unsigned A = 10;

unsigned B = 20;

Проверим, выполнится ли условие (A - B > 0).

Результат вычитания (A - B) равен 10u - 20u = 0xFFFFFFF6u = 4294967286u.

Дальше сравниваем беззнаковое число 4294967286u с нулём. Ноль тоже приводится к беззнаковому типу, но это не имеет значения.

Выражение (4294967286u > 0u) является истинным.

Выражение (A - B > 0) будет ложным только в одном случае, когда A == B.

Является ли это сравнение ошибкой? Я не знаю, так как не знаком с устройством проекта. Думаю, что ошибки нет.

Скорее всего, дело обстоит так. Переменная 'buf_len_save' обычно больше переменной ' 'buf_off_save'. Только иногда значение переменной 'buf_off_save' достигает значения, хранящегося в 'buf_len_save'. На этот случай, когда переменные совпадают и нужна эта проверка. Случай, когда (buf_len_save < buf_off_save), наверное невозможен.

Неинициализированная переменная, которая не приводит к беде

Есть место, где может использоваться неинициализированная переменная. Но ни к каким плохим последствиям это не приведёт. Вот этот код:

int PEM_do_header(....)
{
  int i,j,o,klen;
  ....
  if (o)
    o = EVP_DecryptUpdate(&ctx,data,&i,data,j);
  if (o)
    o = EVP_DecryptFinal_ex(&ctx,&(data[i]),&j);
  ....
  j+=i;
  if (!o)
  {
    PEMerr(PEM_F_PEM_DO_HEADER,PEM_R_BAD_DECRYPT);
    return(0);
  }
  ....  
}

Предупреждение PVS-Studio: V614 Potentially uninitialized variable 'i' used. pem_lib.c 480

Переменная 'i' может оказаться неинициализированной, если (o == false). В результате, к 'j' будет прибавлено непонятно что. Это не страшно, так как, если (o == false), то срабатывает обработчик ошибки, и функция прекращает свою работу.

Код корректен, но не аккуратен. Лучше вначале проверить переменную 'o', а уже потом использовать 'i':

if (!o)
{
  PEMerr(PEM_F_PEM_DO_HEADER,PEM_R_BAD_DECRYPT);
  return(0);
}
j+=i;

Странные присваивания

#define SSL_TLSEXT_ERR_ALERT_FATAL 2
int ssl3_accept(SSL *s)
{
  ....
  if (ret != SSL_ERROR_NONE)
  {
    ssl3_send_alert(s,SSL3_AL_FATAL,al);  
    if (al != TLS1_AD_UNKNOWN_PSK_IDENTITY)   
      SSLerr(SSL_F_SSL3_ACCEPT,SSL_R_CLIENTHELLO_TLSEXT);      
    ret = SSL_TLSEXT_ERR_ALERT_FATAL;      
    ret= -1;
    goto end;  
  }
  ....
}

Предупреждение PVS-Studio: V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 376, 377. s3_srvr.c 377

В начале переменной 'ret' присваивается значение 2, а затем -1. Наверное, первое присваивание лишнее и осталось в коде случайно.

Ещё один случай:

int
dtls1_retransmit_message(....)
{
  ....
  /* save current state */
  saved_state.enc_write_ctx = s->enc_write_ctx;
  saved_state.write_hash = s->write_hash;
  saved_state.compress = s->compress;
  saved_state.session = s->session;
  saved_state.epoch = s->d1->w_epoch;
  saved_state.epoch = s->d1->w_epoch;
  ....
}

Предупреждение PVS-Studio: V519 The 'saved_state.epoch' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1277, 1278. d1_both.c 1278

Потенциальное разыменование нулевого указателя

Самый распространенный ляп в программах (по моему опыту) - это разыменование указателя до того, как он проверен. Не всегда это ошибка. Часто указатель никогда не будет нулевым. Тем не менее, это потенциально опасный код. Особенно когда проект быстро меняется.

Есть такие ляпы и в OpenSSL:

int SSL_shutdown(SSL *s)
{
  if (s->handshake_func == 0)
  {
    SSLerr(SSL_F_SSL_SHUTDOWN, SSL_R_UNINITIALIZED);
    return -1;
  }

  if ((s != NULL) && !SSL_in_init(s))
    return(s->method->ssl_shutdown(s));
  else
    return(1);
  }
  ....
}

Предупреждение PVS-Studio: V595 The 's' pointer was utilized before it was verified against nullptr. Check lines: 1013, 1019. ssl_lib.c 1013

В начале указатель 's' используется: (s->handshake_func == 0).

И только потом проверяется: (s != NULL).

Другой, более сложный случай:

#define bn_wexpand(a,words) \
  (((words) <= (a)->dmax)?(a):bn_expand2((a),(words)))

static int ubsec_dh_generate_key(DH *dh)
{
  ....
  if(bn_wexpand(pub_key, dh->p->top) == NULL) goto err;
  if(pub_key == NULL) goto err;
  ....
}

Предупреждение PVS-Studio: V595 The 'pub_key' pointer was utilized before it was verified against nullptr. Check lines: 951, 952. e_ubsec.c 951

Чтобы понять, где ошибка, нужно раскрыть макросы. Тогда получится вот такой код:

if((((dh->p->top) <= (pub_key)->dmax)?
    (pub_key):bn_expand2((pub_key),
    (dh->p->top))) == ((void *)0)) goto err;
if(pub_key == ((void *)0)) goto err;

Обратите внимание на указатель 'pub_key'.

Вначале он разыменовывается: (pub_key)->dmax.

Ниже он проверяется на равенство нулю: (pub_key == ((void *)0)).

Лишние проверки

Есть несколько фрагментов кода, где переменная сравнивается дважды с одним и тем же значением. Я думаю, это не ошибки. Кажется, просто вторая проверка написана случайно и просто лишняя. Её можно удалить.

Лишняя проверка N1

int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
  ....
  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

Я выделил идентичные проверки с помощью "<<<<====". Про эту дублирующую проверку я писал и в предыдущей статье, но она не исправлена. Так что это точно не проблема.

Лишняя проверка N2, N3

int ssl3_read_bytes(SSL *s, int type,
  unsigned char *buf, int len, int peek)
{
  ....
  if ((type && (type != SSL3_RT_APPLICATION_DATA) &&
       (type != SSL3_RT_HANDSHAKE) && type) ||
      (peek && (type != SSL3_RT_APPLICATION_DATA)))
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'type' to the left and to the right of the '&&' operator. s3_pkt.c 952

Два раза проверяется, что переменная 'type' имеет ненулевое значение.

Рассмотренный фрагмент кода скопирован в другой файл, поэтому там тоже есть лишнее сравнение: d1_pkt.c 760.

Неправильная длина строк

Нехорошо задавать длину строк с помощью магических констант. Очень легко ошибиться. В OpenSSL анализатор PVS-Studio заметил три таких места.

Первое неудачное магическое число

Чтобы продемонстрировать, что это ошибка, давайте рассмотрим несколько примеров вызова функции BIO_write:

BIO_write(bp,"Error in encoding\n",18)
BIO_write(bp,"\n",1)
BIO_write(bp,":",1)
BIO_write(bp,":BAD OBJECT",11)
BIO_write(bp,"Bad boolean\n",12)

Как видно из этих примеров, последнее число задаёт длину строки.

А теперь, некорректный код:

static int asn1_parse2(....)
{
  ....
  if (BIO_write(bp,"BAD ENUMERATED",11) <= 0)
    goto end;
  ....
}

Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'BIO_write'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. asn1_par.c 378

Длина строки "BAD ENUMERATED" не 11, а 14 символов.

Второе неудачное магическое число

static int www_body(char *hostname, int s, unsigned char *context)
{
  ....
  if ( ((www == 1) && (strncmp("GET ",buf,4) == 0)) ||
       ((www == 2) && (strncmp("GET /stats ",buf,10) == 0)))
  ....
}

Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the first argument. s_server.c 2703

Длина строки "GET /stats " не 10, а 11 символов. Не учтён последний пробел. Мелкий недочёт, но всё равно недочёт.

Третье неудачное магическое число

static int asn1_cb(const char *elem, int len, void *bitstr)
{
  ....
  if (!strncmp(vstart, "ASCII", 5))
    arg->format = ASN1_GEN_FORMAT_ASCII;
  else if (!strncmp(vstart, "UTF8", 4))
    arg->format = ASN1_GEN_FORMAT_UTF8;
  else if (!strncmp(vstart, "HEX", 3))
    arg->format = ASN1_GEN_FORMAT_HEX;
  else if (!strncmp(vstart, "BITLIST", 3))
    arg->format = ASN1_GEN_FORMAT_BITLIST;
  else
  ....
}

Беда вот здесь:

if (!strncmp(vstart, "BITLIST", 3))

Длина строки "BITLIST" равна 7 символам.

Отвлекусь немного. У читателя мог возникнуть вопрос, как PVS-Studio находит такие ошибки. Поясню. Он собирает информацию о вызовах функций, в данном случае о strncmp(), и строит матрицу данных:

vstart, "ASCII", 5
vstart, "UTF8", 4
vstart, "HEX", 3
vstart, "BITLIST", 3

У функции есть строковый аргумент и числовой. В основном длина строки совпадает с числом. Значит, этот аргумент задаёт длину строки. В одном месте это не так и значит надо выдать предупреждение V666.

Не очень хорошо

Не очень хорошо распечатывать значение указателя, используя "%08lX". Для этого есть "%p".

typedef struct mem_st
{
  void *addr;
  ....
} MEM;

static void print_leak_doall_arg(const MEM *m, MEM_LEAK *l)
{
  ....
  BIO_snprintf(bufp, BUF_REMAIN, "number=%d, address=%08lX\n",
               m->num,(unsigned long)m->addr);
  ....
}

В функцию передаётся не указатель, а значение типа (unsigned long). Поэтому промолчит компилятор и некоторые анализаторы.

PVS-Studio заметил этот недочёт косвенным образом. Ему не нравится, что указатель явно приводят к типу (unsigned long). Это неправильно. Никто не гарантировал, что указатель поместится в тип 'long'. Например, так нельзя делать в Win64.

Правильный и более короткий код:

BIO_snprintf(bufp, BUF_REMAIN, "number=%d, address=%p\n",
  m->num, m->addr);

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

mem_dbg.c 699
bio_cb.c 78
asn1_lib.c 467

Заключение

Хотя статические анализаторы не нашли эту ошибку, и она просуществовала очень долго, я всё равно призываю всех использовать статический анализ в повседневной работе. Просто не надо искать серебряную пулю, которая решит все проблемы и полностью избавит программный код от ошибок. Наилучший результат достигается в комплексном подходе: юнит-тесты, статический и динамический анализ, регрессионные тесты и так далее. Статический анализ позволит устранить массу опечаток и глупых ошибок на этапе кодирования и этим сэкономит время на другие полезные дела, такие как новая функциональность или написание более тщательных тестов.

Попробуйте наш анализатор кода PVS-Studio.

#Cpp