Suricata

Hi, I have two detail questions regarding the implementation of this:

Question 1¶

1. only check for the user we currently are or
2. also check for the user we will become when we drop privs.

Currently I assume 2. because it would make more sense IMHO. Also, in the case of 2. I think it will suffice to only check for the
user we will become, because we can change users only if we are root, and checking privs for root is unnecesarry per definition.
Any objections to these?

Question 2¶

• If I implement 2. (as described above) I need to move the call to ConfigCheckLogDirectory() to a point later in the initialization.
• Currently it gets called in ParseCommandLine(). At this point we can not know for sure if we will drop privs (--user could be a parameter behind -l, or priviledge dropping could be specified in the config file).
• Therefore I would recommend moving the call to ConfigCheckLogDirectory() before extending. I would recommend after config reading in PostConfLoadedSetup(), Line 2736 (Commit 282dad) directly after we evaluated whether we will drop privs in InitSignalHandler().
• Any objections? Thoughts?

Another observation I had. If the log directory does not exist, we exit. If its not writable, we continue and try to open up the interface. As both have the same affect of no logs being produced, the not writable case should probably also exit.

I think I would leave some form of ConfigCheckLogDirectory where it is. In the case where its set on the command line, the error message includes that context information. If set via the config, the error message provides that context. But perhaps ConfigCheckLogDirectory is better named something like ConfigCheckLogDirectoryExists(), and than a new function ...IsWritable() or something.

Its simple to test writability after switching users. But doing so, may result in nothing be logged to the console in daemon mode when it is not writable. As we daemonize, then drop privileges. So doing something like you suggest above at line 2736 is one option.

Jason Ish wrote:

Another observation I had. If the log directory does not exist, we exit. If its not writable, we continue and try to open up the interface. As both have the same affect of no logs being produced, the not writable case should probably also exit.

Actually I intendend to implement it this way anyway but forgot to mention it anywhere :p

I think I would leave some form of ConfigCheckLogDirectory where it is. In the case where its set on the command line, the error message includes that context information. If set via the config, the error message provides that context. But perhaps ConfigCheckLogDirectory is better named something like ConfigCheckLogDirectoryExists(), and than a new function ...IsWritable() or something.

Okay

Its simple to test writability after switching users. But doing so, may result in nothing be logged to the console in daemon mode when it is not writable. As we daemonize, then drop privileges. So doing something like you suggest above at line 2736 is one option.

Good. It's also not that complicated to test it before priv droping, it's a call to stat, and a few numeric comparisions of ids in the suri instance struct and the stat return struct.