Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

systemd-cryptsetup is broken when used with tpm2 #167994

Closed
ymatsiuk opened this issue Apr 9, 2022 · 22 comments
Closed

systemd-cryptsetup is broken when used with tpm2 #167994

ymatsiuk opened this issue Apr 9, 2022 · 22 comments

Comments

@ymatsiuk
Copy link
Contributor

ymatsiuk commented Apr 9, 2022

Describe the bug

Plugins built with systemd are not available in cryptsetup

machine # [    5.839874] systemd-cryptsetup[455]: Token 0 unusable for segment 0 with desired keyslot priority 2.
machine # [    5.840623] systemd-cryptsetup[455]: Trying to load /nix/store/l6y9kks5z6ywsxivnld12hh326lbx2yc-cryptsetup-2.4.3/lib/cryptsetup/libcryptsetup-token-systemd-tpm2.so.
machine # [    5.841665] systemd-cryptsetup[455]: /nix/store/l6y9kks5z6ywsxivnld12hh326lbx2yc-cryptsetup-2.4.3/lib/cryptsetup/libcryptsetup-token-systemd-tpm2.so: cannot open shared object file: No such file or directory
machine # [    5.842962] systemd-cryptsetup[455]: No TPM2 metadata enrolled in LUKS2 header or TPM2 support not available, falling back to traditional unlocking.

Steps To Reproduce

Steps to reproduce the behavior:

Local with TPM2

systemd-cryptenroll --tpm2-device=list
dd if=/dev/zero of=encrypted.img bs=1 count=0 seek=1G
echo -n lukspass | cryptsetup luksFormat -q encrypted.img -

# confirm it works with cryptsetup and password
sudo cryptsetup luksOpen encrypted.img foo
# enter password: lukspass
sudo cryptsetup luksClose foo

# enroll tpm2
sudo PASSWORD=lukspass systemd-cryptenroll --tpm2-device=auto --tpm2-pcrs=7 encrypted.img
sudo ${pkgs.systemd}/lib/systemd/systemd-cryptsetup attach luks encrypted.img - tpm2-device=auto

Password prompt appears:

Set cipher aes, mode xts-plain64, key size 512 bits for device encrypted.img.
🔐 Please enter passphrase for disk foo: (press TAB for no echo)
#cleanup
sudo systemd-cryptenroll --wipe-slot=tpm2 encrypted.img

Using tests with swtpm

  1. enable debug in systemd boot.kernelParams = [ "systemd.log_level=debug" "systemd.log_target=console" "console=ttyS0,38400" "console=tty1" ];
  2. run the test nix build .#nixosTests.systemd-cryptenroll (give it a minute since it spits out lots of debug statement)
  3. inspect the log nix log .#nixosTests.systemd-cryptenroll

Expected behavior

Password prompt should not appear

Additional context

#139864 should've been caught this issue

strace suggests that the new systemd feature is to blame:

1933590 openat(AT_FDCWD, "/nix/store/l6y9kks5z6ywsxivnld12hh326lbx2yc-cryptsetup-2.4.3/lib/cryptsetup/libcryptsetup-token-systemd-tpm2.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

It seems like cryptsetup expects the library to be present under ${cryptsetup}/lib/cryptsetup but it's present under ${systemd}/lib/cryptsetup
No matter what I do I get into dependency loop 🤷🏻

Notify maintainers

@flokli @kloenk @Mic92

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

 - system: `"x86_64-linux"`
 - host os: `Linux 5.17.1, NixOS, 22.05 (Quokka)`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.8.0pre20220322_d5d4d98`
 - channels(ymatsiuk): `""`
 - channels(root): `"nixos"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
@ymatsiuk
Copy link
Contributor Author

Potential solution might be:

  • patch cryptsetup with fixed location for libraries (/lib/cryptsetup)
  • link ${systemd}/lib/cryptsetup to /lib/cryptsetup

Or maybe creating a module that links required libraries 🤔 + instantiate cryptsetup for external plugins support?

@flokli
Copy link
Contributor

flokli commented Apr 10, 2022

I think the libcryptsetup interface is to blame here. libcryptsetup-token-systemd-tpm2.so is shipped by systemd, and is using the

crypt_token_load_external(struct crypt_device *cd, const char *name, struct crypt_token_handler_internal *ret)

interface in cryptsetup/lib/luks2/luks2_token.c.

Instead of accepting a path to the .so file, it asks for a name, and assembles the path of the library to load on its own:

r = snprintf(buf, sizeof(buf), "%s/libcryptsetup-token-%s.so", crypt_token_external_path(), name);

crypt_token_external_path() returns EXTERNAL_LUKS2_TOKENS_PATH, which is a constant that can be set in cryptsetup configure phase.

On NixOS, this is a problem. Due to cryptsetup being a build dependency of a systemd with cryptsetup, so we can't bake in the path to systemd binaries. We could probably hardcode some imperative /run/cryptsetup-modules path which we populate, but that's messy, and might be problematic during initrd boot.

Ideally, the crypt_token_load_external interface would look different (or there would be a similar function), accepting a fully qualified path to the library to load. Another option would be a function to effectively alter the path crypt_token_load_external looks from, so systemd could call this with the dirname before loading the library.

@ymatsiuk
Copy link
Contributor Author

Before submitting any patches I decided to ask cryptsetup maintainers.

@arianvp
Copy link
Member

arianvp commented Apr 19, 2022

Wouldn't /run/current-system/lib be sufficient for this? As long as we also make sure it exists in early initrd? Doesn't sound impossible to me

@arianvp arianvp added this to To Do in systemd in Stage 1 via automation Apr 19, 2022
@flokli
Copy link
Contributor

flokli commented Apr 19, 2022

I'm not sure - loading random .so files from /run/current-system/lib could cause runtime breakages if not done very careful.

@dasJ
Copy link
Member

dasJ commented Apr 20, 2022

We (@Mic92, @luis-hebendanz) just had a discussion in Mumble about this. Our proposed solution would be something like this in cryptsetup (untested!):

diff --git a/lib/luks2/luks2_token.c b/lib/luks2/luks2_token.c
index 61be376..37befe8 100644
--- a/lib/luks2/luks2_token.c
+++ b/lib/luks2/luks2_token.c
@@ -151,12 +151,10 @@ crypt_token_load_external(struct crypt_device *cd, const char *name, struct cryp

        token = &ret->u.v2;

-       r = snprintf(buf, sizeof(buf), "%s/libcryptsetup-token-%s.so", crypt_token_external_path(), name);
+       r = snprintf(buf, sizeof(buf), "libcryptsetup-token-%s.so", name);
        if (r < 0 || (size_t)r >= sizeof(buf))
                return -EINVAL;

-       assert(*buf == '/');
-
        log_dbg(cd, "Trying to load %s.", buf);

        h = dlopen(buf, RTLD_LAZY);

This would allow us to load the libraries from the default library search path that we can alter using our default tools like patchelf.
This should not weaken security since LD_LIBRARY_PATH cannot be set on setuid binaries.

@flokli
Copy link
Contributor

flokli commented Apr 20, 2022

Sounds like a nice idea, but we should definitely ask upstream (in https://gitlab.com/cryptsetup/cryptsetup/-/issues/733) about this - they wrote:

Using absolute path is intentional for security reasons - this code can run under root, and it should not allow load plugins from a different path than is compiled in.

If I see this correctly, by dropping the absolute PATH we'd find all systemd cryptsetup external token .so files (as systemd's lib should end up in rpath).

@ymatsiuk
Copy link
Contributor Author

I might be completely off, but running patchelf inside of a cryptsetup derivation yields dependency loop, since we have to drag systemd in it.

@flokli
Copy link
Contributor

flokli commented Apr 21, 2022

No, the idea was to use make use of RUNPATH in ${systemd}/bin/… files. They include both ${cryptsetup}/lib and ${systemd}/lib, and affect dlopen behaviour inside libcryptsetup code (if loaded not with an absolute path, what this patch is proposing)

@dasJ
Copy link
Member

dasJ commented Apr 30, 2022

@flokli I really don't see the issue since overriding LD_LIBRARY_PATH only works when not using setuid
@ymatsiuk no, we don't have to patchelf cryptsetup, we have to patchelf systemd.

From the dlopen man page:

(ELF only) If the executable file for the calling program contains a DT_RPATH tag, and does not contain a DT_RUNPATH tag, then the directories listed in the DT_RPATH tag are searched.

If, at the time that the program was started, the environment variable LD_LIBRARY_PATH was defined to contain a colon-separated list of directories, then these are searched. (As a security measure this variable is ignored for set-user-ID and set-group-ID programs.)

(ELF only) If the executable file for the calling program contains a DT_RUNPATH tag, then the directories listed in that tag are searched.

@flokli
Copy link
Contributor

flokli commented May 2, 2022

@dasJ LD_LIBRARY_PATH and DT_R(UN)PATH are different things - the above proposal would rely on RUNPATH, rather than setting LD_LIBRARY_PATH - which is what we want here, and I'm happy to test a PR with it 🙂 Could you open one? 😉

@dasJ
Copy link
Member

dasJ commented May 2, 2022

With the aforementioned patch + this patch, the test succeeds:

diff --git a/nixos/tests/systemd-cryptenroll.nix b/nixos/tests/systemd-cryptenroll.nix
index 055ae7d1681..2204d7e1bc4 100644
--- a/nixos/tests/systemd-cryptenroll.nix
+++ b/nixos/tests/systemd-cryptenroll.nix
@@ -1,11 +1,17 @@
-import ./make-test-python.nix ({ pkgs, ... }: {
+import ./make-test-python.nix ({ pkgs, ... }: let
+
+  cryptsetup = pkgs.cryptsetup.overrideAttrs (oA: {
+    patches = oA.patches ++ [ ../../pkgs/os-specific/linux/cryptsetup/relative-token-path.patch ];
+  });
+
+in {
   name = "systemd-cryptenroll";
   meta = with pkgs.lib.maintainers; {
     maintainers = [ ymatsiuk ];
   };
 
   nodes.machine = { pkgs, lib, ... }: {
-    environment.systemPackages = [ pkgs.cryptsetup ];
+    environment.systemPackages = [ cryptsetup ];
     virtualisation = {
       emptyDiskImages = [ 512 ];
       qemu.options = [
@@ -14,6 +20,22 @@ import ./make-test-python.nix ({ pkgs, ... }: {
         "-device tpm-tis,tpmdev=tpm0"
       ];
     };
+
+    systemd.package = (pkgs.systemd.overrideAttrs (oA: {
+      nativeBuildInputs = oA.nativeBuildInputs ++ [
+        pkgs.makeBinaryWrapper
+      ];
+      postFixup = ''
+        ${oA.postFixup or ""}
+
+        for f in lib/systemd/systemd-cryptsetup bin/systemd-cryptenroll; do
+          wrapProgram $out/$f --prefix LD_LIBRARY_PATH : ${placeholder "out"}/lib/cryptsetup
+        done
+      '';
+    })).override {
+      inherit cryptsetup;
+    };
   };
 
   testScript = ''
@@ -37,7 +59,7 @@ import ./make-test-python.nix ({ pkgs, ... }: {
         # Enroll new LUKS key and bind it to Secure Boot state
         # For more details on PASSWORD variable, check the following issue:
         # https://github.com/systemd/systemd/issues/20955
-        machine.succeed("PASSWORD=lukspass systemd-cryptenroll --tpm2-device=auto --tpm2-pcrs=7 /dev/vdb")
+        machine.succeed("PASSWORD=lukspass SYSTEMD_LOG_LEVEL=debug SYSTEMD_LOG_LOCATION=1 LD_DEBUG=files systemd-cryptenroll --tpm2-device=auto --tpm2-pcrs=7 /dev/vdb")
         # Add LUKS partition to /etc/crypttab to test auto unlock
         machine.succeed("echo 'luks /dev/vdb - tpm2-device=auto' >> /etc/crypttab")
         machine.shutdown()

dasJ added a commit to helsinki-systems/nixpkgs that referenced this issue May 2, 2022
@dasJ
Copy link
Member

dasJ commented May 2, 2022

Fix: #171242

@dasJ dasJ moved this from To Do to In progress in systemd in Stage 1 May 3, 2022
@lovesegfault lovesegfault moved this from In progress to To Do in systemd in Stage 1 May 5, 2022
@xaverdh
Copy link
Contributor

xaverdh commented Oct 16, 2022

can this be closed with #189676 merged?

@flokli
Copy link
Contributor

flokli commented Oct 16, 2022

Yes, thanks for the ping :-)

@flokli flokli closed this as completed Oct 16, 2022
systemd in Stage 1 automation moved this from To Do to Done Oct 16, 2022
@flokli
Copy link
Contributor

flokli commented Jan 25, 2023

Hmh, I nixosTests.systemd-cryptenroll still is marked as broken, times out and refers to this issue. I think we need to reopen.

@flokli flokli reopened this Jan 25, 2023
systemd in Stage 1 automation moved this from Done to In progress Jan 25, 2023
@flokli
Copy link
Contributor

flokli commented Jan 25, 2023

I checked logs, swtpm seems to fail with the following message:

SWTPM_NVRAM_StoreData: Error (fatal) opening /build/tmppwhki02q/TMP2-00.permall for write failed, No such file or directory

Confusingly, it says that even if I touch a file in there, so maybe we invoke it wrongly - or conventions change midway?

cc @ymatsiuk @blitz

@ymatsiuk
Copy link
Contributor Author

ymatsiuk commented Jan 26, 2023

I'm going to check the test now, but what I noticed some time ago, that systemd-cryptsetup broke again:

[root@nixps:~]# systemd-cryptenroll --tpm2-device=list
PATH        DEVICE     DRIVER
/dev/tpmrm0 STM0125:00 tpm_tis

[root@nixps:~]# systemd-cryptenroll --tpm2-device=auto /dev/disk/by-uuid/2f7823b9-9e81-4813-8721-55e5000f2c7f
TPM2 driver name 'device' not valid, refusing.

it seems like this has stopped working

Edit: the test is failing due to the same issue ☝🏻

vm-test-run-systemd-cryptenroll> machine: must succeed: PASSWORD=lukspass systemd-cryptenroll --tpm2-device=auto --tpm2-pcrs=7 /dev/vdb
vm-test-run-systemd-cryptenroll> machine # TPM2 driver name 'device' not valid, refusing.
SWTPM_NVRAM_StoreData: Error (fatal) opening /build/tmppwhki02q/TMP2-00.permall for write failed, No such file or directory

☝🏻 looks like a race condition to me, might that be that the state directory disappeared before the swtpm process dies?
I ran a simple test hardcoding the swtpm state directory and was not able to reproduce this error anymore.

@blitz
Copy link
Contributor

blitz commented Jan 26, 2023

Ping @RaitoBezarius

@ElvishJerricco
Copy link
Contributor

TPM2 driver name 'device' not valid, refusing.

That was this bug: #210896

@ElvishJerricco
Copy link
Contributor

Yea, and upon cherrypicking that commit on nixos-unstable, nixosTests.systemd-cryptenroll does in fact succeed for me.

@flokli
Copy link
Contributor

flokli commented Jan 28, 2023

I can confirm. I sent a PR to staging (#213182), to mark the test as not broken there. This can be closed, sorry for the noise.

@flokli flokli closed this as completed Jan 28, 2023
systemd in Stage 1 automation moved this from In progress to Done Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants