all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Maxime Devos <maximedevos@telenet.be>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: guix-devel@gnu.org
Subject: Re: TOCTTOU race
Date: Fri, 19 Feb 2021 19:01:11 +0100	[thread overview]
Message-ID: <ef430e273b9199014038403d4deeaf9dbcb85e49.camel@telenet.be> (raw)
In-Reply-To: <87h7m9p8hd.fsf@gnu.org>


[-- Attachment #1.1: Type: text/plain, Size: 1743 bytes --]

On Thu, 2021-02-18 at 18:54 +0100, Ludovic Courtès wrote:
> [...]
> I think this should go either in (gnu build activation) or in a new (gnu
> build utils) module.
> 
> (guix build …) is for non-Guix-System things.

I've moved mkdir-p/perms into (gnu build activation).

> > +;; Based upon mkdir-p from (guix build utils)
> > +(define (verify-not-symbolic dir)
> > +  [...])

I've replaced the (when (eq? 'symlink) ...) with
(unless (eq? 'directory) ...).

> It’s tempting to do something like:
> 
>   (error "file name component is a directory" dir)

I've added a "not" between "is" and "a" ->
  (error "file name component is not a directory" dir)

> Note that, if that happens at boot time, the system will fail to boot (I
> think you’d get a REPL rather than a kernel panic, but it’d be good to
> check in a VM.)

If that happens, that's too bad.  Just ignoring the error seems bad from
a security perspective.  I verified in a VM you'd get a REPL.
From the REPL, a sysadmin could investigate and choose to delete the offending
symlink & reboot (and presumably fix the security bug and upgrade the service),
or decide Guix System needs to be reinstalled.

> > [...]
> 
> Per GNU and Guix convention, “path” is for “search paths”; here it
> should be “file” or something.

Changed in new patch (attached).

Apparently, I forgot a few #:use-module.  This should be corrected now.

Please take note that I didn't correct all potentially insecure activation gexps.
These should ideally be done by someone who knows how to use the particular service
and have a system to test it on.  (My changes to nscld-service-type and knot-activation
are untested.)

Greetings,
Maxime


[-- Attachment #1.2: 0001-services-prevent-following-symlinks-during-activatio.patch --]
[-- Type: text/x-patch, Size: 11837 bytes --]

From 2c3968f658ada27d2062a960d229f3db9cfe208c 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

Currently, there's a TOCTTOU race.  This can be addressed
once guile has bindings for fstatat, openat and friends.

* guix/build/service-utils.scm: new module
  with new procedure 'mkdir-p/perms'.
* Makefile.am (MODULES): compile new module.
* 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        | 51 +++++++++++++++++++++++++++++++--
 gnu/services/authentication.scm | 22 ++++++++------
 gnu/services/cups.scm           | 12 ++++----
 gnu/services/dbus.scm           | 37 ++++++++++++------------
 gnu/services/dns.scm            | 21 +++++++-------
 5 files changed, 96 insertions(+), 47 deletions(-)

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index b458aee4ae..4ee51dfd6e 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,45 @@
 (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))))
+
+(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 TOCTOU 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 --]

  reply	other threads:[~2021-02-19 18:01 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 [this message]
2021-02-22  8:54                       ` Ludovic Courtès
2021-02-22 19:13                         ` Maxime Devos
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=ef430e273b9199014038403d4deeaf9dbcb85e49.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.