From: "Ludovic Courtès" <ludo@gnu.org>
To: Maxime Devos <maximedevos@telenet.be>
Cc: guix-devel@gnu.org
Subject: Re: TOCTTOU race
Date: Thu, 18 Feb 2021 18:54:22 +0100 [thread overview]
Message-ID: <87h7m9p8hd.fsf@gnu.org> (raw)
In-Reply-To: <53c60ce40d68cfc93a9ea2c4a8f865026e12c889.camel@telenet.be> (Maxime Devos's message of "Sun, 14 Feb 2021 13:29:29 +0100")
Hi Maxime,
Maxime Devos <maximedevos@telenet.be> skribis:
> From ad10c577eb1f13b9b66ea387648671df33b869d7 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.
>
> XXX I'm horrible at naming exceptions:
> (throw 'XXX-TODO-does-someone-have-an-idea? path)
>
> * 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.
Nice!
> +(define-module (guix build service-utils)
> + #:use-module (ice-9 match)
> + #:use-module (guix build utils)
> + #:export (mkdir-p/perms))
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.
> +;; 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 path)
> + (when (eq? 'symlink (stat:type (lstat path)))
> + (throw 'XXX-TODO-does-someone-have-an-idea? path)))
It’s tempting to do something like:
(error "file name component is 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.)
> + (let loop ((components (string-tokenize dir not-slash))
> + (root (if absolute?
> + ""
> + ".")))
> + (match components
> + ((head tail ...)
> + (let ((path (string-append root "/" head)))
Per GNU and Guix convention, “path” is for “search paths”; here it
should be “file” or something.
> + (catch 'system-error
> + (lambda ()
> + (verify-component path)
> + (loop tail path))
> + (lambda args
> + (if (= ENOENT (system-error-errno args))
> + #t
> + (apply throw args))))))
> + (() #t))))
That reminded me of the ‘safe_path’ function in OpenSSH, but that one
checks the permissions on file name components, and doesn’t check for
symlinks (maybe it should; there’s an XXX comment there.)
> +(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))
Otherwise LGTM, thanks!
Ludo’.
next prev parent reply other threads:[~2021-02-18 17:55 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 ` Ludovic Courtès [this message]
2021-02-19 18:01 ` TOCTTOU race Maxime Devos
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=87h7m9p8hd.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=guix-devel@gnu.org \
--cc=maximedevos@telenet.be \
/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.