From: Maxime Devos <maximedevos@telenet.be>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: guix-devel@gnu.org
Subject: Re: TOCTTOU race
Date: Mon, 22 Feb 2021 20:13:05 +0100 [thread overview]
Message-ID: <4fbfe8a30a0516bc90dd854575fa585c54ac28ce.camel@telenet.be> (raw)
In-Reply-To: <87r1l8eb4a.fsf@gnu.org>
[-- Attachment #1.1: Type: text/plain, Size: 1204 bytes --]
Hi,
On Mon, 2021-02-22 at 09:54 +0100, Ludovic Courtès wrote:
> [...]
> > Subject: [PATCH] services: prevent following symlinks during activation
> ^
> Nitpick: we usually capitalize here and in the commit log.
Fixed! Also added a period at the end.
> Perhaps add a couple of lines explaining that this fixes a potential
> security issue, with a link to this thread.
Done. But since ....
> > Currently, there's a TOCTTOU race. This can be addressed
> > once guile has bindings for fstatat, openat and friends.
... I only claim it's a partial fix at best in the commit message.
> I’d move that comment next to the ‘mkdir-p/perms’ definition.
I copied it there, but left it (reworded slightly) in the commit
message, to avoid giving a false impression the potential security issue
is really fixed.
> > * guix/build/service-utils.scm: new module
> > with new procedure 'mkdir-p/perms'.
>
> I think you can remove these lines.
I removed the ‘Makefile.am’ and ‘guix/build/service-utils.scm’
lines which aren't relevant anymore, but kept the other lines.
Is all addressed now? (Aside from the TOCTTOU.)
Maxime.
[-- Attachment #1.2: 0001-services-Prevent-following-symlinks-during-activatio.patch --]
[-- Type: text/x-patch, Size: 12180 bytes --]
From 395208e1e8e1ab6dd3eb5739b2726f06a49e0041 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sun, 14 Feb 2021 12:57:32 +0100
Subject: [PATCH] services: Prevent following symlinks during activation.
This addresses a potential security issue, where a compromised
service could trick the activation code in changing the permissions,
owner and group of arbitrary files. However, this patch is
currently only a partial fix, due to a TOCTTOU (time-of-check to
time-of-use) race, which can be fixed once guile has bindings
to openat and friends.
Fixes: <https://lists.gnu.org/archive/html/guix-devel/2021-01/msg00388.html>
* gnu/build/activation.scm: new procedure 'mkdir-p/perms'.
* gnu/services/authentication.scm
(%nslcd-activation, nslcd-service-type): use new procedure.
* gnu/services/cups.scm (%cups-activation): likewise.
* gnu/services/dbus.scm (dbus-activation): likewise.
* gnu/services/dns.scm (knot-activation): likewise.
---
gnu/build/activation.scm | 53 +++++++++++++++++++++++++++++++--
gnu/services/authentication.scm | 22 ++++++++------
gnu/services/cups.scm | 12 ++++----
gnu/services/dbus.scm | 37 ++++++++++++-----------
gnu/services/dns.scm | 21 +++++++------
5 files changed, 98 insertions(+), 47 deletions(-)
diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index b458aee4ae..6cb6f8819b 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -1,6 +1,11 @@
;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
-;;; Copyright © 2015 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013 Nikita Karetnikov <nikita@karetnikov.org>
+;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
+;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
+;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -37,7 +42,8 @@
activate-modprobe
activate-firmware
activate-ptrace-attach
- activate-current-system))
+ activate-current-system
+ mkdir-p/perms))
;;; Commentary:
;;;
@@ -55,6 +61,47 @@
(define (dot-or-dot-dot? file)
(member file '("." "..")))
+;; Based upon mkdir-p from (guix build utils)
+(define (verify-not-symbolic dir)
+ "Verify DIR or its ancestors aren't symbolic links."
+ (define absolute?
+ (string-prefix? "/" dir))
+
+ (define not-slash
+ (char-set-complement (char-set #\/)))
+
+ (define (verify-component file)
+ (unless (eq? 'directory (stat:type (lstat file)))
+ (error "file name component is not a directory" dir)))
+
+ (let loop ((components (string-tokenize dir not-slash))
+ (root (if absolute?
+ ""
+ ".")))
+ (match components
+ ((head tail ...)
+ (let ((file (string-append root "/" head)))
+ (catch 'system-error
+ (lambda ()
+ (verify-component file)
+ (loop tail file))
+ (lambda args
+ (if (= ENOENT (system-error-errno args))
+ #t
+ (apply throw args))))))
+ (() #t))))
+
+;; TODO: the TOCTTOU race can be addressed once guile has bindings
+;; for fstatat, openat and friends.
+(define (mkdir-p/perms directory owner bits)
+ "Create the directory DIRECTORY and all its ancestors.
+Verify no component of DIRECTORY is a symbolic link.
+Warning: this is currently suspect to a TOCTTOU race!"
+ (verify-not-symbolic directory)
+ (mkdir-p directory)
+ (chown directory (passwd:uid owner) (passwd:gid owner))
+ (chmod directory bits))
+
(define* (copy-account-skeletons home
#:key
(directory %skeleton-directory)
diff --git a/gnu/services/authentication.scm b/gnu/services/authentication.scm
index 73969a5a6d..d7efc48cd0 100644
--- a/gnu/services/authentication.scm
+++ b/gnu/services/authentication.scm
@@ -1,6 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2018 Danny Milosavljevic <dannym@scratchpost.org>
;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -31,6 +32,7 @@
#:use-module (guix gexp)
#:use-module (guix records)
#:use-module (guix packages)
+ #:use-module (guix modules)
#:use-module (ice-9 match)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-26)
@@ -521,6 +523,16 @@ password.")
(define (pam-ldap-pam-services config)
(list (pam-ldap-pam-service config)))
+(define %nslcd-activation
+ (with-imported-modules (source-module-closure '((gnu build activation)))
+ #~(begin
+ (use-modules (gnu build activation))
+ (let ((rundir "/var/run/nslcd")
+ (user (getpwnam "nslcd")))
+ (mkdir-p/perms rundir user #o755)
+ (when (file-exists? "/etc/nslcd.conf")
+ (chmod "/etc/nslcd.conf" #o400))))))
+
(define nslcd-service-type
(service-type
(name 'nslcd)
@@ -531,15 +543,7 @@ password.")
(service-extension etc-service-type
nslcd-etc-service)
(service-extension activation-service-type
- (const #~(begin
- (use-modules (guix build utils))
- (let ((rundir "/var/run/nslcd")
- (user (getpwnam "nslcd")))
- (mkdir-p rundir)
- (chown rundir (passwd:uid user) (passwd:gid user))
- (chmod rundir #o755)
- (when (file-exists? "/etc/nslcd.conf")
- (chmod "/etc/nslcd.conf" #o400))))))
+ (const %nslcd-activation))
(service-extension pam-root-service-type
pam-ldap-pam-services)
(service-extension nscd-service-type
diff --git a/gnu/services/cups.scm b/gnu/services/cups.scm
index 17ed04e58b..20e3917b93 100644
--- a/gnu/services/cups.scm
+++ b/gnu/services/cups.scm
@@ -4,6 +4,7 @@
;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
;;; Copyright © 2019 Alex Griffin <a@ajgrf.com>
;;; Copyright © 2019 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -31,6 +32,7 @@
#:use-module (guix packages)
#:use-module (guix records)
#:use-module (guix gexp)
+ #:use-module (guix modules)
#:use-module (ice-9 match)
#:use-module ((srfi srfi-1) #:select (append-map find))
#:export (cups-service-type
@@ -871,13 +873,11 @@ IPP specifications.")
(define %cups-activation
;; Activation gexp.
- (with-imported-modules '((guix build utils))
+ (with-imported-modules (source-module-closure '((gnu build activation)
+ (guix build utils)))
#~(begin
- (use-modules (guix build utils))
- (define (mkdir-p/perms directory owner perms)
- (mkdir-p directory)
- (chown directory (passwd:uid owner) (passwd:gid owner))
- (chmod directory perms))
+ (use-modules (gnu build activation)
+ (guix build utils))
(define (build-subject parameters)
(string-concatenate
(map (lambda (pair)
diff --git a/gnu/services/dbus.scm b/gnu/services/dbus.scm
index e015d3f68d..af1a1e4c3a 100644
--- a/gnu/services/dbus.scm
+++ b/gnu/services/dbus.scm
@@ -1,6 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2015 Sou Bunnbu <iyzsong@gmail.com>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -28,6 +29,7 @@
#:use-module (guix gexp)
#:use-module ((guix packages) #:select (package-name))
#:use-module (guix records)
+ #:use-module (guix modules)
#:use-module (srfi srfi-1)
#:use-module (ice-9 match)
#:export (dbus-configuration
@@ -161,24 +163,23 @@ includes the @code{etc/dbus-1/system.d} directories of each package listed in
(define (dbus-activation config)
"Return an activation gexp for D-Bus using @var{config}."
- #~(begin
- (use-modules (guix build utils))
-
- (mkdir-p "/var/run/dbus")
-
- (let ((user (getpwnam "messagebus")))
- (chown "/var/run/dbus"
- (passwd:uid user) (passwd:gid user))
-
- ;; This directory contains the daemon's socket so it must be
- ;; world-readable.
- (chmod "/var/run/dbus" #o755))
-
- (unless (file-exists? "/etc/machine-id")
- (format #t "creating /etc/machine-id...~%")
- (invoke (string-append #$(dbus-configuration-dbus config)
- "/bin/dbus-uuidgen")
- "--ensure=/etc/machine-id"))))
+ (with-imported-modules (source-module-closure
+ '((gnu build activation)
+ (guix build utils)))
+ #~(begin
+ (use-modules (gnu build activation)
+ (guix build utils))
+
+ (let ((user (getpwnam "messagebus")))
+ ;; This directory contains the daemon's socket so it must be
+ ;; world-readable.
+ (mkdir-p/perms "/var/run/dbus" user #o755))
+
+ (unless (file-exists? "/etc/machine-id")
+ (format #t "creating /etc/machine-id...~%")
+ (invoke (string-append #$(dbus-configuration-dbus config)
+ "/bin/dbus-uuidgen")
+ "--ensure=/etc/machine-id")))))
(define dbus-shepherd-service
(match-lambda
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index d4aefe6285..55211cb08f 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -2,6 +2,7 @@
;;; Copyright © 2017 Julien Lepiller <julien@lepiller.eu>
;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
;;; Copyright © 2020 Pierre Langlois <pierre.langlois@gmx.com>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -28,6 +29,7 @@
#:use-module (guix packages)
#:use-module (guix records)
#:use-module (guix gexp)
+ #:use-module (guix modules)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
@@ -607,17 +609,14 @@
(shell (file-append shadow "/sbin/nologin")))))
(define (knot-activation config)
- #~(begin
- (use-modules (guix build utils))
- (define (mkdir-p/perms directory owner perms)
- (mkdir-p directory)
- (chown directory (passwd:uid owner) (passwd:gid owner))
- (chmod directory perms))
- (mkdir-p/perms #$(knot-configuration-run-directory config)
- (getpwnam "knot") #o755)
- (mkdir-p/perms "/var/lib/knot" (getpwnam "knot") #o755)
- (mkdir-p/perms "/var/lib/knot/keys" (getpwnam "knot") #o755)
- (mkdir-p/perms "/var/lib/knot/keys/keys" (getpwnam "knot") #o755)))
+ (with-imported-modules (source-module-closure '((gnu build activation)))
+ #~(begin
+ (use-modules (gnu build activation))
+ (mkdir-p/perms #$(knot-configuration-run-directory config)
+ (getpwnam "knot") #o755)
+ (mkdir-p/perms "/var/lib/knot" (getpwnam "knot") #o755)
+ (mkdir-p/perms "/var/lib/knot/keys" (getpwnam "knot") #o755)
+ (mkdir-p/perms "/var/lib/knot/keys/keys" (getpwnam "knot") #o755))))
(define (knot-shepherd-service config)
(let* ((config-file (knot-config-file config))
--
2.30.0
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
next prev parent reply other threads:[~2021-02-22 19:13 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-28 21:53 Potential security weakness in Guix services Leo Famulari
2021-01-29 13:33 ` Maxime Devos
2021-01-29 15:25 ` Maxime Devos
2021-02-01 15:35 ` Ludovic Courtès
2021-02-01 15:47 ` Julien Lepiller
2021-02-01 16:19 ` Maxime Devos
2021-02-02 13:07 ` Ludovic Courtès
2021-02-02 13:38 ` Maxime Devos
2021-02-02 15:30 ` Maxime Devos
2021-02-05 9:57 ` Ludovic Courtès
2021-02-05 12:20 ` Maxime Devos
2021-02-05 14:16 ` Maxime Devos
2021-02-06 21:28 ` Ludovic Courtès
2021-02-06 22:01 ` Maxime Devos
2021-02-10 20:45 ` Ludovic Courtès
2021-02-06 21:26 ` Ludovic Courtès
2021-02-14 12:29 ` TOCTTOU race (was: Potential security weakness in Guix services) Maxime Devos
2021-02-14 17:19 ` Bengt Richter
2021-02-18 17:54 ` TOCTTOU race Ludovic Courtès
2021-02-19 18:01 ` Maxime Devos
2021-02-22 8:54 ` Ludovic Courtès
2021-02-22 19:13 ` Maxime Devos [this message]
2021-02-23 15:30 ` Ludovic Courtès
2021-02-27 7:41 ` Maxime Devos
2021-03-10 10:07 ` Ludovic Courtès
2021-02-10 20:54 ` Potential security weakness in Guix services Christopher Lemmer Webber
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4fbfe8a30a0516bc90dd854575fa585c54ac28ce.camel@telenet.be \
--to=maximedevos@telenet.be \
--cc=guix-devel@gnu.org \
--cc=ludo@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.