Project

General

Profile

Actions

Bug #2386

closed

check if default log dir is writable at start up

Added by Victor Julien almost 5 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

If the log dir doesn't exist, we refuse to start up:

suricata -c suricata.yaml -l noexist
[94239] 20/12/2017 -- 16:10:02 - (suricata.c:1957) <Error> (ParseCommandLine) -- [ERRCODE: SC_ERR_LOGDIR_CMDLINE(117)] - The logging directory "noexist" supplied at the commandline (-l noexist) doesn't exist. Shutting down the engine.

But if it's not writable, we error comes much later. E.g.:

suricata -c suricata.yaml -l /var/log/ -T --disable-detection
[94280] 20/12/2017 -- 16:10:34 - (suricata.c:1886) <Info> (ParseCommandLine) -- Running suricata under test mode
[94280] 20/12/2017 -- 16:10:34 - (suricata.c:1761) <Info> (ParseCommandLine) -- detection engine disabled
Error opening file /var/log/suricata/suricata.log
[94280] 20/12/2017 -- 16:10:34 - (suricata.c:1112) <Notice> (LogVersion) -- This is Suricata version 4.1.0-dev (rev 223d9a1)
[94280] 20/12/2017 -- 16:10:34 - (util-logopenfile.c:318) <Error> (SCLogOpenFileFp) -- [ERRCODE: SC_ERR_FOPEN(44)] - Error opening file: "/var/log//fast.log": Permission denied
[94280] 20/12/2017 -- 16:10:34 - (runmodes.c:776) <Error> (RunModeInitializeOutputs) -- [ERRCODE: SC_ERR_INVALID_ARGUMENT(13)] - output module setup failed

I think it would be good to extend the first check to include basic permission testing.


Related issues 2 (2 open0 closed)

Related to Bug #2337: give warning if permissions won't allow log reopen after dropping privsAssignedOISF DevActions
Related to Optimization #1595: Suricata starts in known conditions of no dataNewOISF DevActions
Actions #1

Updated by Victor Julien almost 5 years ago

  • Related to Bug #2337: give warning if permissions won't allow log reopen after dropping privs added
Actions #2

Updated by Richard Sailer almost 5 years ago

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

Question 1

Should the patch?
  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?
Actions #3

Updated by Jason Ish almost 5 years ago

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.

Actions #4

Updated by Richard Sailer almost 5 years ago

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.

Actions #5

Updated by Richard Sailer almost 5 years ago

  • % Done changed from 0 to 70
Actions #6

Updated by Andreas Herz almost 4 years ago

  • Assignee changed from Richard Sailer to OISF Dev
Actions #7

Updated by Victor Julien about 3 years ago

  • Status changed from New to Assigned
  • Assignee changed from OISF Dev to Shivani Bhardwaj
  • Target version changed from TBD to 5.0.0
Actions #8

Updated by Victor Julien about 3 years ago

Actions #9

Updated by Shivani Bhardwaj about 3 years ago

  • Priority changed from Normal to Urgent
Actions #10

Updated by Victor Julien about 3 years ago

  • Status changed from Assigned to Closed
  • Priority changed from Urgent to Normal
Actions

Also available in: Atom PDF