Bug 223052 - [PATCH] security/suricata: fix suricata stale pid file issue
Summary: [PATCH] security/suricata: fix suricata stale pid file issue
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Niclas Zeising
URL:
Keywords: patch
Depends on:
Blocks: 223322
  Show dependency treegraph
 
Reported: 2017-10-16 19:10 UTC by Reshad Patuck
Modified: 2017-11-27 20:36 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (franco)


Attachments
v1 - patch - suricata pre_cmd and new config variables (1.95 KB, patch)
2017-10-16 19:10 UTC, Reshad Patuck
no flags Details | Diff
stale pid fix (1.81 KB, patch)
2017-11-20 21:26 UTC, Franco Fichtner
franco: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Reshad Patuck 2017-10-16 19:10:20 UTC
Created attachment 187220 [details]
v1 - patch - suricata pre_cmd and new config variables

If the suricata pid file is not removed when suricata is stopped, the next time you attempt to run suricata, it complains that there is a stale pid file and refuses to start.

To test this:
- Run `service suricata start`
- kill -9 the suricata process
- Run `service suricata start` again
- suricata will fail with the following error

[ERRCODE: SC_ERR_INITIALIZATION(45)] - pid file '/var/run/suricata.pid' exists but appears stale. Make sure Suricata is not running and then remove /var/run/suricata.pid. Aborting!

This patch adds code to the rc script pre-command to check whether a stale pid file exists and clears it before suricata starts.

I have also added two new rc config variables:
- suricata_user - The user to run suricata as (defaulting to root)
- suricata_pidfile - The pid file to use for the suricata process.
Comment 1 Franco Fichtner 2017-10-17 20:34:33 UTC
Hi,

Do we really need multiple pid files? What user do you use to run suricata? And wouldn't it suffice to kill the pid file if it can't be opened?

if ! pgrep -qF $pidfile 2> /dev/null; then
    rm -f $pidfile
fi


Cheers,
Franco
Comment 2 Reshad Patuck 2017-10-18 04:37:31 UTC
(In reply to Franco Fichtner from comment #1)

Hey,

I run suricata as a user suricata which for me is in the bpf group.
The bpf grop has read access to /dev/bpf via a devfs config:
```
# Allow members of group bpf to read from /dev/bpf
own bpf root:bpf
perm bpf 0740
```
This allows me to run packet captures from a user account instead of as root, as long as the user is in the bpf group.

I don't need multiple pid files, I need to move the pid file to a location where the suricata user can write it.
For this I chown /var/run/suricata to user suricata and put the pid file in there (/var/run/suricata/suricata.pid)

As for killing the pidfile, if the box has rebooted because of a power failure there is a chance (remote) that something else may be using the pid which suricata was on previously.
In this case, the pid file will not be cleared and suricata will fail to start.

To make sure, I run the status command which not only checks that the pid in the pidfile is running but also that the process associated with it is suricata.
I then clear the pid file if suricata is not running and the pid file exists.

Hope this clears your queries.

Best,

Reshad
Comment 3 Franco Fichtner 2017-10-20 11:39:48 UTC
Hi Reshad,

Ok, thanks for explaining.  I just want to make sure that we don't assume only one Suricata is ever running and that is causing issues with the pid-file detection.

Is it ok for you if I prepare a separate patch with the update to 4.0.1 and incorporate your fixes?


Thanks,
Franco
Comment 4 Reshad Patuck 2017-10-21 18:50:39 UTC
Hi Franco,

Sure you can prepare a separate patch with the update to 4.0.1 that incorporates theses fixes.

Please let me know when we have a patch for 4.0.1, and I will mark this PR as closed.

Thanks,

Reshad
Comment 5 Franco Fichtner 2017-10-30 11:36:38 UTC
Hi Reshad,

I've opened this PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223322

Thank you for your contribution!  :)


Cheers,
Franco
Comment 6 Reshad Patuck 2017-10-31 04:29:17 UTC
Hi Franco,

Thanks for the update, I have added myself to your suricata 4.0.1 PR.

Will close this one when that is merged.

Best,

Reshad
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2017-10-31 12:28:45 UTC
Comment on attachment 187220 [details]
v1 - patch - suricata pre_cmd and new config variables

Obsoleted by bug 223322
Comment 8 Franco Fichtner 2017-11-20 21:26:29 UTC
Created attachment 188145 [details]
stale pid fix

Here is the requested patch for inclusion.
Comment 9 commit-hook freebsd_committer freebsd_triage 2017-11-21 18:56:04 UTC
A commit references this bug:

Author: zeising
Date: Tue Nov 21 18:55:28 UTC 2017
New revision: 454649
URL: https://svnweb.freebsd.org/changeset/ports/454649

Log:
  Fix suricata failing to start if there is a stale pid file laying around.
  This can happen if suricata is ungracefully shut down.

  PR:		223052, 223322
  Submitted by:	Reshad Patuck, Franco Fichtner
  Approved by:	Franco Fichtner (maintainer)
  MFH:		2017Q4

Changes:
  head/security/suricata/Makefile
  head/security/suricata/files/suricata.in
Comment 10 Niclas Zeising freebsd_committer freebsd_triage 2017-11-21 19:00:14 UTC
Committed, thanks!
Comment 11 Franco Fichtner 2017-11-22 04:46:33 UTC
Thank you!
Comment 12 commit-hook freebsd_committer freebsd_triage 2017-11-27 20:36:48 UTC
A commit references this bug:

Author: zeising
Date: Mon Nov 27 20:36:11 UTC 2017
New revision: 454991
URL: https://svnweb.freebsd.org/changeset/ports/454991

Log:
  MFH: r454649

  Fix suricata failing to start if there is a stale pid file laying around.
  This can happen if suricata is ungracefully shut down.

  PR:		223052, 223322
  Submitted by:	Reshad Patuck, Franco Fichtner
  Approved by:	Franco Fichtner (maintainer)

  Approved by:	ports-secteam (swills)

Changes:
_U  branches/2017Q4/
  branches/2017Q4/security/suricata/Makefile
  branches/2017Q4/security/suricata/files/suricata.in