[Oisf-devel] [MOVED to Oisf-devel] Re: [Oisf-users] Config options discrepancies between suricata.yaml and ConfNodeLookupChildValue() in the source.

[Victor Julien]

On 02/16/2012 08:29 PM, Nikolay Denev wrote:
> 
> On Feb 16, 2012, at 6:46 PM, Victor Julien wrote:
> 
>> On 02/16/2012 05:44 PM, Nikolay Denev wrote:
>>>
>>> On Feb 16, 2012, at 6:30 PM, Victor Julien wrote:
>>>
>>>> On 02/16/2012 03:16 PM, Nikolay Denev wrote:
>>>>> Hi,
>>>>>
>>>>> I've noticed that there are some discrepancies regarding the config options that are in the default suricata.yaml file, that
>>>>> I guess most people use as a starting point to modify for their needs. For example I've tried to set the pcap logging to use only 10 files and rotate them,
>>>>> and I noticed that this didn't work. I've found that the source uses "ConfNodeLookupChildValue(conf, "max-files");" to get the number of files, but suricata.yaml
>>>>> has "max_files", so this option is not parsed.
>>>>> I see other similar mixups of dashes and underscores like : 
>>>>>
>>>>> /usr/local/etc/suricata/suricata.yaml:      use_stream_depth: no #If set to "yes" packets seen after reaching stream inspection depth are ignored. "no" logs all packets
>>>>> ./log-pcap.c:        use_stream_depth = ConfNodeLookupChildValue(conf, "use-stream-depth");
>>>>>
>>>>> ./log-pcap.c:            s_dir = ConfNodeLookupChildValue(conf, "sguil-base-dir");
>>>>> ./log-pcap.c:                s_dir = ConfNodeLookupChildValue(conf, "sguil_base_dir");
>>>>>
>>>>>
>>>>> ts-format and ts_format…
>>>>>
>>>>> I can try to prepare a patch for these when I have free time, if it's clear what should be the convention: underscore or dash.
>>>>
>>>> Yeah it's inconsistent. The goal is to have the dash approach
>>>> everywhere. Complicating things is that for every setting we convert, I
>>>> want the old way to continue to work. So backwards compatibility, like
>>>> with the sguil-base-dir.
>>>>
>>>> Still interested in helping out? :)
>>>>
>>>> -- 
>>>> ---------------------------------------------
>>>> Victor Julien
>>>> http://www.inliniac.net/
>>>> PGP: http://www.inliniac.net/victorjulien.asc
>>>> ---------------------------------------------
>>>
>>> Hmm :)
>>> Does converting all underscores to dashes in the ConfNodeLookup* functions (emitting warning about the uncerscore versions being deprecated) and using only dashes internally makes sense?
>>> This way even "sguil_base-dir" would work as a side effect, but I doubt it would break anything.
>>>
>>
>> Thats actually a pretty good idea, as long as we only do it for the
>> option name, not the value.
>>
>> -- 
>> ---------------------------------------------
>> Victor Julien
>> http://www.inliniac.net/
>> PGP: http://www.inliniac.net/victorjulien.asc
>> ---------------------------------------------
>>
> 
> I've hacked this ugly piece :)
> 
> --- suricata.orig/work/suricata-1.2.1/src/conf-yaml-loader.c	2012-01-20 17:17:30.000000000 +0200
> +++ suricata/work/suricata-1.2.1/src/conf-yaml-loader.c	2012-02-16 21:21:21.000000000 +0200
> @@ -104,7 +104,7 @@
>                  if (state == CONF_KEY) {
>                      if (parent->is_seq) {
>                          if (parent->val == NULL) {
> -                            parent->val = SCStrdup(value);
> +                            parent->val = SCStrdup_mangle(value);
>                          }
>                      }
>                      ConfNode *n0 = ConfNodeLookupChild(parent, value);
> @@ -113,7 +113,7 @@
>                      }
>                      else {
>                          node = ConfNodeNew();
> -                        node->name = SCStrdup(value);
> +                        node->name = SCStrdup_mangle(value);
>                          TAILQ_INSERT_TAIL(&parent->head, node, next);
>                      }
>                      state = CONF_VAL;
> diff -ur suricata.orig/work/suricata-1.2.1/src/util-mem.h suricata/work/suricata-1.2.1/src/util-mem.h
> --- suricata.orig/work/suricata-1.2.1/src/util-mem.h	2012-01-20 17:17:30.000000000 +0200
> +++ suricata/work/suricata-1.2.1/src/util-mem.h	2012-02-16 21:03:23.000000000 +0200
> @@ -118,7 +118,11 @@
>      (void*)ptrmem; \
>  })
>  
> -#define SCStrdup(a) ({ \
> +#define SCStrdup(a) _SCStrdup(a,0)
> +
> +#define SCStrdup_mangle(a) _SCStrdup(a,1)
> +
> +#define _SCStrdup(a,mangle) ({ \
>      char *ptrmem = NULL; \
>      extern size_t global_mem; \
>      extern uint8_t print_mem_flag; \
> @@ -134,6 +138,12 @@
>          } \
>      } \
>      \
> +    if ((mangle != 0)) { \
> +        char *u; \
> +        while (u = strchr(ptrmem, '_')) { \
> +            *u = '-'; \
> +        } \
> +    } \
>      global_mem += len; \
>      if (print_mem_flag == 1) \
>          SCLogInfo("SCStrdup return at %p of size %"PRIuMAX, \
> @@ -196,7 +206,11 @@
>      (void*)ptrmem; \
>  })
>  
> -#define SCStrdup(a) ({ \
> +#define SCStrdup(a) _SCStrdup(a,0)
> +
> +#define SCStrdup_mangle(a) _SCStrdup(a,1)
> +
> +#define _SCStrdup(a,mangle) ({ \
>      char *ptrmem = NULL; \
>      \
>      ptrmem = strdup((a)); \
> @@ -209,6 +223,12 @@
>              exit(EXIT_FAILURE); \
>          } \
>      } \
> +    if ((mangle != 0)) { \
> +        char *u; \
> +        while (u = strchr(ptrmem, '_')) { \
> +            *u = '-'; \
> +        } \
> +    } \
>      (void*)ptrmem; \
>  })
>  
> 
> 
> Seems to work, except that I have to specially handle the address-group and port-group variables, as this currently
> makes HOME_NET -> HOME-NET and the rules fail.
> 
> 

I don't think this is solved in the proper place. However the mangling
is done, the code should be contained in conf.c :)

The port and address vars should be easy to catch I think. They are
always requested with a address-groups/port-groups prefix I think.

-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------