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

    PVS-Studio and 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
      ....
    }

    Предупреждение 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 second argument. asn1_gen.c 371

    Беда вот здесь:
    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.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. A Boring Article About a Check of the OpenSSL Project.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Comments 93

      +9
      Ждал этой такой статьи именно от вас.
        0
        Вопрос немного не в тему — разве при эксплуатации этой уязвимости некоторые приложения иногда не будут падать с segfault?
        • UFO just landed and posted this here
            +3
            Вообще то да. Атакующий не может знать как у меня распределение память, и не выйдем ли мы за пределы сегмента при попытке скопировать память. Да что там — мы и сами то не знам выйдем мы за сегмент или нет, зависит от того, как в данном конкретном случае.эта структура будет лежать в памяти, точнее — где будет лежать.

            Собственно в си при случайном выходе за границу масссива (или просто обращении по не вал догму указателю) сегфолт бывает лишь иногда, а иногда просто чтение/запись не того что ожидалось. И это меняется от запуска к запуску приложения, и просто от раза к разу. Думаю каждый наблюдал подобное неоднократно в своих программах.
            • UFO just landed and posted this here
                0
                Поясни мысль плиз.
                • UFO just landed and posted this here
                    +2
                    SIGSEGV (если мы говорим о юниксах и им подобных типа линукса) возникает следующим образом — все зарождается в MMU, при попытке обратиться к адресу не отображенному в память, это сигнал (прервывание? никогда непосредственно с MMU не работал, надо будет попробовать) дергает за хвост ядро, которое уже обрабатывает эту ситуацию, если эта страничка в свопе, то она из свопа подгружается в ОЗУ, и прикладному процессу возвращается управление как будто ничего и не было, если же приложение не имеет прав на чтение/записть этой странички, или она вообще не была для него валидна (скажем через mmap не отображена), то приложению посылается сигнал SIGSEGV.

                    Соответственно SIGSEGV в случае heartbleed может возникнуть ровно по той же причине, по которой он может возникнуть и в случае обычного выхода за границу массива — наступили на страничку которая не отображена в память, либо к которой у нас нет прав на read.

                    Поэтому, иногда, в некоторых случаях, сегфолт тут таки возможен. Вероятность возникновения сегфолта при эксплуатации дыры сильно зависит от менеджера памяти данной конкретной софтины и того как активно им пользуются.
                    • UFO just landed and posted this here
                        0
                        Процесса? Сколько угодно. Ибо в этом же процессе может быть еще и вся бизнес-логика, работа с графикой, звуком и управлением 3Д печатью в одном флаконе.

                        А может занимать совсем мало — если это мой личный hello world работы с сокетами.
                        • UFO just landed and posted this here
                            +5
                            OpenSSL это либа, а не отдельное приложение. Даже если она создает отдельный поток для какой-то там своей активности (как скажем делает x264), то все равно это всё будет находиться в одном адресном пространстве с моей программой, которая печатает на 3Д принтере и запускает пони в космос. Более того — у них даже менеджер памяти скорее всего будет один и тот же.

                            Если бы весь OpenSSL был в отдельном процессе, то эта уязвимость стала бы сразу на порядок менее критической — ведь вся бизнеслогика, все логины, пароли, номера кредиток и так далее, были бы от нее изолированы, лежали бы в другом, отдельном адресном пространстве, до которого из OpenSSL нельзя было бы добраться даже теоретически, даже если поменять его код ради этого.

                            Уязвимы бы были только те данные которые прошли через OpenSSL в данную сессию в явном (не шифрованном) виде. (тут сессия — сессия работы приложения, то есть от старта процесса с OpenSSL)
                              +3
                              OpenSSL может использоваться с неблокирующими сокетами в асинхронном сервере, где лишние форки вообще запрещены, ибо число воркеров должно быть постоянным.
                        +6
                        Я уже ответил ниже.

                        SEGFAULT может возникнуть, когда буфер, из которого подпрограмма обработки TLS Heartbeat читает данные, волею звезд на небе расположился в конце динамической кучи, слишком близко к границе сегмента этой самой кучи.

                        Вероятность такого события зависит от используемых алгоритмов распределения памяти, однако ни в одном из случаев не становится нулевой.

                        Кстати, если натравить на сервер одновременно пару десятков тысяч атакующих, комбинируя при этом heartbleed с долгим чтением, то SEGFAULT становится неизбежным.
                        • UFO just landed and posted this here
                            0
                            и зря)
                              +2
                              Ну и не зря)
                                –2
                                Ну вооббще все равно зря плюсанули, SEGFAULT не будет.
                                –2
                                читаемый буфер НЕ вызодит за границы выделнной маллоком памяти, откуда сегфаул?
                                  +1
                                  Heartbleed — это атака выхода за границы буфера. Что еще не понятно?
                                    0
                                    Посмотрел код — угу, читаем за пределами буфера pl
                                  –1
                                  Не будет крэша. Потому что в один запрос — одна операция. Это возможно только для каких то редких приложений или самописных древностей, где бы СРАЗУ десятки тысячи запросов обрабатывались в пределах одного процесса (мультитред). А Apache будет форкать до предела, а потом ставить в очередь. При этом каждый форк будет обрабатывать один запрос.
                                    0
                                    Как раз наоборот, сейчас популярен асинхронный подход, как в nginx, где один и тот же процесс (и один и тот же поток) одновременно обрабатывает множество запросов.
                                      0
                                      Ну, я рассматривал случай с общим адресным пространством. Специфика ОС, в которой я работаю…

                                      Да, если каждый процесс выполняет не более 1 запроса — его так не положить.
                              +1
                              Но, как правило, память под кучу выделяется довольно крупными блоками, и очень редко отдается обратно системе. Чем дольше сервер работает — тем больше у него куча, и тем ниже вероятность случайного выхода за ее пределы.

                              Для того, чтобы сервер упал с сегфолтом, очередной буфер должен быть выделен близко к концу динамической кучи — но его малый размер приводит к тому, что он чаще оказывается в начале.

                              Ну, и наконец, далеко не каждый segfault приводит к смерти приложения. Приложение может быть достаточно кривым, чтобы игнорировать сигнал — или же мастер-процесс может просто перезапустить упавший воркер.
                                0
                                Либо куча должна выделяться не непрерывным куском адресного пространства. То есть некоторые диапазоны адресов «из середины» должны быть не отображены в память.

                                Грубо говоря, если мои секретные данные лежат только в диапазоне адресов за страницей не отображенной в память, то через эту уязвимость до них не добраться в принципе.

                                Плюс, насколько я понимаю, если не увлекаться malloc/free, то есть если расположение данных довольно статично и предсказуемов (в пулах), то сделать дамп всей памяти процесса через эту уязвимость не получится, просто потому, что скорее всего через уязвимость будут утекать одни и те же 64Кб данных.
                                  +1
                                  Совершенно верно. Кстати, хорошая идея для хранения приватных ключей в памяти — хранить их в отдельных блоках, окруженных блоками зарезервированными.

                                  Кстати, работает и в обратную сторону. Я так делал с буферами, отдаваемыми сторонним библиотекам — была одна глючная либа, которая так и норовила повредить управляемую кучу и уронить сервер. Но отдельная область памяти плюс перехват AccessViolationException сотворили чудо…

                                  Однако, такую сущность, как расшифрованный http-запрос, прятать заранее бесполезно — а значит, все на свете защитить особой работой с памятью не получится.
                            –1
                            Если вы про heartbleed, то segfault не возможен при атаках
                              +1
                              Почему?
                                0
                                Мне каэется потому что при чтении указателя по указтелю pl, payload байт, memcpy не выходит за размеры, которые были выделены под буфер pl. Так как под буфер откуда копируется (pl), выделено памяти больше, чем фактически записано, а именно 64 кб. Надо смотреть исходники для пруфа)
                                  +1
                                  Тогда это не было бы уязвимостью, скорее всего. По крайней мере не было бы ТАКОЙ уязвимостью. Ну и лечилось бы банальной заменой malloc на calloc.
                                    0
                                    Логично
                                      –1
                                      Но и такой вариант был бы уязвимостью с примерно таким же выходом.
                              0
                              >Код слишком запутанный.
                              >Да потому, что OpenSSL качественный проект.
                              Может быть, сам факт того, что код запутанный говорит что-то о качестве продукта?
                                +9
                                Был бы некачественным, не использовался бы во всём мире. А недостатки у всех есть.
                                  0
                                  Качественный проект с точки зрения статического анализатора?
                                  Потому что с точки зрения использования — это ужасный продукт. Да, я его использовал достаточно много, но лишь потому что это единственный в свое время инструмент который мог подписывать документы по ГОСТ Р 34.10-2012 в среде linux. Других плюсов у данного инструмента — нет.
                                    0
                                    А в чем конкретно неудобство использования?
                                    И речь идет о консольной тулзе или сишной либе?
                                      0
                                      Не знаю, но в тему, единственный минус, который я нашёл в OpenSSL это то, что его под Windows самостоятельно приходится собирать с обязательными правками, это претензия именно к качеству кода. Для любого опытного программиста, хоть раз встречавшегося с необходимостью интеграции сторонней библиотеки это всё привычно, но факт остаётся фактом.
                                0
                                Наверно не существует проекта где нету ляпов с проверкой указателя после его разыменования…
                                • UFO just landed and posted this here
                                    0
                                    то есть они не используют популярные структуры данных, типо бинарного дерева, или связного списка?
                                    • UFO just landed and posted this here
                                        0
                                        Я знаю о c++. Но я всегда верил в то, что рекурсивные структуры данных не могут использовать не указатели, потому что рекурсия сразу развернется и займет всю доступную память. Я не прав?
                                          0
                                          Вы правы для Си, но не для языков типа Лиспа, к примеру.
                                          –2
                                          template <typename T>
                                          class bin_tree
                                          {
                                          private:
                                          	struct node {
                                          		node left;
                                          		node right;
                                          		T val;
                                          	} root;
                                          };
                                          

                                          Вот такой код у меня даже не компилируется, если расставить указатели — все ок
                                          • UFO just landed and posted this here
                                              0
                                              Вы мне пример уже покажите, не терпится увидеть.
                                              В 1960 конечно не было, но на сколько я понимаю тогда можно было оперировать сырой памятью и самому написать указатели, не называя их таковыми.
                                              • UFO just landed and posted this here
                                                  0
                                                  не знаю как вы, я всегда считал идиотизмом так извращаться и делать структуру, у которой главное приемущество — бесконечность в пределах памяти и динамическое добавление/удаление. В таком виде прийдется постоянно копировать элементы
                                                  • UFO just landed and posted this here
                                                    +1
                                                    Кольцевой буфер без указателей в точности также подвержен тем же ошибкам что и с указателями — промахнуться мимо конца массива как нефиг делать :-)

                                                    Какая разница, пишешь ты foo[n] или же *(foo+n)?
                                                      +1
                                                      Но уж выход-то за границы массива отловить можно. Если вспомнить про такой язык, как Паскаль, то там проверки времени выполнения на выход за пределы диапазона включены по умолчанию…
                                                      • UFO just landed and posted this here
                                                          0
                                                          То есть Паскаль за хранение длины строки в самой строке — уважения не достоин, в отличие от Фортрана?..
                                                          • UFO just landed and posted this here
                                                            0
                                                            Ок. Отловите на этапе компиляции будет ли вот тут выход за границу массива:

                                                            int a = new int[rand()];
                                                            a[rand()] = 42;
                                                            
                                                              0
                                                              Без проблем. Ошибка компиляции: невозможно привести тип int* к типу int.

                                                              PS мы же вроде договаривались без указателей?
                                                                0
                                                                Да какая разница?

                                                                int a[1234];
                                                                a[rand()] = 42;
                                                                


                                                                Вместо rand может быть что угодно — например значение которое приехало по сети (собственно так heartbleed и работает), или вычисляемое по хитрому алгоритму значение. Или что-то там еще.

                                                                Единственное что тут можно делать — заставить явным образом перед использованием этого значения проверять его на вхождение в диапазон индексов массива. Но это, мягко говоря, не всегда эффективно будет, и в ряде случаев будет замусоривать код, делая его менее читабельным. Соответственно эту проверку будут отключать.
                                                              • UFO just landed and posted this here
                                                              –1
                                                              В паскале это отлавливается только в рантайме. В compile time, в общем случае, это не ловится даже теоретически.

                                                              Да, и отличий тут от указателей опять таки никаких — отловить обращение указателя не туда тоже можно в runtime. При желании.
                                                                0
                                                                И что? Чем вас не устраивает отлов таких ситуаций в рантайме?
                                                                  +1
                                                                  Обычно устраивает (кроме тех случаев когда это начинает существенно тормозить работу алгоритма, бывают такие ситуации).

                                                                  Но речь шла не об этом — речь шла о том, что если выпилить указатели из того же кольцевого буфера, то все сразу станет безопасней. Не станет. Валидность указателей в рантайме тоже можно проверять. Или точно также не проверять.

                                                                  Опять таки, если вернуться в Си, то там убрав указатели и заменив на индексы массива мы не добьемся вообще ничего — они там просто эквивалентны.
                                                                    0
                                                                    Да, сразу — не станет. Но если действовать последовательно, то вполне реально получить проект, который в принципе не может испытывать проблем с указателями (если вы не забыли, началось все это обсуждение с вопроса — существуют ли такие проекты).
                                                                      –1
                                                                      Ну да, проблем с указателями не будет. Будут другие проблемы :-) Например проблемы с индексами ;-)

                                                                      Понятно что Си — штука не безопасная в том смысле, что там вероятность того, что там самодиагностика приложения находится в жутко рудиментарном состоянии. Там где в java будет exception, там в Си часто будет просто порча памяти. Молча. (причем оно потом может быть даже и крашнется, но совершенно внезапно и в совершенно другом месте)

                                                                      Вопрос лишь в цене которую мы готовы заплатить за улучшенную самодиагностику приложения в рантайме, и большую предсказуемость поведения в случае ошибок :-)
                                                                        –2
                                                                        Не «сразу — не станет», а никогда не станет, по крайней мере, если не переписать OpenSSL на другом языке, на котором ссылки и указатели — не одно и то же, как в С.
                                                      0
                                                      Ну внутри-то STD все равно используются указатели. Без них трудно.
                                                      • UFO just landed and posted this here
                                                        0
                                                        Как насчёт Common Lisp или Scheme? Всё там есть — и деревья, и собаки. Однако указателей нет.
                                                          0
                                                          Я к сожалению мало знаком с функциональными языками. И я говорил на счет указателей относительно C/C++. В моем понимании указатель — это переменная, которая хранит адрес значения, а не само значение. Без такой возможности языка я не понимаю как можно создать рекурсивные структуры данных, которые хранят самих себя, с бесконечной (ограниченно памятью) вложенностью.
                                                          Ну и мы обсуждаем написание с нуля структуры данных, а не встроенное в язык. В C++ тоже есть std::list, который, кстати, использует указатели, но саму структуру можно использовать без указателей.
                                                            +1
                                                            зато есть итераторы, мимо которых тоже можно промахнуться, и получить undefined behaviour ;-)
                                                    +2
                                                    Расскажите, пожалуйста, а как анализатор узнал о BIO_write? Он сам увидел, что вместе со строкой функции всегда передается ее длина, или это увидели Вы и пометили ее для правила?
                                                      +3
                                                      В статье написано:
                                                      Отвлекусь немного. У читателя мог возникнуть вопрос, как PVS-Studio находит такие ошибки. Поясню. Он собирает информацию о вызовах функций, в данном случае о strncmp(), и строит матрицу данных:

                                                      vstart, «ASCII», 5
                                                      vstart, «UTF8», 4
                                                      vstart, «HEX», 3
                                                      vstart, «BITLIST», 3

                                                      У функции есть строковый аргумент и числовой. В основном длина строки совпадает с числом. Значит, этот аргумент задаёт длину строки. В одном месте это не так и значит надо выдать предупреждение V666.
                                                        +4
                                                        Я в конце рассказал, как работает диагностика V666. Ещё раз кратко. Анализатор PVS-Studio не требует никаких «пометок». Он собирает однотипные вызовы функций и строит матрицу данных. Затем, делает на её основе некоторые предположения. Если в функцию передаются строки и какие-то числовые аргументы, значения которых совпадает с длиной строки, то, наверное, это и есть длина строки. Если скажем, 20 раз совпало, а два раза нет, то видимо есть 2 ошибки.
                                                          +2
                                                          это, на самом деле, круто!
                                                            +7
                                                            О! Каждое правило это целая история и философские поиски. Я как ни будь начну рассказывать эти истории о разработке наиболее интересных диагностик. Просто всё никак. Вот Qt проверили, уже статья есть, но тут OpenSSL вклинился. Не публиковать же всё сразу. Статья про WinSCP лежит протухает… Прям очередь… А ведь ещё обязательно нужно статью про сравнение другими словами написать. А то прошлую заминусовали за то, что непочтительно отозвался о священной корове (Cppcheck). Никто так и не понял, какой объём работы был проделан и насколько адекватно мы старались сделать сравнение. Один только автор Cppcheck оценил проделанную работу :).
                                                            Но ничего, расскажу, расскажу про разработку привил.
                                                              +3
                                                              Читаю все ваши статьи что попадаются. Но только после этого вашего комментария понял на сколько неординарный продукт вы делаете: «Он собирает однотипные вызовы функций и строит матрицу данных. Затем, делает на её основе некоторые предположения. Если в функцию передаются строки и какие-то числовые аргументы, значения которых совпадает с длиной строки, то, наверное, это и есть длина строки. Если скажем, 20 раз совпало, а два раза нет, то видимо есть 2 ошибки.»
                                                                +1
                                                                Рад что попал на вашу статью. Теперь читаю все с интересом. У вас шикарный инструмент, надеюсь когда нибудь я смогу его себе приобрести!
                                                                Отдельно очень интересно было бы почитать рассказ о формировании правил проверки.
                                                                  +2
                                                                  Можно начать с CppCat. Это намного более доступный инструмент.
                                                          0
                                                          Кстати, Каверти исправился и теперь находит heartbleed
                                                            +17
                                                            А толку то. Это ведь для галочки сделано. Будет другая такая сложная ситуация и опять мимо. Я не планирую делать такую диагностику в PVS-Studio. Маркетологи конечно расскажут, как Coverity теперь вообще «все уязвимости ищет» и в том числе heartbleed. Толку-то. Parasoft вот тоже говорят «нашёл» эту ошибку. Где они раньше то были.
                                                            +4
                                                            Маркетологи заставили написать детектор именно для него. Будут теперь трубить на каждом шагу, что мол наш анализатор детектит heartbleed (неявно подразумевая, что и другие подобные баги тоже)! А на деле нет, но зато продается.
                                                            –2
                                                            > Код корректен, но не аккуратен. Лучше вначале проверить переменную 'o', а уже потом использовать 'i':
                                                            На самом деле, возможно, что и нет. Т.к. тогда время выполнения кода при инициализированной o и при неинициализированной будет сильно различаться, что может как-то использовать атакующий. en.wikipedia.org/wiki/Timing_attack
                                                              0
                                                              Мне кажется, Вы что-то перемудрили. По вашим словам получается, что сложить два числа (j+=i;) это медленно и можно заметить потраченное время. Не верю.
                                                                0
                                                                Каюсь, невнимательно посмотрел. Мне показалось, что вы рекомендовали проверять o в САМОМ начале, до DecryptUpdate и т.п. Вот это действительно может быть проблемой, а перестановка двух соседних команд, скорее всего, нет. Хотя разницу в десяток сложений уже легко поймать статистически и использовать.
                                                                +2
                                                                Чтобы делать такие заявления, надо иметь перед глазами листинг ассмеблера после всех оптимизаций. Компилятор не дурак и вполне может реорганизовать дерево условий так, чтобы вынести одинаковые подвыражения «за скобки». CSE называется. Как и в примере с ASN1_PRINTABLE_type.
                                                                  0
                                                                  Там парой строчек ниже и так стоит досрочный выход из функции! Время выполнения будет отличаться, независимо от местоположения j+=i
                                                                  +1
                                                                  А разве качество кода как-то связано с его malware-назначением?
                                                                  Ясно, что в openssl это был банальный недосмотр — но по-моему требовать, чтобы подобное было обнаружено статическим анализатором — это уж слишком.
                                                                  Если так рассуждать — то при анализе исходников явных вирусов он тоже должен сразу распознавать явно злобные намерения кода и ругаться, что есть мочи?
                                                                    0
                                                                    Переменная 'i' может оказаться неинициализированной, если (o == false). В результате, к 'j' будет прибавлено непонятно что. Это не страшно, так как, если (o == false), то срабатывает обработчик ошибки, и функция прекращает свою работу.

                                                                    IMO, это всё-таки достаточно серьёзно, поскольку использование неинициализированной переменной вызывает неопределённое поведение, и в результате может произойти что угодно.

                                                                    Например, компилятор может думать так:
                                                                    1. В строчке j += i; неопределённое поведение, если o == false перед вызовом EVP_DecryptUpdate.
                                                                    2. Правильная программа никогда не вызовет неопределённое поведение в этой строчке.
                                                                    3. То есть, o никогда не будет false перед вызовом EVP_DecryptUpdate.
                                                                    4. Поэтому выбрасываем из кода первый if (o).
                                                                      0
                                                                      Компиляторы так не думают. И не будут так думать.
                                                                      Просто потому, что это превратит огромное количество кода в русскую рулетку.
                                                                      Так что не думаю что ваши опасения оправданны.
                                                                        +1
                                                                        ну именно поэтому часть оптимизаций идут под грифом unsafe и не включаются автоматически. то есть компиляторы могут так думать.
                                                                        но по дефолту не будут, так как это слишком умно для людей.

                                                                    Only users with full accounts can post comments. Log in, please.