System log files are world readable

  • Done
  • quality assurance status badge
Details
3 participants
  • Bengt Richter
  • Diego Nicola Barbato
  • Ludovic Courtès
Owner
unassigned
Submitted by
Diego Nicola Barbato
Severity
normal
D
D
Diego Nicola Barbato wrote on 3 Apr 2020 15:19
(address . bug-guix@gnu.org)
87v9mg1zbt.fsf@GlaDOS.home
Hey Guix,

On Guix System the log files (in /var/log) generated by syslogd are
currently (commit 151f3d4) world readable. They should probably only be
readable by root (for the same reason that dmesg can only be run by
root).

It isn't possible to set the umask with fork-exec-constructor, is it?
Otherwise that might have been a simple solution.

Regards,

Diego
D
D
Diego Nicola Barbato wrote on 3 Apr 2020 15:34
(address . 40405@debbugs.gnu.org)
87r1x41yna.fsf@GlaDOS.home
Diego Nicola Barbato <dnbarbato@posteo.de> writes:

Toggle quote (8 lines)
> Hey Guix,
>
> On Guix System the log files (in /var/log) generated by syslogd are
> currently (commit 151f3d4) world readable. They should probably only be
> readable by root (for the same reason that dmesg can only be run by
> root).
>
> It isn't possible to set the umask with fork-exec-constructor, is it?
^^^^^^^^^^^^^^^^^^^^^
That should be 'make-forkexec-constructor'. Sorry for the noise.

Toggle quote (5 lines)
> Otherwise that might have been a simple solution.
>
> Regards,
>
> Diego
L
L
Ludovic Courtès wrote on 5 Apr 2020 23:32
control message for bug #40405
(address . control@debbugs.gnu.org)
87blo5hb45.fsf@gnu.org
tags 40405 + security
quit
L
L
Ludovic Courtès wrote on 6 Apr 2020 00:12
Re: bug#40405: System log files are world readable
(name . Diego Nicola Barbato)(address . dnbarbato@posteo.de)(address . 40405@debbugs.gnu.org)
874ktxh99k.fsf@gnu.org
Hi,

Diego Nicola Barbato <dnbarbato@posteo.de> skribis:

Toggle quote (8 lines)
> On Guix System the log files (in /var/log) generated by syslogd are
> currently (commit 151f3d4) world readable. They should probably only be
> readable by root (for the same reason that dmesg can only be run by
> root).
>
> It isn't possible to set the umask with fork-exec-constructor, is it?
> Otherwise that might have been a simple solution.

That would be a nice solution to implement in the Shepherd. If you feel
like giving it a try, that would be great!

In the meantime, the patch below fixes the syslogd problem. Also
attached is a patch for the accounting database, though that one is
questionable.

Thoughts?

Thanks,
Ludo’.
Toggle diff (53 lines)
diff --git a/gnu/services.scm b/gnu/services.scm
index 7941cd3af0..d631e8dd32 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -528,15 +528,20 @@ ACTIVATION-SCRIPT-TYPE."
(use-modules (gnu build activation)
(guix build utils))
+ (define (ensure-file-exists file)
+ (let ((port (open-file file "a0")))
+ (chmod port #o640)
+ (close-port port)))
+
;; Make sure the user accounting database exists. If it
;; does not exist, 'setutxent' does not create it and
;; thus there is no accounting at all.
- (close-port (open-file "/var/run/utmpx" "a0"))
+ (ensure-file-exists "/var/run/utmpx")
;; Same for 'wtmp', which is populated by mingetty et
;; al.
(mkdir-p "/var/log")
- (close-port (open-file "/var/log/wtmp" "a0"))
+ (ensure-file-exists "/var/log/wtmp")
;; Set up /run/current-system. Among other things this
;; sets up locales, which the activation snippets
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 8d9a563e2b..e59b6fea80 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1436,10 +1436,17 @@ Service Switch}, for an example."
(documentation "Run the syslog daemon (syslogd).")
(provision '(syslogd))
(requirement '(user-processes))
- (start #~(make-forkexec-constructor
- (list #$(syslog-configuration-syslogd config)
- "--rcfile" #$(syslog-configuration-config-file config))
- #:pid-file "/var/run/syslog.pid"))
+ (start #~(let ((fork (make-forkexec-constructor
+ (list #$(syslog-configuration-syslogd config)
+ "--rcfile"
+ #$(syslog-configuration-config-file config))
+ #:pid-file "/var/run/syslog.pid")))
+ (lambda ()
+ ;; Set the umask such that file permissions are #o640.
+ (let ((mask (umask #o137))
+ (pid (fork)))
+ (umask mask)
+ pid))))
(stop #~(make-kill-destructor))))))
;; Snippet adapted from the GNU inetutils manual.
L
L
Ludovic Courtès wrote on 7 Apr 2020 00:07
(name . Diego Nicola Barbato)(address . dnbarbato@posteo.de)(address . 40405@debbugs.gnu.org)
87blo4clpp.fsf@gnu.org
Hi,

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (4 lines)
> In the meantime, the patch below fixes the syslogd problem. Also
> attached is a patch for the accounting database, though that one is
> questionable.

I pushed the syslog bits along with a test as commit
d7113bb655ff80a868a9e624c913f9d23e6c63ad. (I think already
world-readable files will remain world-readable though?)

The main remaining issue here is log files created by
‘fork+exec-command’. We’ll have to address that in the Shepherd proper,
I think.

Ludo’.
B
B
Bengt Richter wrote on 7 Apr 2020 02:49
(name . Ludovic Courtès)(address . ludo@gnu.org)
20200407004958.GA8760@LionPure
Hi Ludo,

On +2020-04-07 00:07:14 +0200, Ludovic Courtès wrote:
Toggle quote (13 lines)
> Hi,
>
> Ludovic Courtès <ludo@gnu.org> skribis:
>
> > In the meantime, the patch below fixes the syslogd problem. Also
> > attached is a patch for the accounting database, though that one is
> > questionable.
>
> I pushed the syslog bits along with a test as commit
> d7113bb655ff80a868a9e624c913f9d23e6c63ad. (I think already
> world-readable files will remain world-readable though?)
>

Could build daemons do some kind of maintenance rebuild to chmod them?
And maybe be scheduled to monitor new files for other mistakes as well?

Meanwhile, could a superuser chmod them without affecting hashes?
(curious as to whether permission bits escape hashing).

Toggle quote (9 lines)
> The main remaining issue here is log files created by
> ‘fork+exec-command’. We’ll have to address that in the Shepherd proper,
> I think.
>
> Ludo’.
>
>
>

--
Regards,
Bengt Richter
L
L
Ludovic Courtès wrote on 7 Apr 2020 09:30
(name . Bengt Richter)(address . bokr@bokr.com)
87zhbnbvmy.fsf@gnu.org
Hi,

Bengt Richter <bokr@bokr.com> skribis:

Toggle quote (17 lines)
> On +2020-04-07 00:07:14 +0200, Ludovic Courtès wrote:
>> Hi,
>>
>> Ludovic Courtès <ludo@gnu.org> skribis:
>>
>> > In the meantime, the patch below fixes the syslogd problem. Also
>> > attached is a patch for the accounting database, though that one is
>> > questionable.
>>
>> I pushed the syslog bits along with a test as commit
>> d7113bb655ff80a868a9e624c913f9d23e6c63ad. (I think already
>> world-readable files will remain world-readable though?)
>>
>
> Could build daemons do some kind of maintenance rebuild to chmod them?
> And maybe be scheduled to monitor new files for other mistakes as well?

Yes, we could do that, I just haven’t checked if this is necessary or
thought about how to do it.

Toggle quote (2 lines)
> Meanwhile, could a superuser chmod them without affecting hashes?

Definitely. (There’s no “hashing” involved for /var/log.)

Ludo’.
D
D
Diego Nicola Barbato wrote on 8 Apr 2020 14:32
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40405@debbugs.gnu.org)
87pnciximi.fsf@GlaDOS.home
Hey,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (15 lines)
> Hi,
>
> Diego Nicola Barbato <dnbarbato@posteo.de> skribis:
>
>> On Guix System the log files (in /var/log) generated by syslogd are
>> currently (commit 151f3d4) world readable. They should probably only be
>> readable by root (for the same reason that dmesg can only be run by
>> root).
>>
>> It isn't possible to set the umask with fork-exec-constructor, is it?
>> Otherwise that might have been a simple solution.
>
> That would be a nice solution to implement in the Shepherd. If you feel
> like giving it a try, that would be great!

I've attached two patches for the Shepherd. The first one makes sure
that 'exec-command' creates log files with mode #o640 (I thought about
making it a parameter instead of hard coding it, but I doubt it would be
very useful). The second one makes it possible to set the umask with
'exec-command', 'fork+exec-command', and 'make-forkexec-constructor'. I
wasn't quite sure how to avoid a collision with the procedure umask
(would `((@ (guile) umask) umask)' have been ok?) so I named the
parameter file-creation-mask.

I haven't tested the changes. What would be a straight forward way to
do that on Guix? Looking at the documentation it doesn't seem possible
to swap out the shepherd package of the %shepherd-root-service with
'modify-services'.

[...]

Regards,

Diego
From 43c9ded791ce5b480504ce3528ee34578168f90e Mon Sep 17 00:00:00 2001
From: Diego Nicola Barbato <dnbarbato@posteo.de>
Date: Tue, 7 Apr 2020 13:58:28 +0200
Subject: [PATCH 1/2] service: Create log files as non-world-readable.

* modules/shepherd/service.scm (exec-command): Create log-file with file
permissions #o640.
---
modules/shepherd/service.scm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Toggle diff (15 lines)
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index fc82cc4..9a4a5d9 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -808,7 +808,7 @@ false."
;; Redirect stout and stderr to use LOG-FILE.
(catch-system-error (close-fdes 1))
(catch-system-error (close-fdes 2))
- (dup2 (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND)) 1)
+ (dup2 (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) #o640) 1)
(dup2 1 2))
(lambda (key . args)
(format (current-error-port)
--
2.26.0
L
L
Ludovic Courtès wrote on 8 Apr 2020 21:49
(name . Diego Nicola Barbato)(address . dnbarbato@posteo.de)(address . 40405@debbugs.gnu.org)
877dyp69mz.fsf@gnu.org
Diego Nicola Barbato <dnbarbato@posteo.de> skribis:

Toggle quote (26 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi,
>>
>> Diego Nicola Barbato <dnbarbato@posteo.de> skribis:
>>
>>> On Guix System the log files (in /var/log) generated by syslogd are
>>> currently (commit 151f3d4) world readable. They should probably only be
>>> readable by root (for the same reason that dmesg can only be run by
>>> root).
>>>
>>> It isn't possible to set the umask with fork-exec-constructor, is it?
>>> Otherwise that might have been a simple solution.
>>
>> That would be a nice solution to implement in the Shepherd. If you feel
>> like giving it a try, that would be great!
>
> I've attached two patches for the Shepherd. The first one makes sure
> that 'exec-command' creates log files with mode #o640 (I thought about
> making it a parameter instead of hard coding it, but I doubt it would be
> very useful). The second one makes it possible to set the umask with
> 'exec-command', 'fork+exec-command', and 'make-forkexec-constructor'. I
> wasn't quite sure how to avoid a collision with the procedure umask
> (would `((@ (guile) umask) umask)' have been ok?) so I named the
> parameter file-creation-mask.

Sounds good to me.

Toggle quote (5 lines)
> I haven't tested the changes. What would be a straight forward way to
> do that on Guix? Looking at the documentation it doesn't seem possible
> to swap out the shepherd package of the %shepherd-root-service with
> 'modify-services'.

Both patches LGTM, but you could add a couple of tests in the Shepherd
itself before testing it on Guix.

The tests/*.sh are simple shell scripts. You could perhaps create a new
one there, run shepherd with a toy service that uses #:log-file and
creates files, and then ensure that the log file is #o640 and that
#:file-creation-mask is honored.

Does that make sense?

Then, to test it on Guix, you can run “make dist” in the Shepherd and
change the ‘shepherd’ package so that its ‘source’ points to that
tarball. You run ‘guix system vm gnu/system/examples/bare-bones.tmpl’,
boot that, and ensure everything’s OK.

Thanks!

Ludo’.
L
L
Ludovic Courtès wrote on 19 Apr 2020 16:28
(name . Diego Nicola Barbato)(address . dnbarbato@posteo.de)(address . 40405-done@debbugs.gnu.org)
87a73735yv.fsf@gnu.org
Hi Diego,

Diego Nicola Barbato <dnbarbato@posteo.de> skribis:

Toggle quote (8 lines)
>>From 43c9ded791ce5b480504ce3528ee34578168f90e Mon Sep 17 00:00:00 2001
> From: Diego Nicola Barbato <dnbarbato@posteo.de>
> Date: Tue, 7 Apr 2020 13:58:28 +0200
> Subject: [PATCH 1/2] service: Create log files as non-world-readable.
>
> * modules/shepherd/service.scm (exec-command): Create log-file with file
> permissions #o640.

[...]

Toggle quote (14 lines)
>>From e491436967a912e6e7372f582b3bf5c9784b8209 Mon Sep 17 00:00:00 2001
> From: Diego Nicola Barbato <dnbarbato@posteo.de>
> Date: Tue, 7 Apr 2020 13:38:47 +0200
> Subject: [PATCH 2/2] service: Add #:file-creation-mask to
> 'make-forkexec-constructor'.
>
> * modules/shepherd/service.scm (exec-command): Add #:file-creation-mask
> parameter and honor it.
> (fork+exec-command): Add #:file-creation-mask parameter and pass it to
> exec-command.
> (make-forkexec-constructor): Add #:file-creation-mask parameter and pass it
> to fork+exec-command.
> * doc/shepherd.texi (Service De- and Constructors): Adjust accordingly.

I went ahead and pushed these two patches.

We’ll need to test current Shepherd master on Guix, but I feel we’ve
accumulated enough improvements for a 0.7.1 release.

Thanks,
Ludo’.
Closed
L
L
Ludovic Courtès wrote on 22 Apr 2020 22:04
(address . 40405-done@debbugs.gnu.org)(address . dnbarbato@posteo.de)
878sinthh4.fsf@gnu.org
Hi,

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (28 lines)
> Diego Nicola Barbato <dnbarbato@posteo.de> skribis:
>
>>>From 43c9ded791ce5b480504ce3528ee34578168f90e Mon Sep 17 00:00:00 2001
>> From: Diego Nicola Barbato <dnbarbato@posteo.de>
>> Date: Tue, 7 Apr 2020 13:58:28 +0200
>> Subject: [PATCH 1/2] service: Create log files as non-world-readable.
>>
>> * modules/shepherd/service.scm (exec-command): Create log-file with file
>> permissions #o640.
>
> [...]
>
>>>From e491436967a912e6e7372f582b3bf5c9784b8209 Mon Sep 17 00:00:00 2001
>> From: Diego Nicola Barbato <dnbarbato@posteo.de>
>> Date: Tue, 7 Apr 2020 13:38:47 +0200
>> Subject: [PATCH 2/2] service: Add #:file-creation-mask to
>> 'make-forkexec-constructor'.
>>
>> * modules/shepherd/service.scm (exec-command): Add #:file-creation-mask
>> parameter and honor it.
>> (fork+exec-command): Add #:file-creation-mask parameter and pass it to
>> exec-command.
>> (make-forkexec-constructor): Add #:file-creation-mask parameter and pass it
>> to fork+exec-command.
>> * doc/shepherd.texi (Service De- and Constructors): Adjust accordingly.
>
> I went ahead and pushed these two patches.

These patches are in Shepherd 0.8.0, which was pushed in Guix master
commit e3358a831e7d5d9e8dc614340e49ea5aeb11a7ff, so we’re done!

Ludo’.
Closed
D
D
Diego Nicola Barbato wrote on 28 Apr 2020 15:11
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40405-done@debbugs.gnu.org)
87y2qfhi0c.fsf@GlaDOS.home
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (35 lines)
> Hi,
>
> Ludovic Courtès <ludo@gnu.org> skribis:
>
>> Diego Nicola Barbato <dnbarbato@posteo.de> skribis:
>>
>>>>From 43c9ded791ce5b480504ce3528ee34578168f90e Mon Sep 17 00:00:00 2001
>>> From: Diego Nicola Barbato <dnbarbato@posteo.de>
>>> Date: Tue, 7 Apr 2020 13:58:28 +0200
>>> Subject: [PATCH 1/2] service: Create log files as non-world-readable.
>>>
>>> * modules/shepherd/service.scm (exec-command): Create log-file with file
>>> permissions #o640.
>>
>> [...]
>>
>>>>From e491436967a912e6e7372f582b3bf5c9784b8209 Mon Sep 17 00:00:00 2001
>>> From: Diego Nicola Barbato <dnbarbato@posteo.de>
>>> Date: Tue, 7 Apr 2020 13:38:47 +0200
>>> Subject: [PATCH 2/2] service: Add #:file-creation-mask to
>>> 'make-forkexec-constructor'.
>>>
>>> * modules/shepherd/service.scm (exec-command): Add #:file-creation-mask
>>> parameter and honor it.
>>> (fork+exec-command): Add #:file-creation-mask parameter and pass it to
>>> exec-command.
>>> (make-forkexec-constructor): Add #:file-creation-mask parameter and pass it
>>> to fork+exec-command.
>>> * doc/shepherd.texi (Service De- and Constructors): Adjust accordingly.
>>
>> I went ahead and pushed these two patches.
>
> These patches are in Shepherd 0.8.0, which was pushed in Guix master
> commit e3358a831e7d5d9e8dc614340e49ea5aeb11a7ff, so we’re done!

Great! Now we can simplify the 'start' method of
'syslogd-service-type'.

I did eventually write a test script, which I've attached in case we
want to add it to the Shepherd. I'm sorry it took so long that I missed
the new Shepherd release.

Regards,

Diego
From 2e7a0795b3a7080376238ab604c50d2c180f8730 Mon Sep 17 00:00:00 2001
From: Diego Nicola Barbato <dnbarbato@posteo.de>
Date: Mon, 27 Apr 2020 16:57:36 +0200
Subject: [PATCH] tests: Test #:file-creation-mask option of
'make-forkexec-constructor'.

* tests/file-creation-mask.sh: New file.
---
tests/file-creation-mask.sh | 79 +++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
create mode 100644 tests/file-creation-mask.sh

Toggle diff (87 lines)
diff --git a/tests/file-creation-mask.sh b/tests/file-creation-mask.sh
new file mode 100644
index 0000000..9f5f10a
--- /dev/null
+++ b/tests/file-creation-mask.sh
@@ -0,0 +1,79 @@
+# GNU Shepherd --- Test the #:file-creation-mask option of 'make-forkexec-constructor'.
+# Copyright © 2020 Diego N. Barbato <dnbarbato@posteo.de>
+#
+# This file is part of the GNU Shepherd.
+#
+# The GNU Shepherd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or (at
+# your option) any later version.
+#
+# The GNU Shepherd is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with the GNU Shepherd. If not, see <http://www.gnu.org/licenses/>.
+
+shepherd --version
+herd --version
+
+socket="t-socket-$$"
+conf="t-conf-$$"
+log="t-log-$$"
+pid="t-pid-$$"
+service_log="t-service-log-$$"
+service_new_file="t-service-new-file-$$"
+
+herd="herd -s $socket"
+
+trap "cat $log || true;
+ rm -f $socket $conf $log $service_log $service_new_file;
+ test -f $pid && kill \`cat $pid\` || true; rm -f $pid" EXIT
+
+function wait_for_file
+{
+ i=0
+ while ! test -f "$1" && test $i -lt 20
+ do
+ sleep 0.3
+ i=`expr $i + 1`
+ done
+ test -f "$1"
+}
+
+cat > "$conf"<<EOF
+(define %command
+ '("$SHELL" "-c" "touch $PWD/$service_new_file; echo foo"))
+
+(register-services
+ (make <service>
+ #:provides '(test)
+ #:start (make-forkexec-constructor %command
+ #:log-file "$PWD/$service_log"
+ ;; Set the umask such that file
+ ;; permissions are #o600.
+ #:file-creation-mask #o177)
+ #:stop (make-kill-destructor)
+ #:respawn? #f))
+EOF
+
+rm -f "$pid"
+shepherd -I -s "$socket" -c "$conf" -l "$log" --pid="$pid" &
+
+# Wait till it's ready.
+wait_for_file "$pid"
+
+# Start the service.
+$herd start test
+
+# Make sure the log file is created with the right permissions independently
+# of the value of #:file-creation-mask.
+wait_for_file "$service_log"
+test `stat -c %a "$service_log"` -eq 640
+
+# Make sure the service creates files with the right permissions as determined
+# by the value of #:file-creation-mask.
+wait_for_file "$service_new_file"
+test `stat -c %a "$service_new_file"` -eq 600
--
2.26.0
Closed
L
L
Ludovic Courtès wrote on 28 Apr 2020 22:57
(name . Diego Nicola Barbato)(address . dnbarbato@posteo.de)(address . 40405-done@debbugs.gnu.org)
87h7x3tjjx.fsf@gnu.org
Hello,

Diego Nicola Barbato <dnbarbato@posteo.de> skribis:

Toggle quote (3 lines)
> Great! Now we can simplify the 'start' method of
> 'syslogd-service-type'.

Oh right, do you want to take care of it?

Toggle quote (4 lines)
> I did eventually write a test script, which I've attached in case we
> want to add it to the Shepherd. I'm sorry it took so long that I missed
> the new Shepherd release.

No problem. I figured it was okay to add it without a test, but having
a test is always better so I’ve happily pushed your patch. Thank you!

Ludo’.
Closed
D
D
Diego Nicola Barbato wrote on 29 Apr 2020 12:02
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40405@debbugs.gnu.org)
87d07qhaoy.fsf@GlaDOS.home
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (9 lines)
> Hello,
>
> Diego Nicola Barbato <dnbarbato@posteo.de> skribis:
>
>> Great! Now we can simplify the 'start' method of
>> 'syslogd-service-type'.
>
> Oh right, do you want to take care of it?


[...]

Regards,

Diego
?