From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <guix-devel-bounces+larch=yhetil.org@gnu.org>
Received: from mp1 ([2001:41d0:8:6d80::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by ms11 with LMTPS
	id WCmiMRiqLmBvIgAA0tVLHw
	(envelope-from <guix-devel-bounces+larch=yhetil.org@gnu.org>)
	for <larch@yhetil.org>; Thu, 18 Feb 2021 17:55:36 +0000
Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by mp1 with LMTPS
	id 81tyLRiqLmClBwAAbx9fmQ
	(envelope-from <guix-devel-bounces+larch=yhetil.org@gnu.org>)
	for <larch@yhetil.org>; Thu, 18 Feb 2021 17:55:36 +0000
Received: from lists.gnu.org (lists.gnu.org [209.51.188.17])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by aspmx1.migadu.com (Postfix) with ESMTPS id 7320D320EA
	for <larch@yhetil.org>; Thu, 18 Feb 2021 18:55:36 +0100 (CET)
Received: from localhost ([::1]:43418 helo=lists1p.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.90_1)
	(envelope-from <guix-devel-bounces+larch=yhetil.org@gnu.org>)
	id 1lCnWZ-0004ne-Eh
	for larch@yhetil.org; Thu, 18 Feb 2021 12:55:35 -0500
Received: from eggs.gnu.org ([2001:470:142:3::10]:38644)
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ludo@gnu.org>) id 1lCnVS-0003jH-9n
 for guix-devel@gnu.org; Thu, 18 Feb 2021 12:54:26 -0500
Received: from fencepost.gnu.org ([2001:470:142:3::e]:47277)
 by eggs.gnu.org with esmtp (Exim 4.90_1)
 (envelope-from <ludo@gnu.org>)
 id 1lCnVQ-00048w-7K; Thu, 18 Feb 2021 12:54:24 -0500
Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=46884 helo=ribbon)
 by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256)
 (Exim 4.82) (envelope-from <ludo@gnu.org>)
 id 1lCnVP-0008H8-NM; Thu, 18 Feb 2021 12:54:24 -0500
From: =?utf-8?Q?Ludovic_Court=C3=A8s?= <ludo@gnu.org>
To: Maxime Devos <maximedevos@telenet.be>
Subject: Re: TOCTTOU race
References: <YBMybeFOP0VfW6G7@jasmine.lan> <87k0rrls0z.fsf@gnu.org>
 <08F0CD76-DDCF-4CFA-AE8D-5FB165A62B25@lepiller.eu>
 <c7e82df3921fb0eaefb9db798d634f63f6eb0142.camel@telenet.be>
 <87o8h2ehy7.fsf@gnu.org>
 <69968b3a01d872cabdf55a94b6c82d5057e010c9.camel@telenet.be>
 <87v9b66dm1.fsf@gnu.org>
 <56adb5efa894304c27beba99b07e2f8cfd8ee7cb.camel@telenet.be>
 <87zh0gzy52.fsf@gnu.org>
 <53c60ce40d68cfc93a9ea2c4a8f865026e12c889.camel@telenet.be>
X-URL: http://www.fdn.fr/~lcourtes/
X-Revolutionary-Date: 30 =?utf-8?Q?Pluvi=C3=B4se?= an 229 de la
 =?utf-8?Q?R=C3=A9volution?=
X-PGP-Key-ID: 0x090B11993D9AEBB5
X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc
X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4  0CFB 090B 1199 3D9A EBB5
X-OS: x86_64-pc-linux-gnu
Date: Thu, 18 Feb 2021 18:54:22 +0100
In-Reply-To: <53c60ce40d68cfc93a9ea2c4a8f865026e12c889.camel@telenet.be>
 (Maxime Devos's message of "Sun, 14 Feb 2021 13:29:29 +0100")
Message-ID: <87h7m9p8hd.fsf@gnu.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
X-BeenThere: guix-devel@gnu.org
X-Mailman-Version: 2.1.23
Precedence: list
List-Id: "Development of GNU Guix and the GNU System distribution."
 <guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
 <mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/guix-devel>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
 <mailto:guix-devel-request@gnu.org?subject=subscribe>
Cc: guix-devel@gnu.org
Errors-To: guix-devel-bounces+larch=yhetil.org@gnu.org
Sender: "Guix-devel" <guix-devel-bounces+larch=yhetil.org@gnu.org>
X-Migadu-Flow: FLOW_IN
X-Migadu-Spam-Score: -1.37
Authentication-Results: aspmx1.migadu.com;
	dkim=none;
	dmarc=pass (policy=none) header.from=gnu.org;
	spf=pass (aspmx1.migadu.com: domain of guix-devel-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-devel-bounces@gnu.org
X-Migadu-Queue-Id: 7320D320EA
X-Spam-Score: -1.37
X-Migadu-Scanner: scn1.migadu.com
X-TUID: ATTzhtAnFK4J

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 =E2=80=A6) 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=E2=80=99s 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=E2=80=99d get a REPL rather than a kernel panic, but it=E2=80=99d=
 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, =E2=80=9Cpath=E2=80=9D is for =E2=80=9Csearch =
paths=E2=80=9D; here it
should be =E2=80=9Cfile=E2=80=9D or something.

> +         (catch 'system-error
> +           (lambda ()
> +             (verify-component path)
> +             (loop tail path))
> +           (lambda args
> +             (if (=3D ENOENT (system-error-errno args))
> +                 #t
> +                 (apply throw args))))))
> +      (() #t))))

That reminded me of the =E2=80=98safe_path=E2=80=99 function in OpenSSH, bu=
t that one
checks the permissions on file name components, and doesn=E2=80=99t check f=
or
symlinks (maybe it should; there=E2=80=99s 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=E2=80=99.