GNU bug report logs - #52882
[PATCH] gnu: system: Add crypt-key field for mapped filesystems

Previous Next

To reply to this bug, email your comments to 52882 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: chayleaf <chayleaf <at> pavluk.org>
To: guix-patches <at> gnu.org
Cc: chayleaf <chayleaf <at> pavluk.org>, chayleaf <chayleaf <at> protonmail.com>
Subject: [PATCH] gnu: system: Add crypt-key field for mapped filesystems
Date: Wed, 29 Dec 2021 21:57:13 +0000

From: chayleaf <chayleaf <at> protonmail.com>

This is a patch that adds a new field for mapped-filesystem that allows
one to specify the LUKS encryption key via G-Expressions.
An example use case is using a key stored on an external device.

Sorry if I made a mistake anywhere, I'm new to both Lisp and mailing
lists.

* gnu/system/mapped-devices.scm (mapped-device-kind):
Add crypt-key field.
(open-luks-device): Use crypt-key as the encryption key if it's
provided.
* gnu/system/linux-initrd.scm (raw-initrd)[device-mapping-commands]:
Utilize the crypt-key field.
* doc/guix.texi (Mapped Devices): Add crypt-key to mapped-device docs.

Signed-off-by: chayleaf <chayleaf <at> pavluk.org>
---
 doc/guix.texi                 |  7 ++++
 gnu/system/linux-initrd.scm   | 11 ++---
 gnu/system/mapped-devices.scm | 77 +++++++++++++++++++++++------------
 3 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index ebfcfee7f7..22495b0cbd 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15125,6 +15125,13 @@ there are several.  The format is identical to @var{target}.
 @item type
 This must be a @code{mapped-device-kind} object, which specifies how
 @var{source} is mapped to @var{target}.
+
+@item crypt-key
+A G-Expression (see @pxref{G-Expressions}) or a bytevector to be used as the
+encryption key for this device.  If none is specified, the user will be asked
+to enter their passphrase.  It can be used for fetching the key from an
+external device or avoiding to enter the passhprase two times with encrypted
+@code{/boot}.
 @end table
 @end deftp
 
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index c78dd09205..36700d91ae 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -203,11 +203,12 @@ (define* (raw-initrd file-systems
   (define device-mapping-commands
     ;; List of gexps to open the mapped devices.
     (map (lambda (md)
-           (let* ((source  (mapped-device-source md))
-                  (targets (mapped-device-targets md))
-                  (type    (mapped-device-type md))
-                  (open    (mapped-device-kind-open type)))
-             (open source targets)))
+           (let* ((source    (mapped-device-source md))
+                  (targets   (mapped-device-targets md))
+                  (type      (mapped-device-type md))
+                  (crypt-key (mapped-device-crypt-key md))
+                  (open      (mapped-device-kind-open type)))
+             (open source targets #:crypt-key crypt-key)))
          mapped-devices))
 
   (define file-system-scan-commands
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 96a381d5fe..4f680b71fe 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -50,6 +50,7 @@ (define-module (gnu system mapped-devices)
             mapped-device-target
             mapped-device-targets
             mapped-device-type
+            mapped-device-crypt-key
             mapped-device-location
 
             mapped-device-kind
@@ -80,6 +81,8 @@ (define-record-type* <mapped-device> %mapped-device
   (source    mapped-device-source)                ;string | list of strings
   (targets   mapped-device-targets)               ;list of strings
   (type      mapped-device-type)                  ;<mapped-device-kind>
+  (crypt-key mapped-device-crypt-key              ;bytevector | gexp
+             (default (const #f)))
   (location  mapped-device-location
              (default (current-source-location)) (innate)))
 
@@ -107,7 +110,7 @@ (define-deprecated (mapped-device-target md)
 (define-record-type* <mapped-device-type> mapped-device-kind
   make-mapped-device-kind
   mapped-device-kind?
-  (open      mapped-device-kind-open)             ;source target -> gexp
+  (open      mapped-device-kind-open)             ;source target #:key (crypt-key #f) -> gexp
   (close     mapped-device-kind-close             ;source target -> gexp
              (default (const #~(const #f))))
   (check     mapped-device-kind-check             ;source -> Boolean
@@ -188,7 +191,10 @@ (define missing
 ;;; Common device mappings.
 ;;;
 
-(define (open-luks-device source targets)
+(define* (open-luks-device source targets #:key
+                                  (crypt-key #f)
+                                  #:allow-other-keys
+                                  #:rest rest)
   "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
 'cryptsetup'."
   (with-imported-modules (source-module-closure
@@ -200,7 +206,9 @@ (define (open-luks-device source targets)
                              (uuid-bytevector source)
                              source)))
            ;; XXX: 'use-modules' should be at the top level.
-           (use-modules (rnrs bytevectors) ;bytevector?
+           (use-modules (ice-9 binary-ports) ;put-bytevector
+                        (ice-9 popen) ;open-pipe*
+                        (rnrs bytevectors) ;bytevector?
                         ((gnu build file-systems)
                          #:select (find-partition-by-luks-uuid))
                         ((guix build utils) #:select (mkdir-p)))
@@ -211,28 +219,37 @@ (define (open-luks-device source targets)
 
            ;; Use 'cryptsetup-static', not 'cryptsetup', to avoid pulling the
            ;; whole world inside the initrd (for when we're in an initrd).
-           (zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
-                           "open" "--type" "luks"
-
-                           ;; Note: We cannot use the "UUID=source" syntax here
-                           ;; because 'cryptsetup' implements it by searching the
-                           ;; udev-populated /dev/disk/by-id directory but udev may
-                           ;; be unavailable at the time we run this.
-                           (if (bytevector? source)
-                               (or (let loop ((tries-left 10))
-                                     (and (positive? tries-left)
-                                          (or (find-partition-by-luks-uuid source)
-                                              ;; If the underlying partition is
-                                              ;; not found, try again after
-                                              ;; waiting a second, up to ten
-                                              ;; times.  FIXME: This should be
-                                              ;; dealt with in a more robust way.
-                                              (begin (sleep 1)
-                                                     (loop (- tries-left 1))))))
-                                   (error "LUKS partition not found" source))
-                               source)
-
-                           #$target)))))))
+           (let ((crypt-key #$crypt-key)
+                 (cryptsetup-cmdline (list #$(file-append cryptsetup-static "/sbin/cryptsetup")
+                                           "open" "--type" "luks"
+
+                                           ;; Note: We cannot use the "UUID=source" syntax here
+                                           ;; because 'cryptsetup' implements it by searching the
+                                           ;; udev-populated /dev/disk/by-id directory but udev may
+                                           ;; be unavailable at the time we run this.
+                                           (if (bytevector? source)
+                                               (or (let loop ((tries-left 10))
+                                                     (and (positive? tries-left)
+                                                          (or (find-partition-by-luks-uuid source)
+                                                              ;; If the underlying partition is
+                                                              ;; not found, try again after
+                                                              ;; waiting a second, up to ten
+                                                              ;; times.  FIXME: This should be
+                                                              ;; dealt with in a more robust way.
+                                                              (begin (sleep 1)
+                                                                     (loop (- tries-left 1))))))
+                                                   (error "LUKS partition not found" source))
+                                               source)
+
+                                           #$target)))
+               (or (and (bytevector? crypt-key)
+                        (let ((port (apply open-pipe*
+                                           (cons OPEN_WRITE
+                                                 (append cryptsetup-cmdline
+                                                         (list "--key-file" "-"))))))
+                          (put-bytevector port crypt-key)
+                          (zero? (status:exit-val (close-pipe port)))))
+                   (zero? (apply system* cryptsetup-cmdline)))))))))
 
 (define (close-luks-device source targets)
   "Return a gexp that closes TARGET, a LUKS device."
@@ -271,7 +288,10 @@ (define luks-device-mapping
    (close close-luks-device)
    (check check-luks-device)))
 
-(define (open-raid-device sources targets)
+(define* (open-raid-device sources targets #:key
+                                   (crypt-key #f)
+                                   #:allow-other-keys
+                                   #:rest rest)
   "Return a gexp that assembles SOURCES (a list of devices) to the RAID device
 TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
   (match targets
@@ -312,7 +332,10 @@ (define raid-device-mapping
    (open open-raid-device)
    (close close-raid-device)))
 
-(define (open-lvm-device source targets)
+(define* (open-lvm-device source targets #:key
+                                 (crypt-key #f)
+                                 #:allow-other-keys
+                                 #:rest rest)
   #~(and
      (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
                      "vgchange" "--activate" "ay" #$source))
-- 
2.34.1

Message #8 received at 52882 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: chayleaf <chayleaf <at> pavluk.org>, 52882 <at> debbugs.gnu.org
Cc: chayleaf <chayleaf <at> pavluk.org>, chayleaf <chayleaf <at> protonmail.com>
Subject: Re: [bug#52882] [PATCH] gnu: system: Add crypt-key field for mapped
 filesystems
Date: Thu, 30 Dec 2021 11:57:19 +0100

Hello,

chayleaf <chayleaf <at> pavluk.org> writes:

> From: chayleaf <chayleaf <at> protonmail.com>
>
> This is a patch that adds a new field for mapped-filesystem that allows
> one to specify the LUKS encryption key via G-Expressions.
> An example use case is using a key stored on an external device.

This is a feature that many people have on their wishlist, and it looks
like your code would do precisely that, however I have to admit that I
am against adding this code into master for security reasons.

The open-luks-device gexp, along with the whole passphrase [1], end up
in the boot script in the store, and the guix store is r-xr-xr-x,
meaning that any program on your computer is able to read it.

This is a pretty significant security risk that can reduce the benefits
of full-disk encryption to nothing, so having it easily available to
users would work against them.  Feel free to use this patch on your
local installation though, if you understand the security risks :)

On other distros, you can simply have keyfiles and initrds root-owned
and r--------, and I think you could do something similar here, but
you'd have to keep them out of the store and load them separately.  This
could be a solution, but I don't know off the top of my head how one
could implement it.

[1] the actual encryption key is stored encrypted inside the LUKS
header, which is unlocked with a passphrase, roughly.

-- 
Josselin Poiret

Message #11 received at 52882 <at> debbugs.gnu.org (full text, mbox):

From: chayleaf <chayleaf <at> pavluk.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>, 52882 <at> debbugs.gnu.org
Subject: Re: [bug#52882] [PATCH] gnu: system: Add crypt-key field for mapped
 filesystems
Date: Fri, 31 Dec 2021 01:25:44 +0700

> The open-luks-device gexp, along with the whole passphrase [1], end
> up in the boot script in the store, and the guix store is r-xr-xr-x,
> meaning that any program on your computer is able to read it.

Wouldn't it be fine if the key is stored on an external device and the
user supplies a G-Expression that loads it?  Or is the G-Expression
executed at reconfigure as opposed to at boot?

Storing the key itself is indeed insecure.  However, I think the
ability to load the key from something other than user input could
become a building block for hardcoding the key in more secure ways. 
For example, as far as I can tell, Grub supports multiple initrd
images [1], if the user puts their key on the boot partition in the
cpio format and tells Grub to use it as a secondary initrd, perhaps it
could be done.

I do agree that at the very least the potential security issues
hardcoding the key can cause need to be documented.


> On other distros, you can simply have keyfiles and initrds root-owned
> and r--------, and I think you could do something similar here, but
> you'd have to keep them out of the store and load them separately. 
> This
> could be a solution, but I don't know off the top of my head how one
> could implement it.

The biggest problem is there need to be multiple generations available
at the same time.  While you could create a separate "private" only-
read-by-root initrd store for this purpose, that would be too much work
for just a single feature.  A possible compromise is maintaining a
single out-of-store initrd at a given time, or, combined with the
above, the "secret" initrd parts could be stored in a separate archive,
similar to how grub resides in its own directory outside of the store.

[1] https://www.gnu.org/software/grub/manual/grub/html_node/initrd.html

Message #14 received at 52882 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: chayleaf <chayleaf <at> pavluk.org>, 52882 <at> debbugs.gnu.org
Subject: Re: [bug#52882] [PATCH] gnu: system: Add crypt-key field for mapped
 filesystems
Date: Fri, 31 Dec 2021 18:58:28 +0100

Hello,

chayleaf <chayleaf <at> pavluk.org> writes:

> Wouldn't it be fine if the key is stored on an external device and the
> user supplies a G-Expression that loads it?  Or is the G-Expression
> executed at reconfigure as opposed to at boot?

It would indeed be fine.  The open-luks-device g-exp is executed at
boot, in early userspace (ie no root mounted yet, still in the
initramfs), by `build-system` in (gnu build linux-boot) as a member of
`pre-mount`.

> Storing the key itself is indeed insecure.  However, I think the
> ability to load the key from something other than user input could
> become a building block for hardcoding the key in more secure ways. 
> For example, as far as I can tell, Grub supports multiple initrd
> images [1], if the user puts their key on the boot partition in the
> cpio format and tells Grub to use it as a secondary initrd, perhaps it
> could be done.

Yes, this is what I was suggesting, although I don't really know how
Linux handles multiple initrds.  Is the resulting initramfs a union of
the different initrds?

> I do agree that at the very least the potential security issues
> hardcoding the key can cause need to be documented.

Agreed.

> The biggest problem is there need to be multiple generations available
> at the same time.  While you could create a separate "private" only-
> read-by-root initrd store for this purpose, that would be too much work
> for just a single feature.  A possible compromise is maintaining a
> single out-of-store initrd at a given time, or, combined with the
> above, the "secret" initrd parts could be stored in a separate archive,
> similar to how grub resides in its own directory outside of the store.

Out-of-store that's specified by the user seems like a good idea.  Do
you think you could handle adding additional initrd support to GRUB?  I
don't think it should be that hard.

Apart from that, the patch would be ok to merge for me if there was some
accompanying documentation that describes the security risks in a way
that would be understable for a layperson.

Best,
Josselin Poiret

Message #17 received at 52882 <at> debbugs.gnu.org (full text, mbox):

From: chayleaf <chayleaf <at> pavluk.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>, 52882 <at> debbugs.gnu.org
Subject: Re: [bug#52882] [PATCH] gnu: system: Add crypt-key field for mapped
 filesystems
Date: Tue, 04 Jan 2022 02:12:49 +0700

In advance, sorry if you received this message twice. The spam filters
seemed to reject this E-mail at first. No idea if it will go through
now, I'm still in the process of requesting a PTR entry.

> Yes, this is what I was suggesting, although I don't really know how
> Linux handles multiple initrds.  Is the resulting initramfs a union
> of the different initrds?

As far I know, the initrds are simply concatenated in-memory, and then
the kernel extracts all of the images to a tmpfs.

> Do you think you could handle adding additional initrd support to
> GRUB?  I don't think it should be that hard.

I could totally do that, I would really appreciate it if you told me
what the end-user interface should look like though.  Currently,
operating-system's initrd key is supposed to be a derivation, but in
case of initrds that might contain the user's encryption key it should
be a regular path.  One option would be to change "initrd" to "initrds"
(similar to mapped-device's "target" and "targets") and interpret
string? initrds as a path.  Another one is to add a new key in
bootloader-configuration.

A potential problem is that mounted paths and filesystem paths (I don't
know the exact terminology) may differ - consider, for example, a
separate /boot partition, or btrfs subvolumes.  If mounted paths are
used, it needs to be documented and the correct partition needs to be
mounted in GRUB, if filesystem paths are used, it once again needs to
be documented and the user needs to be able to specify not just the
path, but also the device the initrd resides on.

Also, I'm not sure all bootloaders support multiple initrds, and I
can't test the EFI bootloaders.  In particular, I couldn't find
anything that could let one use multiple initrds in U-Boot
documentation.  You can load multiple images at different addresses,
but I'm not sure whether that is enough to load multiple initrds. 
However, EXTLINUX documentation states "The initrd parameter supports
multiple filenames separated by commas".

Message #20 received at 52882 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: chayleaf <chayleaf <at> pavluk.org>
Cc: 52882 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 chayleaf <chayleaf <at> protonmail.com>
Subject: Re: bug#52882: [PATCH] gnu: system: Add crypt-key field for mapped
 filesystems
Date: Wed, 05 Jan 2022 22:20:37 +0100

Hello,

One comment about the interface (the security showstopper Josselin
described would need to be addressed first, though):

chayleaf <chayleaf <at> pavluk.org> skribis:

> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -50,6 +50,7 @@ (define-module (gnu system mapped-devices)
>              mapped-device-target
>              mapped-device-targets
>              mapped-device-type
> +            mapped-device-crypt-key
>              mapped-device-location
>  
>              mapped-device-kind
> @@ -80,6 +81,8 @@ (define-record-type* <mapped-device> %mapped-device
>    (source    mapped-device-source)                ;string | list of strings
>    (targets   mapped-device-targets)               ;list of strings
>    (type      mapped-device-type)                  ;<mapped-device-kind>
> +  (crypt-key mapped-device-crypt-key              ;bytevector | gexp
> +             (default (const #f)))
>    (location  mapped-device-location
>               (default (current-source-location)) (innate)))

The <mapped-device> type is used for mapped devices other than LUKS,
such as RAID devices.  Thus, there’s no reason for there to be a
‘crypt-key’ field.

Instead, the extra information should be passed in some other way,
either via the ‘source’ field, or maybe via an extra ‘arguments’ field
that would be passed as-is to the mapped-device type handler.

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.