Re: Potential security weakness in Guix services

[Maxime Devos <maximedevos@telenet.be>]

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

On Fri, 2021-01-29 at 14:33 +0100, Maxime Devos wrote:
> Hi Guix,
> [...]
> > Below is a summary of their messages, including a mitigation proposal.
> > Your feedback is requested!
> 
> I'm writing a patch right now.  It's a little more elaborate than my
> mkdir-p/own proposal.  In the patch, directories with owner, group
> and permission bits are created via extensions to a ‘fs-entry-service-type’,
> which will perform various basic consistency checks at build time
> (e.g., no directory can be owned by multiple users).
> 
> I'll post a draft when it's ready.

[First draft is attached, with many parts missing, it doesn't even
compile]
I think I've got a basic idea on how to handle this.
Some problems to address:

* Guile does not have ‘openat, mkdirat’ procedures.
  How to resolve: implement these upstream, write FFI bindings,
  or use 'chdir' carefully.

* Verify whether symlinks are handled correctly.
  (stat vs lstat vs fstatat ...)

* Perhaps O_NOCTTY, O_NOLINK, O_NOTRANS, O_NONBLOCK, O_DIRECTORY,
  O_NOFOLLOW ... need to be used at some places.

* Maybe fsync needs to be used in some places.

  The service definitions don't seem to do that anywhere when chmodding
  and chowning, so not implementing this shouldn't be a regression,
  but it does seem like something to verify.

* On some Linux versions and filesystems,
  the use of O_TMPFILE might simplify reasoning about security properties,
  race windows, etc., but idk if it's supported on the Hurd, and which
  (linux version, filesystem) combinations are supported.

* Mounting filesystems.
  Can all filesystems used by services when activating be assumed to be up?
  idk. 

* Support more security stuff (SELinux, SMACK, POSIX ACL, ...)
  Something for the far future, perhaps?

Perhaps I should just implement the basic mkdir-p/own proposal for now,
and in the future something more elaborate can be implemented?
All but the last two points probably still apply, though.

I'll take a look at how other systems handle this.

Maxime

[-- Attachment #1.2: directory-setup.scm --]
[-- Type: text/x-scheme, Size: 8414 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
(define-module (gnu services fs-entry)
  #:use-module stuff ...)
;;;
;;; Create directory structures for services with security context,
;;; without race conditions.  Symbolic links are not followed.
;;;

;; Values passed in extensions to @code{fs-entry-service-type}.
;; TODO maybe also allow defining SELinux, SMACK and POSIX ACL.
(define-record-type* <fs-entry>
  fs-entry make-fs-entry fs-entry?
  (where fs-entry-where) ; /name/of/file
  (bits  fs-entry-bits) ; permission bits
  (type  fs-entry-type) ; directory, regular or symlink
  (owner fs-entry-owner) ; owner, as a string
  (group fs-entry-group)) ; group, as a string

;; Likewise, but converted to a tree structure.
(define-record-type* <fs-entry/tree>
  fs-entry/tree make-fs-entry/tree fs-entry/tree?
  (name     fs-entry/tree-where) ; basename
  (bits     fs-entry/tree-bits) ; permission bits
  (type     fs-entry/tree-type) ; directory, regular or symlink
  (owner    fs-entry/tree-owner) ; owner, as a string
  (group    fs-entry/tree-group) ; group, as a string
  ;; boolean, for when <fs-entry> for /a/b is defined,
  ;; but not <fs-entry> for / and /a are defined, in which case
  ;; a ‘filler?’ <fs-entry/tree> for / and /a are created
  ;; in fs-entries->tree, which have as child /a and /a/b
  ;; respectively.
  ;;
  ;; (Note: the security context for / is currently ignored)
  (filler?  fs-entry/tree-filler?
            (default #f))
  ;; list of known children
  (children fs-entry-children))

(define %directory-separator #\/)
(define (fs-entry-name-components x)
  (string-split (fs-entry-where x) %directory-separator))

(define (fs-entries->tree list)
  "Translate @var{list}, a list of @code{fs-entry}, into a tree
structure (of <fs-entry/tree>)."
  ;; Sort list to prepare for a depth-first construction
  (define (list<? component<? x y)
    (cond ((and (null? x) (null? y)) #f)
	  ((null? x) #t)
	  ((null? y) #f)
	  ((component<? (car x) (car y)) #t)
	  ((component<? (car y) (car x)) #f)
	  (else (list<? component<? (cdr x) (cdr y)))))
  (define (entry<? x y)
    (list<? string<?
            (fs-entry-name-components x)
            (fs-entry-name-components y)))
  (define sorted (sort list entry<?))
  ;; Now construct the tree.
  ;; XXX insert filler for ???
  XXX
  ;; XXX make sure there are no inconsistencies
  ;; XXX prevent some screw-ups such as chowning or chmodding
  ;; entries from /gnu/store/.... Maybe that's prevented
  ;; by bind-mounting anyway.
  ;; (e.g. a symlink and directory with the same name).
  )

(define (tree->alist tree)
  `((name . ,(fs-entry/tree-name tree))
    (bits . ,(fs-entry/tree-bits tree))
    (type . ,(fs-entry/tree-type tree))
    (owner . ,(fs-entry/tree-owner tree))
    (group . ,(fs-entry/tree-group tree))
    (filler? . ,(fs-entry/tree-filler? tree))))

(define* (fs-entry-activation tree)
  ;; XXX for efficiency reasons, it might be useful to implement
  ;; some sort of caching mechanism to avoid looking up a uid/gid
  ;; multiple times from user name / user gid.
  #~(let* ((root (open "/" O_RDONLY))
           (ref  (lambda (sexp-tree obj))))
      (use-modules (srfi srfi-26))
      ;; XXX dynamic-wind stuff to close directories
      ;; and leaves.
      ;; XXX bindings to openat, or use chdir
      (define (activate-children! parent-fd parent-tree)
        (for-each (cute activate-child! parent-fd <>)
                  (assq-ref parent-tree 'children)))
      (define (activate-child! parent-fd child-tree)
        (let* ((name (assq-ref child-tree 'name))
               (child
                ;; XXX define
                (false-if-not-found
                 (openat parent-fd (fs-entry/tree-name child-tree)))))
          (if child ;; already exists
              (maybe-fixup-child! child child-tree)
              (create-child! parent-fd name child-tree))))
      (define (maybe-fixup-child! child child-tree)
        ;; First check if any changes need to be made.
        ;; If not, don't perform any write I/O.
        ;; XXX what happens if child is a symbolic link?
        ;; XXX handle (assq-ref child 'filler?)
        (let* ((stat (stat child))
               (child:bits (assq-ref child-tree 'bits))
               (child:uid (xxx (assq-ref child-tree 'uid)))
               (child:gid (xxx (assq-ref child-tree 'gid)))
               (bits-ok? (= (stat:perms child) child:bits))
               (owner-ok? (= (stat:uid child) child:uid))
               (group-ok? (= (stat:gid child) child:gid))
               (type-ok? (eq? (stat:type child)
                              (assq-ref child-tree 'type))))
          ;; XXX if programs hold open files to some files,
          ;; which aren't permitted by the new configuration,
          ;; then these programs ???
          ;; XXX log stuff perhaps
          (cond ((not type-ok?) (xxx-what-now))
                ;; Easy, no risk of accidentally creating
                ;; a setuid/setgid binary.
                ((and group-ok? owner-ok? (not bits-ok?))
                 (chmod child child:bits)
                 (activate-children! child child-tree))
                ;; XXX this relies on the Linux behaviour
                ;; of clearing setuid and setgid at chown
                ;; (in some cases), check the behaviour
                ;; on the Hurd and Linux
                ((not (and group-ok? owner-ok?))
                 ;; XXX check behaviour on symbolic links
                 (chown child child:uid child:gid)
                 (chmod child child:bits)
                 (activate-children! child child-tree))
                ;; Everything is OK!  Descend down the tree.
                ((and bits-ok? owner-ok? group-ok? type-ok?)
                 (activate-children! child child-tree))
                (else (XXX-I-missed-a-case)))))
      (define (create-child! parent-fd name child-tree)
        (case (assq-ref child-tree 'type)
          ((regular)
           ;; XXX default contents? Maybe allow including
           ;; a gexp #~(lambda (file-fd) do-stuff)
           ;; in the <fs-entry>?
           xxx-???-regular)
          ((directory)
           ;; XXX handle filler?
           ;; XXX check security implications of sticky-bit
           (mkdirat parent-fd name (assq-ref child-tree 'bits))
           (chown xxx-the-just-created-dir (assq-ref child-tree 'owner))
           (activate-chilren! xxx-the-just-created-dir child-tree))
          ;; XXX target?  Also, does any service actually require
          ;; this?
          ((symlink) xxx-???-symlink)
          (else ???)))
      (call-with-saved-umask
       (lambda ()
         ;; Prevent a race windows were newly-created directories
         ;; are temporarily world-executable where inappropriate.
         (umask #o777)
         (activate-children! root tree)))))

(define fs-entry-service-type
  (service-type (name 'fs-entries)
                (extensions
                 (list (service-extension activation-service-type
                                          fs-entry-activation)))
                (compose concatenate)
                (extend append)
                (description
                 "Create directory structures, with permission
bits, owner and groups (together called the security context),
without race conditions.  The value of this service is a list
of @code{fs-entry}.  The old security context is overwritten
at activation time, and some inconsistencies are detected at
build time.

If some parent directories of a @code{fs-entry} are not
explicitely specfied, it is required (at activation time)
they are root-owned (both user and group) and
world-unwritable.")))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]