Re: [devel] [git update] packages/katrin: heads/master

Re: [devel] [git update] packages/katrin: heads/master

[Slava Semushin]

2007/7/12, Denis Klimov <zver / altlinux.org>:

Это опять я :)))

[...]
> --- a/src/db/db-service/katrin-db-service.c
> +++ b/src/db/db-service/katrin-db-service.c
> @@ -2,28 +2,39 @@
>  #include "katrin-db-service.h"
>  #include "db/dl-db.h"
>  #include "utils/utils.h"
> +
> +#define PROG katrin-db-service

Я не нашел где бы это использовалось. Если оно не используется, то
предлагаю удалить. А если и использовать, то как строку, в таком
случае katrin-db-service нужно заключить в кавычки, я думаю.

[...]
> + * katrin-db-service <action> <params>
> + *
> + * katrin-db-service clean YYYY-MM-DD YYYY-MM-DD [--nullcost]
> + * katrin-db-service sum YYYY-MM-DD YYYY-MM-DD <user|src_port|dst_port|src_addr|dst_addr|proto>
> + *
> + */
>  int main (int argc, char **argv) {
>         #include "conf/conf-katrin.inc"
>         #include "db/dl-db.inc"
> -       int find_all = 0;
> -       int i=0;
> -       for (i = 1; i < argc; i++) {
> -               if (strcmp(argv[i], "--clean") == 0)
> -                       find_all = 1;
> -               else
> -                       break;
> +       int find = 0;
> +       int num_rows = 0;
> +       if (strcmp(argv[1], "clean") == 0) {
> +               debug("enter into clean section");
> +               if (argc - 1 < 2) {
> +                       err("Usage with clean action: katrin-db-service clean YYYY-MM-DD YYYY-MM-DD [--nullcost]");
> +                       return 1;
> +               }
> +               num_rows = cleanstats(argv[2],argv[3]);
> +               info("DB clean: %d rows deleted",num_rows);
>         }
> -
> -
> -       if (argc - i != 2) {
> -               err("Two arguments required: a startdate and enddate. Date format YYYY-MM-DD [HH-MM-SS]\n");
> -               return 1;
> +       if (strcmp(argv[1], "sum") == 0) {
> +               debug("enter into sum section");
> +               if (argc - 1 < 3) {
> +                       err("Usage with sum action: katrin-db-service YYYY-MM-DD YYYY-MM-DD <user|src_port|dst_port|src_addr|dst_addr|proto>");
> +                       return 1;
> +               }
> +               num_rows = sumstats(argv[2],argv[3],argv[4]);
> +               info("DB sum: %d rows sum");
>         }
>
> -       int num_rows = cleanstats(argv[i],argv[i+1]);
> -
> -       info("DB Clean: %d rows deleted",num_rows);
> -
>         return 0;
>  }
>
> diff --git a/src/db/db.h b/src/db/db.h
> index 6cc318b..92d6caf 100644
> --- a/src/db/db.h
> +++ b/src/db/db.h
> @@ -16,6 +16,7 @@ void setblockuser (int userid);
>  void storestat (struct userTraffInfo traffik,int accuracy_stat);
>  struct filterslist *tariffid2filters (int tariffid);
>  int cleanstats(char *startdate, char *enddate);
> +int sumstats(char *startdate, char *enddate, char *opts);
>  struct userslist *tariffid2users(int tariffid);
>  #endif
>
> diff --git a/src/db/dl-db.h b/src/db/dl-db.h
> index 21fc5a1..bb15118 100644
> --- a/src/db/dl-db.h
> +++ b/src/db/dl-db.h
> @@ -22,6 +22,7 @@ void (*setblockuser)(int userid);
>  void (*storestat) (struct userTraffInfo traffik,int accuracy_stat);
>  struct filterslist *(*tariffid2filters) (int tariffid);
>  int (*cleanstats)(char *startdate, char *enddate);
> +int (*sumstats)(char *startdate, char *enddate, char *opts);
>  struct userslist *(*tariffid2users)(int tariffid);
>  #endif
>
> diff --git a/src/db/dl-db.inc b/src/db/dl-db.inc
> index 08fa47a..f952dac 100644
> --- a/src/db/dl-db.inc
> +++ b/src/db/dl-db.inc
> @@ -16,4 +16,5 @@ setblockuser = dlsym(handleLibDB,"setblockuser");
>  storestat = dlsym(handleLibDB,"storestat");
>  tariffid2filters = dlsym(handleLibDB,"tariffid2filters");
>  cleanstats = dlsym(handleLibDB,"cleanstats");
> +sumstats = dlsym(handleLibDB,"sumstats");
>  tariffid2users = dlsym(handleLibDB,"tariffid2users");
> diff --git a/src/db/libkatrin-db-mysql.c b/src/db/libkatrin-db-mysql.c
> index 6dfa17d..0871c92 100644
> --- a/src/db/libkatrin-db-mysql.c
> +++ b/src/db/libkatrin-db-mysql.c
> @@ -321,6 +321,7 @@ dbDisconnectMySQL(&mysqllink);
>  return ret_p;
>  }
>
> +// DB service functions
>  int cleanstats(char *startdate, char *enddate) {
>         debug("cleanstats");
>         MYSQL mysqllink;
> @@ -341,6 +342,43 @@ int cleanstats(char *startdate, char *enddate) {
>         return num_rows;
>  }
>
> +int sumstats(char *startdate, char *enddate, char *opts) {
[...]
> +       if (mysql_real_query(&mysqllink,query,(unsigned int) strlen(query)) !=0 ) {
[...]
> +                       if (mysql_real_query(&mysqllink,query,(unsigned int) strlen(query)) !=0 )
[...]

Здесь и в других файлах я вижу, что вы всегда приводите третий
аргумент ф-ции mysql_real_query() к типу unsigned int.

Вопрос: зачем?

strlen() возвращает тип size_t, который, насколько мне известно, на 32
битах и есть не что иное как unsigned int. Даже если и приводить
результат strlen() то уж точно не к unsigned int, а к unsigned long,
который и ожидает ф-ция mysql_real_query(), согласно описанию на
http://dev.mysql.com/doc/refman/5.0/en/mysql-real-query.html.

[...]

Вот ваш код:

>  int main (int argc, char **argv) {
[...]
> +       if (strcmp(argv[1], "sum") == 0) {
> +               debug("enter into sum section");
> +               if (argc - 1 < 3) {
> +                       err("Usage with sum action: katrin-db-service YYYY-MM-DD YYYY-MM-DD <user|src_port|dst_port|src_addr|dst_addr|proto>");
> +                       return 1;
> +               }
> +               num_rows = sumstats(argv[2],argv[3],argv[4]);
> +               info("DB sum: %d rows sum");
>         }
[...]

А теперь посмотрим на тело ф-ции sumstat():

> +int sumstats(char *startdate, char *enddate, char *opts) {
[...]
> +       sprintf(query,"select userid,sum(bytes),sum(cost) from stats WHERE storetime >= '%s' AND storetime <= '%s' group by userid;",startdate,enddate);
> +       debug("query: %s",query);
> +       if (mysql_real_query(&mysqllink,query,(unsigned int) strlen(query)) !=0 ) {
> +               err("MySQL select query error: %s",mysql_error(&mysqllink));
> +       }
[...]
> +}

Вы что-нибудь видите подозрительное в коде? Я да.

Например, использование sprintf() вместо более безопасного snprintf().
Но это цветочки... В данном коде допускается *ОЧЕНЬ* серьёзная ошибка
в безопасности. Вы используете в SQL-запросе данные, полученные от
пользователя, при этом не проверяя их!!!

Немного по теме: ru.wikipedia.org/wiki/Инъекция_SQL

Совет в данном случае: добавить ф-цию, к примеру, validate_date(),
которая бы получала указатель на строку и проверяла дата это или нет.
И потом везде где ожидается дата проверять аргументы. В качестве
примера можете посмотреть как проверяется дата в ф-ции
is_string_confirm_to_format() в OpenFM:

http://svn.berlios.de/viewcvs/openfm/trunk/src/common.c?rev=75&view=markup


P.S. BTW, char *startdate, char *enddate, char *opts в ф-ции
sumstats() не изменяются, а это значит что их неплохо было бы объявить
как const.


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

Re: [devel] [git update] packages/katrin: heads/master

[Denis Klimov]

On Fri, 13 Jul 2007 13:12:14 +0700 Slava Semushin wrote:

> Вы используете в SQL-запросе данные, полученные от
> пользователя, при этом не проверяя их!!!
> 
> Немного по теме: ru.wikipedia.org/wiki/Инъекция_SQL
> 
> Совет в данном случае: добавить ф-цию, к примеру, validate_date(),
> которая бы получала указатель на строку и проверяла дата это или нет.
> И потом везде где ожидается дата проверять аргументы.

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

Если у кого то возникнет желаение закодить это раньше - смержу его коммиты.

Но все равно спасибо.
--
Denis Klimov 
zver

Re: [devel] [git update] packages/katrin: heads/master

[Slava Semushin]

13.07.07, Denis Klimov<kliden / km.ru> написал(а):
[...]
> Да, я знаю об этом, пока я хочу сначала реализовать основные функции
> биллинга, а уже потом, к релизу, добавить подобные проверки.

Мммм.. это порочная практика... признаться, я и сам так несколько раз
делал. Но! Я в таком случае расставлял комментарии типа:

// XXX: check this value proper

или

// TODO: add sctrict checks for all parameters

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

> Но все равно спасибо.

Предупреждён -- значит вооружен. ;)

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

[devel] UA: при разработке сетевого ПО _надо_ думать _сперва_ про безопасность!

[Michael Shigorin]

On Fri, Jul 13, 2007 at 02:06:28PM +0600, Denis Klimov wrote:
> > Вы используете в SQL-запросе данные, полученные от
> > пользователя, при этом не проверяя их!!!
> > Немного по теме: ru.wikipedia.org/wiki/Инъекция_SQL
> Да, я знаю об этом, пока я хочу сначала реализовать основные
> функции биллинга, а уже потом, к релизу, добавить подобные
> проверки.

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

Денис, не надо плодить код, который не продуман хотя бы по уже
известным Вам проблемам безопасности _заранее_.  Вы очень обяжете
этим как админов, которым с ним работать (if any), так и
репутацию команды.

> Если у кого то возникнет желаение закодить это раньше -
> смержу его коммиты.  Но все равно спасибо.

Поверьте на слово, с таким отношением проще попасть в чей-нить
чёрный список "эти пакеты не устанавливаются в принципе или без
ручной проверки/пересборки".  К сожалению, такие списки у ряда
участников команды и системных администраторов существуют и не
от хорошей жизни.

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

Вы ж не хотите прослыть таким "разработчиком", надеюсь?

PS: вообще блажь писать веб-интерфейсы на C должна быть очень
сильно обоснована как требованиями к характеристикам, так и 
уже наличествующей квалификацией разработчика.  Пишите лучше
на перле или хотя бы поищите библиотеки для cgi processing
и sql input sanitization, никакого смысла топтать все грабли
самостоятельно здесь нет.

-- 
 ---- WBR, Michael Shigorin <mike@altlinux.ru>
  ------ Linux.Kiev http://www.linux.kiev.ua/

Re: [devel] UA: при разработке сетевого ПО _надо_ думать _сперва_ про безопасность!

[Denis Klimov]

On Fri, 13 Jul 2007 11:59:55 +0300 Michael Shigorin wrote:

> Денис, не надо плодить код, который не продуман хотя бы по уже
> известным Вам проблемам безопасности _заранее_.  Вы очень обяжете
> этим как админов, которым с ним работать (if any), так и
> репутацию команды.

Пока этот биллинг я не отправляю в Сизиф, и не агетирую ставить его на
рабочий сервер, потому что он еще не готов, а еще в процессе
разработки, на git.alt выкладываю как раз для того чтобы мне что то
подсказали, или даже помогли.

> PS: вообще блажь писать веб-интерфейсы на C должна быть очень
> сильно обоснована как требованиями к характеристикам

Веб интерфейс у меня на PHP.
--
Denis Klimov 
zver

Re: [devel] UA: при разработке сетевого ПО _надо_ думать _сперва_ про безопасность!

[Anton Farygin]

Denis Klimov wrote:
> On Fri, 13 Jul 2007 11:59:55 +0300 Michael Shigorin wrote:
> 
>> Денис, не надо плодить код, который не продуман хотя бы по уже
>> известным Вам проблемам безопасности _заранее_.  Вы очень обяжете
>> этим как админов, которым с ним работать (if any), так и
>> репутацию команды.
> 
> Пока этот биллинг я не отправляю в Сизиф, и не агетирую ставить его на
> рабочий сервер, потому что он еще не готов, а еще в процессе
> разработки, на git.alt выкладываю как раз для того чтобы мне что то
> подсказали, или даже помогли.
> 
>> PS: вообще блажь писать веб-интерфейсы на C должна быть очень
>> сильно обоснована как требованиями к характеристикам
> 
> Веб интерфейс у меня на PHP.

Денис, немного не в тему, но напомните пожалуйcта, где посмотреть по 
биллингу документацию.

Меня интересует, в первую очередь, архитектура. В частности - каким 
образом планируется организовать работу с netflow, коммутаторами (SNMP) 
и pppoe/pptp.

Rgds,
Anton

Re: [devel] UA: при разработке сетевого ПО _надо_ думать _сперва_ про безопасность!

[Denis Klimov]

> Денис, немного не в тему, но напомните пожалуйcта, где посмотреть по 
> биллингу документацию.
> 
> Меня интересует, в первую очередь, архитектура. В частности - каким 
> образом планируется организовать работу с netflow, коммутаторами (SNMP) 
> и pppoe/pptp.
> 
> Rgds,
> Anton

Все что есть - тут. 
http://katrin.sf.net
Модуль получения информации о трафике есть пока netflow (работает)
Модуль авторизации клиента есть пока pptp (работал, но с недавнего времени перестал собираться в Сизифе и бранче)
В выходные как раз планировал занятся devel документацией
http://katrin.distance.ru/wiki/devel


--
Denis Klimov 
zver

Re: [devel] UA: при разработке сетевого ПО _надо_ думать _сперва_ про безопасность!

[Денис Смирнов]

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

On Fri, Jul 13, 2007 at 11:59:55AM +0300, Michael Shigorin wrote:

MS> Денис, не надо плодить код, который не продуман хотя бы по уже
MS> известным Вам проблемам безопасности _заранее_.  Вы очень обяжете
MS> этим как админов, которым с ним работать (if any), так и
MS> репутацию команды.

Добавлю к этому -- биллинг задача-то на самом деле тривиальнейшая.
Решается любым пьяным студентом на коленке чуть ли не за неделю. Только
вот не нужен такой биллинг совершенно никому.

А вот биллинг, который сможет корректно обрабатывать различные нештатные
ситуации, быть безопасным, и при всем этом ещё и функциональным (прошу
обратить внимание на порядок требований) нужен, и их нетути.

Плодить очередную поделку смысла нет, а вот вложитсья в написание
действительно надежного кода -- смысл есть. 

Даже маленький модуль для биллинга, но написаный _хорошо_ -- востребован.
Биллинг уровня кропного ISP который будет "иногда" падать или ломаться --
не нужен никому. 

-- 
С уважением, Денис

http://freesource.info
----------------------------------------------------------------------------
Тяжела и неказиста
Жизнь простого программиста

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

Re: [devel] [git update] packages/katrin: heads/master

[Eugene Prokopiev]

Denis Klimov пишет:
> On Fri, 13 Jul 2007 13:12:14 +0700 Slava Semushin wrote:
> 
>> Вы используете в SQL-запросе данные, полученные от
>> пользователя, при этом не проверяя их!!!
>>
>> Немного по теме: ru.wikipedia.org/wiki/Инъекция_SQL
>>
>> Совет в данном случае: добавить ф-цию, к примеру, validate_date(),
>> которая бы получала указатель на строку и проверяла дата это или нет.
>> И потом везде где ожидается дата проверять аргументы.
> 
> Да, я знаю об этом, пока я хочу сначала реализовать основные функции
> биллинга, а уже потом, к релизу, добавить подобные проверки.

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

-- 
С уважением,
Прокопьев Евгений