Re: TOCTTOU race

[Maxime Devos]

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

From 2c3968f658ada27d2062a960d229f3db9cfe208c Mon Sep 17 00:00:00 2001
From: Maxime Devos 
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 
-;;; Copyright © 2015 Mark H Weaver 
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès 
+;;; Copyright © 2013 Nikita Karetnikov 
+;;; Copyright © 2013 Andreas Enge 
+;;; Copyright © 2015, 2018 Mark H Weaver 
+;;; Copyright © 2018 Arun Isaac 
+;;; Copyright © 2018, 2019 Ricardo Wurmus 
+;;; Copyright © 2021 Maxime Devos 
 ;;;
 ;;; 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)
+  

Will 2021 be the year of build systems on gexps?

[Ludovic Courtès]

Hello Guix!

Over the last few days I’ve been head-down working on
‘wip-build-systems-gexp’, the mythical branch that brings gexps to build
systems and packages, so we can say goodbye to
‘build-expression->derivation’.  And… it’s quite a ride!

I rebased it on ‘core-updates’, which was a bit tedious (it hadn’t been
touched in 3 years or so).  There are still test failures and build
systems not yet converted, but that’s the easy part, although both are
time-consuming.

The more difficult part is performance.  On current ‘core-updates’ I get:

--8<---cut here---start->8---
$ GUIX_PROFILING=gc time ./pre-inst-env guix build qemu -d --no-grafts
/gnu/store/z27l6plrxr5wm7818xhj9mdll99jcqz3-qemu-5.1.0.drv
Garbage collection statistics:
  heap size:80.52 MiB
  allocated:191.14 MiB
  GC times: 15
  time spent in GC: 0.66 seconds (32% of user time)
2.05user 0.12system 0:01.93elapsed 112%CPU (0avgtext+0avgdata 
242872maxresident)k
0inputs+0outputs (0major+34228minor)pagefaults 0swaps
--8<---cut here---end--->8---

but on ‘wip-build-systems-gexp’ I get:

--8<---cut here---start->8---
$ GUIX_PROFILING=gc time ./pre-inst-env guix build qemu -d --no-grafts
/gnu/store/5n44l8cmrmkr747nsqbxpm4764jdsl3l-qemu-5.1.0.drv
Garbage collection statistics:
  heap size:80.52 MiB
  allocated:249.89 MiB
  GC times: 16
  time spent in GC: 0.75 seconds (34% of user time)
2.22user 0.13system 0:02.06elapsed 114%CPU (0avgtext+0avgdata 
243532maxresident)k
0inputs+0outputs (0major+34304minor)pagefaults 0swaps
--8<---cut here---end--->8---

In short, 30% more garbage allocated and an 8% slowdown.

I’ve made progress identifying and mitigating what seems to be the main
cause of this (the fact that it’s possible to ungexp a list and that
list will be scanned in its entirety in search of file-like
objects—convenient but expensive) but as you can see, there’s still a
lot to do.

Statprof and gcprof are not super helpful here; I feel a need for better
tools here, or better metrics.

Anyway, that’s the situation.  If you have ideas or if you’re in a
profiling & optimization mood, now’s the time to unleash your
creativity.  :-)

Ludo’.

Re: Joining committers

[Ludovic Courtès]

Hi Léo,

Léo Le Bouter  skribis:

> I sent a commit access application few days ago and it was approved! I
> am really happy that is the case!

Yay, welcome on board!  :-)

Check out
 if
you haven’t already and don’t hesitate to ask other committers when in
doubt.

> I've been working on PowerPC 64-bits support on and off on GNU Guix
> since 2 years as I own a RaptorCS Talos II machine at home that runs
> with only free firmware and software.

It’s exciting to see your work coming to fruition!

Ludo’.