#3490 PR merged: Garble LUKS password in disklayout.conf

Labels: bug, fixed / solved / done

jsmeix opened issue at 2025-07-04 08:26:

In layout/prepare/GNU/Linux/160_include_luks_code.sh
replace the LUKS password value by XXXXX
in disklayout.conf after it was copied into a file
so it is still visible that some LUKS password
was set during "rear recover" but its secret value
is not visible after "rear recover" in
/var/log/rear/recover/layout/disklayout.conf
on the recreated system

Additionally use mktemp $TMP_DIR/LUKS_password.XXXXXXXXXXXXXXX
instead of mktemp $TMPDIR/LUKS_password.XXXXXXXXXXXXXXX

jsmeix commented at 2025-07-04 08:32:

How this pull request was tested:

With the changes here I did the same as in
https://github.com/rear/rear/issues/3483#issuecomment-3023400410
but now:

RESCUE localhost:~ # rear -D recover
...

RESCUE localhost:~ # grep johannes /var/log/rear/rear-localhost.log
[no output]

RESCUE localhost:~ # grep johannes /var/lib/rear/layout/diskrestore.sh
[no output]

RESCUE localhost:~ # grep johannes /var/lib/rear/layout/disklayout.conf
[no output]

RESCUE localhost:~ # find / | grep LUKS_password
/var/tmp/rear.f6TITqJUE2FgVv8/tmp/LUKS_password.AjhhsSXzv1tX4pW
/var/tmp/rear.f6TITqJUE2FgVv8/tmp/LUKS_password.pJdPiqaPsdyihsg

RESCUE localhost:~ # ls -l /var/tmp/rear.f6TITqJUE2FgVv8/tmp/LUKS_password*
-rw------- 1 root root 13 Jul  4 09:55 /var/tmp/rear.f6TITqJUE2FgVv8/tmp/LUKS_password.AjhhsSXzv1tX4pW
-rw------- 1 root root 13 Jul  4 09:55 /var/tmp/rear.f6TITqJUE2FgVv8/tmp/LUKS_password.pJdPiqaPsdyihsg

But currently the LUKS password is still in
/var/lib/rear/layout/disklayout.conf.20250704095513.recover.787.orig
and
/mnt/local/var/log/rear/recover/layout/disklayout.conf.20250704095513.recover.787.orig

I will also fix that last remaining case
later when time permits (perhaps next week)
via this pull request.

jsmeix commented at 2025-07-04 12:11:

The current

sed -i -e "\|^crypt $target_device $source_device |s|password=.*|password=XXXXX|" "$LAYOUT_FILE"

is problematic because it assumes password=... is last.

That password=... is last is specified in
https://github.com/rear/rear/blob/master/doc/user-guide/06-layout-configuration.adoc#disk-layout-file-syntax
which reads (excerpts)

The syntax is of the form

keyword value1 value2 value3 ...

where keyword denotes one kind of component
(disk, partition, filesystem, ...​)
and keyword and all the values are separated
by single space characters
...
For most component keywords the values are positional parameters
...
For some component keywords the parameters have a form like

keyword value1 value2 optionA=valueA optionB=valueB ...

where the first ones are positional parameters
but not the option=value parameters
so empty option=value parameters are allowed

...

LUKS Devices

crypt /dev/mapper/<name> <device> [type=<type>] [cipher=<cipher>] [key_size=<key size>] [hash=<hash function>] [uuid=<uuid>] [pbkdf=<pbkdf algo> ] [keyfile=<keyfile>] [password=<password>]

so

... /dev/mapper/<name> <device>

are positional parameters
and for the option=value parameters
empty option=value parameters are allowed
BUT
because only "empty option=value parameters are allowed"
it means that existing option=value parameters must
appear in the ordering of the specified sequence
so when password=... exists it must be last.

Nevertheless when the robustness principle
"be liberal in what you accept"
can be implemented with reasonable effort
we should do it so it should also work
when password=... is not last.

With only the current commit
https://github.com/rear/rear/pull/3490/commits/c30503d509643a09c59b68aa23b35090fceab001
when password=... is not last it garbles too much because

# echo 'crypt this that foo password=mypassword bar' | sed -e "\|^crypt this that |s|password=.*|password=XXXXX|"

results

crypt this that foo password=XXXXX

instead of the intended result

crypt this that foo password=XXXXX bar

It can be made working with

# echo 'crypt this that foo password=mypassword bar' | sed -e "\|^crypt this that |s|password=[^[:space:]]*|password=XXXXX|"

crypt this that foo password=XXXXX bar

which also works when password=... is last

# echo 'crypt this that foo password=mypassword' | sed -e "\|^crypt this that |s|password=[^[:space:]]*|password=XXXXX|"

crypt this that foo password=XXXXX

The subsequent
https://github.com/rear/rear/pull/3490/commits/420edc454c75fed0ee79cba5aa445e1336bcdeaf
implements that.

jsmeix commented at 2025-07-07 14:07:

https://github.com/rear/rear/pull/3490/commits/2b3c2ee6dbd01bcdbc9a7789efc6473782fb191c
fixes the last remaining case that LUKS passwords
still appear in saved disklayout.conf files of the form
/var/log/rear/recover/layout/disklayout.conf.DATE.recover.PID.orig
also after "rear recover" in the recreated system.

jsmeix commented at 2025-07-07 14:39:

With latest commit all works well for me.

jsmeix commented at 2025-07-08 07:11:

I am now thinking about to make a generic function for the
"put a secret value into a temporary file"
functionality...

jsmeix commented at 2025-07-08 13:48:

https://github.com/rear/rear/pull/3490/commits/83e5ab1e59179f1ac7208cdd1d3529d42e221cbb
implements
https://github.com/rear/rear/pull/3490#issuecomment-3047647088
BUT
currently this new function is not yet used (i.e. called)
because the "how to use it" is work in progress because
e.g. when called as

password_file=$( store_value_in_tmp_file "$value" "LUKS password" )

then store_value_in_tmp_file() is run is a sub-shell
so its

AddExitTask "rm -f $tmp_file"

has no effect because

EXIT_TASKS=( "$*" "${EXIT_TASKS[@]}" )

adds "rm -f $tmp_file" to EXIT_TASKS in a sub-shell
so store_value_in_tmp_file would have to be called like

password_file=$( store_value_in_tmp_file "$value" "LUKS password" )
AddExitTask "rm -f $password_file"

which is not what I intended how to call it.

BUT
after thinking about it I saw that the root cause is in the
AddExitTask QuietAddExitTask RemoveExitTask functions
which currently simply cannot be called in a subshell
because changing the EXIT_TASKS array in a subshell
has no effect in the main shell where the EXIT_TASKS array
will be used to run the exit tasks, cf.
https://github.com/rear/rear/pull/3494

I cannot fix that in my store_value_in_tmp_file function.

So I decided to enhance my store_value_in_tmp_file function
with an optional third argument to specify
whether or not the temporary file should get
removed via 'AddExitTask "rm -f $tmp_file"'.
Currently I don't know if there is a use case
where that third argument could be used in practice.
I there is no use case where store_value_in_tmp_file()
is not called in a subshell, then support of that
third argument can be "just removed" because the
store_value_in_tmp_file function is no user interface
(i.e. this function is a ReaR internal thing).

jsmeix commented at 2025-07-09 15:48:

With the latest commits here
https://github.com/rear/rear/pull/3490/commits/8363a55826fcc3c82994aa2e4c901707430a7c74
and
https://github.com/rear/rear/pull/3490/commits/2cc2a54698a382b82eb1e64ddebaf339691c8c86
plus the commit
https://github.com/rear/rear/commit/5de9a4ace2ba5a79d4d6e8993ed61bc11ecfaba2
from
https://github.com/rear/rear/pull/3494
all works well (i.e. as intended) for me.

jsmeix commented at 2025-07-10 07:39:

I re-tested things again
(in particular "rear -D recover" versus "rear -e -D recover")
and all works well for me (i.e. things work as intended by me)
so
@rear/contributors
I would like to merge it tomorrow afternoon
unless there are objections.

[Export of Github issue for rear/rear.]