[Oisf-devel] [MOVED to Oisf-devel] [Oisf-users] Config options discrepancies between suricata.yaml and ConfNodeLookupChildValue() in the source.
Victor Julien
victor at inliniac.net
Sat Feb 18 17:13:54 UTC 2012
On 02/18/2012 06:12 PM, Nikolay Denev wrote:
>
> On Feb 18, 2012, at 6:49 PM, Victor Julien wrote:
>
>> On 02/18/2012 05:44 PM, Nikolay Denev wrote:
>>>
>>> On Feb 18, 2012, at 2:48 PM, Nikolay Denev wrote:
>>>
>>>>
>>>> On Feb 18, 2012, at 2:37 PM, Victor Julien wrote:
>>>>
>>>>> On 02/17/2012 05:09 PM, Nikolay Denev wrote:
>>>>>>
>>>>>> On Feb 17, 2012, at 11:03 AM, Nikolay Denev wrote:
>>>>>>
>>>>>>>
>>>>>>> On Feb 17, 2012, at 7:10 AM, Nikolay Denev wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On Feb 17, 2012, at 12:15 AM, Victor Julien wrote:
>>>>>>>>
>>>>>>>>> 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
>>>>>>>>> ---------------------------------------------
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I thought that doing it in the parsers should be better and easier since this is the place when the supplied
>>>>>>>> strings are copied. But now that I look at it should be more readable and proper if it's in conf.c, probably in ConfSet().
>>>>>>>>
>>>>>>>> For the address and port groups I'm thinking of checking the parent config node name. They should be always with dash and no underscore even if they are underscored in the config file since I'm counting that they are set with
>>>>>>>> the modified ConfSet() that did the mangling.
>>>>>>>>
>>>>>>>> I'll see what I can do today.
>>>>>>>
>>>>>>> Hmm.. ConfSet() is used only for config keys/values set from the command line, and those from the config file are already populated
>>>>>>> in conf-yaml-loader.c . Changing them in conf.c seems too late?
>>>>>>
>>>>>> Here's a (I hope) working version. I was not able to think of a way to do this in conf.c, at least not easily, so it's still in the yaml parser.
>>>>>> Also since SCStrdup seems to be used also in critical sections I think that the compiler would optimize the macros and not penalize the
>>>>>> common case. If not then I will make another macro.
>>>>>>
>>>>>> diff -ur suricata.orig/work/suricata-1.2.1/src/conf-yaml-loader.c suricata/work/suricata-1.2.1/src/conf-yaml-loader.c
>>>>>> --- 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-17 17:56:57.000000000 +0200
>>>>>> @@ -104,7 +104,12 @@
>>>>>> if (state == CONF_KEY) {
>>>>>> if (parent->is_seq) {
>>>>>> if (parent->val == NULL) {
>>>>>> - parent->val = SCStrdup(value);
>>>>>> + /* convert underscores to dashes */
>>>>>> + if (strchr(value, '_')) {
>>>>>> + parent->val = SCStrdup_mangle(value);
>>>>>> + } else {
>>>>>> + parent->val = SCStrdup(value);
>>>>>> + }
>>>>>> }
>>>>>> }
>>>>>> ConfNode *n0 = ConfNodeLookupChild(parent, value);
>>>>>> @@ -113,7 +118,22 @@
>>>>>> }
>>>>>> else {
>>>>>> node = ConfNodeNew();
>>>>>> - node->name = SCStrdup(value);
>>>>>> + /* convert underscores to dashes */
>>>>>> + if (strchr(value, '_')) {
>>>>>> + /* do no covert underscores in var names */
>>>>>> + if (parent->name &&
>>>>>> + ((strcmp(parent->name, "address-groups") == 0) ||
>>>>>> + (strcmp(parent->name, "port-groups") == 0))) {
>>>>>> + node->name = SCStrdup(value);
>>>>>> + } else {
>>>>>> + node->name = SCStrdup_mangle(value);
>>>>>> + SCLogWarning(SC_WARN_DEPRECATED,
>>>>>> + "%s is deprecated. Please use %s",
>>>>>> + value, node->name);
>>>>>> + }
>>>>>> + } else {
>>>>>> + node->name = SCStrdup(value);
>>>>>> + }
>>>>>> TAILQ_INSERT_TAIL(&parent->head, node, next);
>>>>>> }
>>>>>> state = CONF_VAL;
>>>>>> diff -ur suricata.orig/work/suricata-1.2.1/src/util-error.c suricata/work/suricata-1.2.1/src/util-error.c
>>>>>> --- suricata.orig/work/suricata-1.2.1/src/util-error.c 2012-01-20 17:17:30.000000000 +0200
>>>>>> +++ suricata/work/suricata-1.2.1/src/util-error.c 2012-02-17 10:34:17.000000000 +0200
>>>>>> @@ -215,6 +215,7 @@
>>>>>> CASE_CODE (SC_ERR_SOCKET);
>>>>>> CASE_CODE (SC_ERR_PCAP_TRANSLATE);
>>>>>> CASE_CODE (SC_WARN_OUTDATED_LIBHTP);
>>>>>> + CASE_CODE (SC_WARN_DEPRECATED);
>>>>>>
>>>>>> default:
>>>>>> return "UNKNOWN_ERROR";
>>>>>> diff -ur suricata.orig/work/suricata-1.2.1/src/util-error.h suricata/work/suricata-1.2.1/src/util-error.h
>>>>>> --- suricata.orig/work/suricata-1.2.1/src/util-error.h 2012-01-20 17:17:30.000000000 +0200
>>>>>> +++ suricata/work/suricata-1.2.1/src/util-error.h 2012-02-17 10:35:17.000000000 +0200
>>>>>> @@ -230,6 +230,7 @@
>>>>>> SC_ERR_SOCKET,
>>>>>> SC_ERR_PCAP_TRANSLATE, /* failed to translate ip to dev */
>>>>>> SC_WARN_OUTDATED_LIBHTP,
>>>>>> + SC_WARN_DEPRECATED,
>>>>>> } SCError;
>>>>>>
>>>>>> const char *SCErrorToString(SCError);
>>>>>> 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-17 18:02:15.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; \
>>>>>> })
>>>>>>
>>>>>>
>>>>>
>>>>> I would prefer not to have a SCStrdup_mangle at all. I think a function
>>>>> local to conf-yaml-loader.c should do the mangling. Logic like:
>>>>>
>>>>> parent->val = SCStrdup(value);
>>>>> if (parent->val && strchr(parent->val, '_')) {
>>>>> Mangle(parent->val);
>>>>> SCLogWarning(SC_WARN_DEPRECATED,
>>>>> "%s is deprecated. Please use %s",
>>>>> value, parent->val);
>>>>> }
>>>>>
>>>>> Where Mangle(), probably called something like ConvertUnderscoreToDash()
>>>>> or something, would be local to conf-yaml-loader.c.
>>>>>
>>>>> --
>>>>> ---------------------------------------------
>>>>> Victor Julien
>>>>> http://www.inliniac.net/
>>>>> PGP: http://www.inliniac.net/victorjulien.asc
>>>>> ---------------------------------------------
>>>>>
>>>>
>>>> Yes, looks better, I'll rework it.
>>>>
>>>
>>> How about this :
>>>
>>> diff -ur suricata.orig/work/suricata-1.2.1/src/conf-yaml-loader.c suricata/work/suricata-1.2.1/src/conf-yaml-loader.c
>>> --- 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-18 18:42:51.000000000 +0200
>>> @@ -44,6 +44,24 @@
>>> };
>>>
>>> /**
>>> + * \brief Mangle unsupported characters.
>>> + *
>>> + * \param string A pointer to an null terminated string.
>>> + *
>>> + * \retval none
>>> + */
>>> +static void
>>> +Mangle(char *string)
>>> +{
>>> + char *c;
>>> +
>>> + while ((c = strchr(string, '_')))
>>> + *c = '-';
>>> +
>>> + return;
>>> +}
>>> +
>>> +/**
>>> * \brief Parse a YAML layer.
>>> *
>>> * \param parser A pointer to an active yaml_parser_t.
>>> @@ -105,6 +123,8 @@
>>> if (parent->is_seq) {
>>> if (parent->val == NULL) {
>>> parent->val = SCStrdup(value);
>>> + if (parent->val && strchr(parent->val, '_'))
>>> + Mangle(parent->val);
>>
>> Why is there no warning here?
>>
>
> As far as I can see it's for the case for parent of a sequence like this:
>
> threading.cpu_affinity.0 = management_cpu_set <----
> threading.cpu_affinity.0.management_cpu_set = (null)
> threading.cpu_affinity.0.management_cpu_set.cpu = (null)
> threading.cpu_affinity.0.management_cpu_set.cpu.0 = 0
>
> A warning there gives duplicate warnings.
I see. Cool.
Cheers,
Victor
--
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------
More information about the Oisf-devel
mailing list