[devel] Buffer overflow - help needed

[Slava Semushin]

2008/4/22 Sergey Vlasov <vsu / altlinux.ru>:

Заранее извиняюсь за поднятие столь давней темы, просто вопрос до сих
пор не решен и я хотел помочь в его разрешении..

> On Tue, Apr 22, 2008 at 04:19:12PM +0400, Vitaly Ostanin wrote:
>> Есть такая проблема:
>> https://bugzilla.altlinux.org/show_bug.cgi?id=15284
>>
>> Моего кунфу на неё пока не хватает. Если знатоки C посмотрят и
>> скажут, как исправить, будет очень здорово. Посмотреть можно для
>> скорости здесь:
>>
>> http://git.altlinux.org/people/bga/packages/?p=amanda.git;a=blob;f=amanda/common-src/match.c;h=dba15c802c28add576135af2f4a85476a2ccfd57;hb=c7308c105b84b606039fb617cc7fe2bd7799f1a9#l533
>
> Во-первых, тот, кто вызвал amfetchdump таким образом, не дочитал
> amanda(8) в части DATESTAMP EXPRESSION - дата должна записываться без
> '-', этот символ используется как разделитель при указании диапазона.
>
> Во-вторых, функция match_datestamp() написана отвратительно -
> например, код вида
>
>        strncpy(mydateexp, dateexp, strlen(dateexp));
>
> наводит на мысли, что писавший его не понимал, что делает strncpy() и
> зачем нужна эта функция.

Несмотря на то что код написан отвратительно, ничего лишнего strncpy()
записать вроде бы не сможет, так как выше есть проверка на размер
dateexp. Поэтому в данном примере strlen(dateexp) всегда на единицу
меньше буфера назначения.

> Дальше есть условие, которое никогда не
> может выполниться:
>
>    if(mydateexp[strlen(mydateexp)] == '$') {
>
> (очевидно, тут нужно strlen(mydateexp)-1 и перед ним проверка на
> пустую строку).

Да, согласен.

> Наконец, дальше находится вот такой кусок:
>
>    if((dash = strchr(mydateexp,'-'))) {
>        if(match_exact == 1) {
>            error("Illegal datestamp expression %s",dateexp);
>            /*NOTREACHED*/
>        }
>        len = (size_t)(dash - mydateexp);
>        len_suffix = strlen(dash) - 1;
>        len_prefix = len - len_suffix;
>
> При передаче недопустимого параметра вида "2008-04-09" получается
> len_suffix > len, однако это не проверяется, в результате последующий
> вызов strncpy(lastdate, mydateexp, len_prefix) приводит к переполнению
> буфера.

Предлагаю два варианта для решения сегфолта (вроде правильные):

-len_prefix = len - len_suffix;
+len_prefix = len;

или

-len_prefix = len - len_suffix;
+len_prefix = strlen(mydataexp) - len_suffix -1;

Немного сомневаюсь в их правильности, но вроде падать точно должно
перестать. При этом я не добавляю проверок на  len_suffix > len и
вывода ошибки. Просто в этом случае потом строчка передастя ниже на
сравнение strcmp() и там уже выяснится несовпадение. (Возможно, лучше
здесь проверять и сигнализировать об ошибке?)

> Те же самые ошибки повторяются дальше в функции match_level() (и
> вообще эти функции отличаются только текстом сообщения об ошибке).

И что делать? Всё патчить?


Собственно у меня просьба прокомментировать моё предложение по
исправлению. Также заинтересованным/ответственным попробовать
приложить исправление и проверить его корректность.


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

P.S. А что с автором(ами) amand'ы? Они в курсе дырявости своего кода?


-- 
+ Slava Semushin | slava.semushin @ gmail.com
+ ALT Linux Team | php-coder @ altlinux.ru