[devel] Buffer overflow - help needed

[devel] Buffer overflow - help needed

[Vitaly Ostanin]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Здравствуйте.

Есть такая проблема:
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


-- 
Regards, Vyt
mailto:  vyt@altlinux.org
JID:     vitaly.ostanin@gmail.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [devel] Buffer overflow - help needed

[Grigory Batalov]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Tue, 22 Apr 2008 16:19:12 +0400, Vitaly Ostanin wrote:

> Есть такая проблема:
> https://bugzilla.altlinux.org/show_bug.cgi?id=15284

  В бранче 4.0 пока ещё amanda-2.5.1-alt3.
  Ты с нею не проверял?

> Моего кунфу на неё пока не хватает. Если знатоки C посмотрят и
> скажут, как исправить, будет очень здорово. Посмотреть можно для
> скорости здесь:
>
> http://git.altlinux.org/people/bga/packages/?p=amanda.git;a=blob;f=amanda/common-src/match.c;h=dba15c802c28add576135af2f4a85476a2ccfd57;hb=c7308c105b84b606039fb617cc7fe2bd7799f1a9#l533

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [devel] Buffer overflow - help needed

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

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() и
зачем нужна эта функция.  Дальше есть условие, которое никогда не
может выполниться:

    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) приводит к переполнению
буфера.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [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

Re: [devel] Buffer overflow - help needed

[Slava Semushin]

21 июля 2008 г. 18:14 пользователь Slava Semushin <slava.semushin /
gmail.com> написал:
[...]
> P.S. А что с автором(ами) amand'ы? Они в курсе дырявости своего кода?

В курсе и даже приложили какое-то исправление в trunk (правда только к
ф-ции match_datestamp(), в аналогичной match_level() всё по-прежнему).

Предлагаю взять решение от производителя:
http://amanda.svn.sourceforge.net/viewvc/amanda/amanda/trunk/common-src/match.c?r1=347&r2=1155&view=patch


P.S. Возможно, патч придётся подправить, например, так вроде бы
поддержку gettext добавили и теперь сообщение об ошибке выглядит как
error(_("Illegal datestamp expression %s"),dateexp); Если в нашей
версии Аманды поддержки gettext ещё нет, то нужно будет заменить на
error("Illegal datestamp expression %s", dateexp);


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