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.

Sign up for GitHub

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

Jump to bottom

BUG: clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}' fails on an empty password #494

Open
jarkkojs opened this issue Oct 19, 2024 · 8 comments · May be fixed by #495
Open

BUG: clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}' fails on an empty password #494

jarkkojs opened this issue Oct 19, 2024 · 8 comments · May be fixed by #495

Comments

@jarkkojs
Copy link

jarkkojs commented Oct 19, 2024 •
edited
Loading

[originally posted here: https://social.kernel.org/notice/AnAEEyg3ULvyJzNq88]

Clevis has a bug that the following ends up failing unless the passphrase is non-empty:

sudo clevis luks bind -d /dev/nvme... tpm2 '{"pcr_ids":"1,4,5,7,9"}'

An empty passphrase can be created by the means of:

sudo cryptsetup luksChangeKey --force-password /dev/sda3

It is a totally legit configuration and scenario for my NUC7CJYH, which I use for only kernel testing. Threats are protected by the requirement of having physical presence checked.

Transcript:

$ sudo clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}'
Enter existing LUKS password:
Enter existing LUKS password:
Error adding new binding to /dev/sda3
@jarkkojs jarkkojs changed the title clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}' on an empty password BUG: clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}' fails on an empty password Oct 19, 2024
@jarkkojs
Copy link
Author

This is not right:

clevis_luks_check_valid_key_or_keyfile() {
    local DEV="${1}"
    local KEY="${2:-}"
    local KEYFILE="${3:-}"
    local SLT="${4:-}"
    local EXISTING_TOKEN_ID="${5:-}"

    [ -z "${DEV}" ] && return 1
    [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEYFILE}" ] && [ -z "${KEY}" ] && return 1

    local extra_args
    extra_args="$([ -n "${SLT}" ] && printf -- '--key-slot %s' "${SLT}")"
    if [ -n "${KEYFILE}" ]; then
        cryptsetup open --test-passphrase "${DEV}" --key-file "${KEYFILE}" \
                   ${extra_args}
        return
    fi
    if [ -n "${EXISTING_TOKEN_ID}" ]; then
        cryptsetup open --test-passphrase "${DEV}" --token-id "${EXISTING_TOKEN_ID}" \
                   ${extra_args}
        return
    fi

    printf '%s' "${KEY}" | cryptsetup open --test-passphrase "${DEV}" \
                                      ${extra_args}
}

There should be means to pass --force-password to cryptsetup open, and allow EXISTING_TOKEN_ID to be empty. I.e. a new option is needed for clevis luks bind equivalent to --force-password.

@sergio-correia
Copy link
Collaborator

What about something like this?

diff --git a/src/luks/clevis-luks-common-functions.in b/src/luks/clevis-luks-common-functions.in
index 29e4631..2b39316 100644
--- a/src/luks/clevis-luks-common-functions.in
+++ b/src/luks/clevis-luks-common-functions.in
@@ -334,10 +334,18 @@ clevis_luks_check_valid_key_or_keyfile() {
     local EXISTING_TOKEN_ID="${5:-}"
 
     [ -z "${DEV}" ] && return 1
-    [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEYFILE}" ] && [ -z "${KEY}" ] && return 1
 
     local extra_args
     extra_args="$([ -n "${SLT}" ] && printf -- '--key-slot %s' "${SLT}")"
+
+    # We have an empty key here.
+    if [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEYFILE}" ] \
+         && [ -z "${KEY}" ]; then
+        echo | cryptsetup open --force-password --test-passphrase "${DEV}" \
+               ${extra_args}
+        return
+    fi
+
     if [ -n "${KEYFILE}" ]; then
         cryptsetup open --test-passphrase "${DEV}" --key-file "${KEYFILE}" \
                    ${extra_args}
@@ -798,7 +806,6 @@ clevis_luks_add_key() {
 
     [ -z "${DEV}" ] && return 1
     [ -z "${NEWKEY}" ] && return 1
-    [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEY}" ] && [ -z "${KEYFILE}" ] && return 1
 
     local extra_args='' input
     input="$(printf '%s\n%s' "${KEY}" "${NEWKEY}")"

@jarkkojs
Copy link
Author

What about something like this?

diff --git a/src/luks/clevis-luks-common-functions.in b/src/luks/clevis-luks-common-functions.in
index 29e4631..2b39316 100644
--- a/src/luks/clevis-luks-common-functions.in
+++ b/src/luks/clevis-luks-common-functions.in
@@ -334,10 +334,18 @@ clevis_luks_check_valid_key_or_keyfile() {
     local EXISTING_TOKEN_ID="${5:-}"
 
     [ -z "${DEV}" ] && return 1
-    [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEYFILE}" ] && [ -z "${KEY}" ] && return 1
 
     local extra_args
     extra_args="$([ -n "${SLT}" ] && printf -- '--key-slot %s' "${SLT}")"
+
+    # We have an empty key here.
+    if [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEYFILE}" ] \
+         && [ -z "${KEY}" ]; then
+        echo | cryptsetup open --force-password --test-passphrase "${DEV}" \
+               ${extra_args}
+        return
+    fi
+
     if [ -n "${KEYFILE}" ]; then
         cryptsetup open --test-passphrase "${DEV}" --key-file "${KEYFILE}" \
                    ${extra_args}
@@ -798,7 +806,6 @@ clevis_luks_add_key() {
 
     [ -z "${DEV}" ] && return 1
     [ -z "${NEWKEY}" ] && return 1
-    [ -z "${EXISTING_TOKEN_ID}" ] && [ -z "${KEY}" ] && [ -z "${KEYFILE}" ] && return 1
 
     local extra_args='' input
     input="$(printf '%s\n%s' "${KEY}" "${NEWKEY}")"

I still get with this patch:

❯ sudo clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}'
Enter existing LUKS password:
Enter existing LUKS password:
Error adding new binding to /dev/sda3

I built a local version installed under ~/local prefix in order to test this in Fedora 40 Server Edition.

@sergio-correia
Copy link
Collaborator

I still get with this patch:

❯ sudo clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}'
Enter existing LUKS password:
Enter existing LUKS password:
Error adding new binding to /dev/sda3

Are you sure it is using the updated version? I get this exact output without the patch. With it, it should not show that Enter existing LUKS password twice.

@jarkkojs
Copy link
Author

I still get with this patch:

❯ sudo clevis luks bind -d /dev/sda3 tpm2 '{"pcr_ids":"1,4,5,7,9"}'
Enter existing LUKS password:
Enter existing LUKS password:
Error adding new binding to /dev/sda3

Are you sure it is using the updated version? I get this exact output without the patch. With it, it should not show that Enter existing LUKS password twice.

I'll sanity check this.

@jarkkojs
Copy link
Author

Sorry have been busy work with other not forgotten.

@jarkkojs
Copy link
Author

jarkkojs commented Oct 28, 2024 •
edited
Loading

It fixes the issue:

jarkko@mustatorvisieni:~$ sudo clevis luks list -d /dev/sda3
1: tpm2 '{"hash":"sha256","key":"ecc","pcr_bank":"sha1","pcr_ids":"1,4,5,7,9"}'
j

jarkko@mustatorvisieni:~$ sudo systemd-cryptenroll /dev/sda3 --tpm2-device=auto --tpm2-pcrs=1,4,5,7,
9
🔐 Please enter current passphrase for disk /dev/sda3:                         
New TPM2 token enrolled as key slot 2.

@sergio-correia sergio-correia linked a pull request Oct 28, 2024 that will close this issue
Open
@sergio-correia
Copy link
Collaborator

It fixes the issue:

Thanks for testing; I added a PR to address it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix support for LUKS device with an empty password sergio-correia/clevis
2 participants
@jarkkojs @sergio-correia