changed when/where the pidfile of daemons gets created #928

alaaeddineelamri · 2021-09-17T14:36:13Z

Description:

Fix for CVE-2017-14610 vulnerability, where PID files could be exploited on certain systems under certain circumstances.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

PR name is meaningful
Purpose of the PR is understood
Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
Commit descriptions are understandable and well formatted

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR
bareos-check-sources --since-merge does not report any problems
git status should not report modifications in the source tree after building and testing

pstorz

Good work. We should check if we want to switch the man page creation for the daemons also to be created by sphinx.

core/src/dird/dird.cc

core/src/lib/daemon.h

core/src/stored/stored.cc

CHANGELOG.md

core/src/dird/dird.cc

arogge

This looks good and very much like what we had discussed.
I have two main areas of concern left:

when running with -f we still accept -p but ignore it, we should either write the pidfile correctly or deny a combination of -f and -p.
on Windows we accept -p and then simply ignore it. Maybe we shouldn't accept it in the first place.

Other than that, I would delegate writing the file to daemon_start() and DeletePidFile() can be made even simpler.

core/src/lib/bsys.cc

core/src/lib/region_locking.cc

core/src/lib/region_locking.h

core/src/stored/stored.cc

CHANGELOG.md

arogge

I have two minor remarks, mostly concerning messages and documentation.

Before merging the PR, we should also create a matching security advisory in GitHub that we can link to from the CHANGELOG.md. (I'll take care of that)

core/src/filed/filed.cc

core/src/lib/bsys.cc

CHANGELOG.md

arogge

This looks great.
I think we should change the docs, so the user knows that the directive Pid Directory won't have an effect anymore.
Other than that, I hope I can finish the Security Advise tomorrow, so we can finally merge this.

arogge

I think we can go one step further with the documentation. I think it is enough if the modern (i.e. 21+) documentation states that this has no effect anymore and tells you how to do it with the parameter.

arogge · 2021-11-10T14:37:49Z

...anuals/source/manually_added_config_directive_descriptions/dir-director-PidDirectory.rst.inc

+This directive is optional and specifies a directory in which the Director may put its process Id file. The process Id file is used to shut down Bareos and to prevent multiple copies of Bareos from running simultaneously. Standard shell expansion of the Directory is done when the configuration file is read so that values such as $HOME will be properly expanded.

 The PID directory specified must already exist and be readable and writable by the Bareos daemon referencing it.

 Typically on Linux systems, you will set this to: :file:`/var/run`. If you are not installing Bareos in the system directories, you can use the Working Directory as defined above.
+
+As of bareos 21, this directive is deprecated. The way to set up a pid file is to do it as an option to the Director binary with the ``-p <file>`` option, where <file> is the path to a pidfile of your choosing. By default, no pidfile is created.


Suggested change

This directive is optional and specifies a directory in which the Director may put its process Id file. The process Id file is used to shut down Bareos and to prevent multiple copies of Bareos from running simultaneously. Standard shell expansion of the Directory is done when the configuration file is read so that values such as $HOME will be properly expanded.

The PID directory specified must already exist and be readable and writable by the Bareos daemon referencing it.

Typically on Linux systems, you will set this to: :file:`/var/run`. If you are not installing Bareos in the system directories, you can use the Working Directory as defined above.

As of bareos 21, this directive is deprecated. The way to set up a pid file is to do it as an option to the Director binary with the ``-p <file>`` option, where <file> is the path to a pidfile of your choosing. By default, no pidfile is created.

Since :sinceVersion:`21.0.0` this directive has no effect anymore. The way to set up a pid file is to do it as an option to the Director binary with the ``-p <file>`` option, where <file> is the path to a pidfile of your choosing. By default, no pidfile is created.

Makes sense, but shouldn't we keep the old description for users of previous versions?

arogge

The headers in daemon.cc and daemon.h need the newer year.
But the elephant in the room is all the occurances of "Pid Directory" in test- and example-configurations.
I'll take a look at that if you don't mind.

arogge · 2021-11-22T10:06:52Z

core/src/lib/daemon.cc

@@ -38,7 +38,10 @@



year in the header is 2020, should be 2021

arogge · 2021-11-22T10:07:01Z

core/src/lib/daemon.h

@@ -21,6 +21,6 @@
 #ifndef BAREOS_LIB_DAEMON_H_


year in the header is 2020, should be 2021

pstorz

I have some comments that may be discussed.

core/platforms/slackware/functions.bareos.in

core/scripts/bareos-ctl-funcs

core/src/dird/dird_conf.cc

core/src/dird/dird.cc

core/src/filed/filed.cc

core/manpages/bareos-fd.8

core/manpages/bareos-dir.8

core/manpages/bareos-fd.8

core/manpages/bareos-sd.8

pstorz

Good work!

The default is now not to create a pidfile. Daemons can be run with parameter `-p <file>` where <file> is the full path to a pidfile that should be created. Previously pidfiles were created in a rather unsafe manner in a location determined by Bareos' parameters.

This patch deprecates the now unused `Pid Directory` directives and removes the default value.

this patch removes all occurences of Pid Directory from every configuration file and template.

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 3 times, most recently from 030b322 to c4367dd Compare September 17, 2021 15:49

pstorz force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 2 times, most recently from 9b0bb1f to b4aaab5 Compare September 19, 2021 18:06

pstorz requested changes Sep 21, 2021

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from 752dae7 to fb73c9b Compare September 21, 2021 10:50

pstorz reviewed Sep 21, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 3 times, most recently from a382e73 to 57abb93 Compare September 21, 2021 14:51

alaaeddineelamri marked this pull request as ready for review September 22, 2021 09:05

arogge reviewed Sep 22, 2021

View reviewed changes

core/src/dird/dird.cc Show resolved Hide resolved

alaaeddineelamri marked this pull request as draft September 23, 2021 08:56

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from c43af60 to 9c638e7 Compare October 4, 2021 15:34

arogge reviewed Oct 6, 2021

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 3 times, most recently from 55ef5eb to 699eba8 Compare October 12, 2021 11:52

joergsteffens assigned arogge Oct 14, 2021

alaaeddineelamri marked this pull request as ready for review October 14, 2021 09:16

alaaeddineelamri requested a review from pstorz October 14, 2021 09:17

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 6 times, most recently from 50451d1 to 9b8892d Compare October 21, 2021 11:35

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 2 times, most recently from 2edef52 to 72a2407 Compare October 25, 2021 10:59

arogge requested changes Nov 8, 2021

View reviewed changes

core/src/filed/filed.cc Outdated Show resolved Hide resolved

core/src/lib/bsys.cc Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from 4f528a6 to 37bcc7b Compare November 9, 2021 11:57

arogge reviewed Nov 9, 2021

View reviewed changes

arogge approved these changes Nov 10, 2021

View reviewed changes

arogge reviewed Nov 10, 2021

View reviewed changes

pstorz approved these changes Nov 10, 2021

View reviewed changes

alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch 3 times, most recently from ad7d2f3 to 8f8884e Compare November 17, 2021 10:27

arogge requested changes Nov 22, 2021

View reviewed changes

arogge force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from 8f8884e to 9978b97 Compare November 22, 2021 14:37

pstorz requested changes Nov 23, 2021

View reviewed changes

arogge approved these changes Nov 29, 2021

View reviewed changes

arogge requested a review from pstorz November 29, 2021 15:27

arogge force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from 1577092 to b5fbe68 Compare November 30, 2021 11:13

pstorz approved these changes Nov 30, 2021

View reviewed changes

alaaeddineelamri and others added 10 commits November 30, 2021 20:40

dird: changed NULLs into nullptrs

9362f48

filed: replacing NULLs with nullptrs

d342f8a

stored: replacing NULLs with nullptrs

2d6b11b

manpages: update manpages for daemons

ec096d7

docs: deprecate Pid Directory directives

c87a7b3

This patch deprecates the now unused `Pid Directory` directives and removes the default value.

docs: update Pid Directory directive description

ae3473c

remove Pid Directory from configuration files

18e42a7

this patch removes all occurences of Pid Directory from every configuration file and template.

scripts: don't use @piddir@ anymore

94a894d

cmake: remove piddir

5a6255b

arogge force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from b5fbe68 to dc0b568 Compare November 30, 2021 20:08

update CHANGELOG.md

9764f61

arogge force-pushed the dev/alaaeddineelamri/master/issue-847-pid-file-creation branch from dc0b568 to 9764f61 Compare November 30, 2021 20:10

arogge merged commit 421078e into bareos:master Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changed when/where the pidfile of daemons gets created #928

changed when/where the pidfile of daemons gets created #928

alaaeddineelamri commented Sep 17, 2021 •

edited by pstorz

pstorz left a comment

arogge left a comment

arogge left a comment

arogge left a comment

arogge left a comment

arogge Nov 10, 2021

alaaeddineelamri Nov 10, 2021

arogge left a comment

arogge Nov 22, 2021

arogge Nov 22, 2021

pstorz left a comment

pstorz left a comment

changed when/where the pidfile of daemons gets created #928

changed when/where the pidfile of daemons gets created #928

Conversation

alaaeddineelamri commented Sep 17, 2021 • edited by pstorz

Description:

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

pstorz left a comment

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge Nov 10, 2021

Choose a reason for hiding this comment

alaaeddineelamri Nov 10, 2021

Choose a reason for hiding this comment

arogge left a comment

Choose a reason for hiding this comment

arogge Nov 22, 2021

Choose a reason for hiding this comment

arogge Nov 22, 2021

Choose a reason for hiding this comment

pstorz left a comment

Choose a reason for hiding this comment

pstorz left a comment

Choose a reason for hiding this comment

alaaeddineelamri commented Sep 17, 2021 •

edited by pstorz