Проверки и безопасность

[abbra]

4 апреля центр CERT, занимающийся координацией обменом информацией о компьютерных уязвимостях между экспертами по безопасности опубликовал заметку о уязвимости в новых версиях (4.2+) GNU C Compiler, из-за которой определенные проверки на переполнение буферов памяти оптимизируются при компиляции приложения как ненужные. Связано это с тем, что в стандарте языка C определено, что при увеличении указателя на константу получающийся указатель должен указывать либо на первоначальный объект, либо на объект следом за первоначальным. То есть, фактически, предполагается, что операция сложения указателя и константы неубывающая. В таком случае код

uint_t *x, *foo = some_object();
uint_t i = 0, len = some_object_length();

for (x=foo; x < foo+len; x++) {
     i++;
}

становится просто ненужным, ведь указанное условие и так выполняется согласно требованию стандарта языка C, а значит, можно оптимизировать этот цикл и развернуть его в последовательность из len операций i++, которую тоже можно соптимизировать. Обратное неверно и, более того, в стандарте неопределено.

Реально такое изменение поведения компилятора правильно, оно приводит компилятор в соответствие со стандартом, и команда разработчиков gcc не собирается его "откатывать". Но в то же время оно фактически устраняет существующие во многих приложениях проверки на переполнение буферов. И эти проверки (неправильные с точки зрения стандарта C) необходимо переписывать.

Замечу, что изменение в поведении касается только проверок указателей. Это тот случай, когда тип сравниваемого имеет значение. Проверки на то, что сумма указателя и константы по-прежнему больше самого указателя, то есть, получившийся указатель не вышел за границу разрядности (и тем самым старший бит был потерян, а мы "завернулись" в начало адресного пространства), неверны сами по смыслу, ведь в таком случае все равно надо проверять не на "оборачиваемость" указателя, а на превышение размеров буфера. Фактически, изначальная проверка была неверной и работала только потому, что компилятор ее "не замечал".

В Samba подобная проблема существует, если ее собрать 32-битной версией gcc 4.2+ для тех фрагментов, где мы имеем входящий пакет, в котором указаны блоки смещением внутри пакета и размером блока от указанного смещения. Текущий код выглядит вот так:

char *buff_base;
uint32_t in_offset, in_size;

       if (buff_base + in_offset + in_size < buf_base)
               goto error;

Прямая замена на сравнение in_offset+in_size < alloc_size, где uint32_t alloc_size -- размер буфера, в который мы копируем данные из пакета, на самом деле не эквивалентна оригинальной проверке. Полная проверка будет выглядеть вот так:

   if (in_offset > alloced_size || in_size > alloced_size ||
                       in_offset + in_size > alloced_size ||
                       in_offset + in_size < in_offset )
               goto error;

То есть, количество проверок увеличилось, но это плата за понимание стандарта языка. Этот код будет правильно скомпилирован любой версией gcc.

Comments

По-моему, эта «полная проверка», как-раз, подвержена ...

[poige.livejournal.com]

... integer overflow: http://poige.livejournal.com/332851.html

Re: По-моему, эта «полная проверка», как-раз, подвержена .

[abbra]

Нет, я же говорил -- тип имеет значение. В твоем случае тип размера должен быть unsigned int (long), а не unsigned char. В крайнем случае, size_t, вообщем, максимальным на архитектуре . Тогда все проверки будут верными.

В случае 2^N мы имеем "обертку" в unsigned int, при которой sz_offset+sz_size превысят 2^N и всегда будут меньше sz_allocated. Предлагаю подумать почему. :-)

Тип, действительно, имеет значение, потому и выбрал byte.

[poige.livejournal.com]

Для демонстрации с обычной integer-размерностью, буфер был бы немного большим. )

Re: Тип, действительно, имеет значение, потому и выбрал by

[abbra]

Ага, я не дописал одну проверку:
in_offset+in_size < in_offset

Собственно, это можно видеть в коммите http://git.samba.org/?p=samba.git;a=commitdiff;h=09852899cadc48abe2f2651ecbceaf881198e648;hp=a4e3bc2bade8bf74696e1c6ced74da563ff2df7b
там соответствующая проверка выполняется до проверок на переполнение буфера.

> не дописал одну проверку

[poige.livejournal.com]

Вот-вот.

[buldozr.livejournal.com]

Я всегда полагал, что использовать операции порядкового сравнения над указателями - дурной тон.

Кстати, ваш код для полной проверки тоже не эквивалентен первоначальному: в первом не проверяется выход за alloced_size без 32-битного переполнения.

[abbra]

Полную картину можно увидеть вот в этом коммите:
http://gitweb.samba.org/?p=samba.git;a=commitdiff;h=09852899cadc48abe2f2651ecbceaf881198e648

Но вообще, порядковое сравнение указателей мне тоже всегда было как-то не по себе.

[ktirf-ru.livejournal.com]

У меня возник, наверное, профанский вопрос: а как этот код отловили? Методом пристального взгляда или инструменты какие-то помогли?

[abbra]

При написании GCC закручивают гайки с целью более точного соответствия стандартам. В данном случае как раз и закрутили так, что довольно большой класс ошибок, попадающих в недокументированное поведение по стандарту. Соответственно, этот код gcc 4.2 оптимизирует в пустышку и об этом сообщает в виде warning. А дальше довольно легко пишутся анализаторы.

[ktirf-ru.livejournal.com]

А, компилятор предупреждает. Очень мило с его стороны.