Bug#901795: cryptsetup: new version may break 3rd party keyscripts (and thus boot)

[Christoph Anton Mitterer]

Source: cryptsetup
Version: 2:2.0.3-2
Severity: important


Hi.

Fritst thanks for work you've done in the recent new versions. Sooo many
nice things have been implemented/fixed :-)

Unfortunately, it breaks booting with my personal openpgp keyscripts.

The problem seems that in earlier versions, the initramfs got this file:
main/conf/conf.d/cryptroot with:
target=system,source=/dev/disk/by-uuid/97d2d814-72f6-11e8-a274-742b62897688,rootdev,keyscript=/lib/cryptsetup/scripts/decrypt_openpgp,tries=0,key=device=/dev/disk/by-label/keyFilePart:pathname=/etc/dm-crypt/keys/keyfile_for_system

As you can see, I use the 3rd field of the crypttab, for giving addtional
options to the keyscript:
device=/dev/disk/by-label/keyFilePart => the device on which the keyfile is to be found at boot
pathname=/etc/dm-crypt/keys/keyfile_for_sysstem => the name of the keyfile on the rootfs of that device

This file is gone, but now there is
main/cryptroot/crypttab with:
system UUID=31a2a126-2947-47ad-a87e-f5b9cb0b6c8a device=/dev/disk/by-label/gss-boot-data_ec713fc2-901a-4f51-8ffe-b9f4df02537b:pathname=/etc/dm-crypt/keys/heisenberg.scientia.net_system loud,luks,keyscript=decrypt_openpgp,tries=0


1) Such a file/format change should go to the NEWS file ;-)
This is one of the main reasons I reported #826122 back then
to get that "interface" stable for 3rd party users
2) I assume main/cryptroot/crypttab can have multiple lines, right?
How can I find out in my keyscript, which one is the right line
for it right now (i.e. for the device the keyscript currently
tries to open)?
3) Is there any documentaion of the (stable) format of main/cryptroot/crypttab?
Cause it doesn't seem to be the same than the normal /etc/crypttab


Thanks,
Chris.

[Guilhem Moulin]

Control: tag -1 moreinfo

Hi Christoph,

On Mon, 18 Jun 2018 at 15:06:59 +0200, Christoph Anton Mitterer wrote:
> Fritst thanks for work you've done in the recent new versions. Sooo many
> nice things have been implemented/fixed :-)

:-)

> The problem seems that in earlier versions, the initramfs got this file:
> main/conf/conf.d/cryptroot with:
> target=system,source=/dev/disk/by-uuid/97d2d814-72f6-11e8-a274-742b62897688,rootdev,keyscript=/lib/cryptsetup/scripts/decrypt_openpgp,tries=0,key=device=/dev/disk/by-label/keyFilePart:pathname=/etc/dm-crypt/keys/keyfile_for_system

> […]

> 1) Such a file/format change should go to the NEWS file ;-)

I disagree, the location of this file and its format are internal
(undocumented) implementation details, so third-party keyscripts
shouldn't rely on this. Please use the interface documented in
crypttab(5) to determine which device your keyscript is processing.
You should find the following in the keyscript's environment:

CRYPTTAB_NAME=system
CRYPTTAB_SOURCE=/path/to/source/device
CRYPTTAB_KEY=device=/dev/disk/by-label/keyFilePart:pathname=/etc/dm-crypt/keys/heisenberg.scientia.net_system
CRYPTTAB_OPTION_loud=yes
CRYPTTAB_OPTION_luks=yes
CRYPTTAB_OPTION_keyscript=/lib/cryptsetup/scripts/decrypt_openpgp
CRYPTTAB_OPTION_tries=0

IMHO this bug should either be closed (not a bug) or, if there is a need
to improve the documentation, its severity lowered to wishlist, and its
title changed accordingly. It's not a regression in either case; it's
in no way ‘important’ since you were the one shooting yourself in the
foot by relying on undocumented behavior :-P

Cheers,
--
Guilhem.

[Christoph Anton Mitterer]

Hey :-)

Sorry, I completely messed up my bug report ^^...

Actually, in the keyscript I already use CRYPTTAB_* ... it's the
initramfs hook which fails.

During the initramfs hook, I obviously won't have any of CRYPTTAB_*, or
do I?
But I do have the main/conf/conf.d/cryptroot (respectively now it's
main/cryptroot/crypttab - within the initramfs image).


So why do I need stuff from crypttab during initramfs generation?

Well the first thing is, I do some sanity checking already in the
initramfs hook, e.g. if there is no pathname= option my keyscript would
never be able to succeed, thus I already warn during initramfs
generation, that this initramfs will fail to boot.

Second thing is, in addition to device=, I alternatively offer an
option to simply include the key inside the initramfs, which is however
quite questionable from a security PoV,... so the hook script needs to
know that option and only if that is set, include the key (which is
taken again from pathname=).
Further, only if the key is read from some device, and not contained in
the initramfs image,... my hook scripts includes passdev (which I use
for reading) in the initramfs, otherwise it's not needed.
Last but not least if both, device= and "key_file_in_initramfs_images"
options are set, the initramfs hook script gives a warning, that one
will be ignored.



So in order to make the initramfs generation a bit more powerful, I
need the options for the device currently being processed.

Cheers,
Chris.

[Guilhem Moulin]

Control: severity -1 wishlist
Control: tag -1 - moreinfo
Control: retitle -1 cryptsetup-initramfs: please provide documented shell functions to validate/sanitize cryptroot entries in 3rd party hook files

On Mon, 18 Jun 2018 at 23:54:09 +0200, Christoph Anton Mitterer wrote:
> So why do I need stuff from crypttab during initramfs generation?

> […]

I see, thanks for the explanation, sounds like a valid use case indeed.

Thus lowering the bug severity to ‘wishlist’ and retiling the bug
accordingly.

FWIW, what we're currently doing (so far undocumented and subject to
change) is to go through the system crypttab(5) and copy each entry
requiring unlocking at initramfs stage to $DESTDIR/cryptroot/crypttab
(the format of which is therefore analogous to crypttab(5)). So the
following template should work when the hook file has ‘cryptroot’ as
prerequisite:

. /lib/cryptsetup/functions

[ -s "$DESTDIR/cryptroot/crypttab" ] || return 0
while read CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY CRYPTTAB_OPTIONS; do
if [ "${CRYPTTAB_NAME#\#}" = "$CRYPTTAB_NAME" ] && \
crypttab_parse_options "$CRYPTTAB_OPTIONS" n; then
[…]
fi
done <"$DESTDIR/cryptroot/crypttab"

But please note that this is subject to change until we document the
snippet and close this bug :-P

Cheers,
--
Guilhem.

[Christoph Anton Mitterer]

On Tue, 2018-06-19 at 00:26 +0200, Guilhem Moulin wrote:
> Thus lowering the bug severity to ‘wishlist’ and retiling the bug
> accordingly.

Well... it still broke some existing setups... it was always advertised
that people could make their own keyscripts and they simply used what
seemed usable and provided by cryptsetup ;-)

Guess that's why you saw quite a number of reports of people that saw
their stuff breaking... but anyway I don't really care which severity a
bug has, as long as there's going to be a nice solution :-)

> FWIW, what we're currently doing (so far undocumented and subject to
> change) is to go through the system crypttab(5) and copy each entry
> requiring unlocking at initramfs stage to $DESTDIR/cryptroot/crypttab
> (the format of which is therefore analogous to crypttab(5)). So the
> following template should work when the hook file has ‘cryptroot’ as
> prerequisite:
>
> . /lib/cryptsetup/functions
>
> [ -s "$DESTDIR/cryptroot/crypttab" ] || return 0
> while read CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY
> CRYPTTAB_OPTIONS; do
> if [ "${CRYPTTAB_NAME#\#}" = "$CRYPTTAB_NAME" ] && \
> crypttab_parse_options "$CRYPTTAB_OPTIONS" n; then
> […]
> fi
> done <"$DESTDIR/cryptroot/crypttab"

Will that process only those crypttab entries which actually require to
be processed during initramfs?

> But please note that this is subject to change until we document the
> snippet and close this bug :-P

Any idea already when this "API" is going to be stabilised? I'd prefer
to only adapt my stuff (&upgrade) once this is done. :D



One off topic things...
Why the whole split into cryptsetup-initramfs? Seems a bit overkill and
cosmetically less nice to me.

Wouldn't it have been the best if the cryptsetup initramfs hooks simply
only add stuff to the initramfs, if this was required for booting (or
didn't they do this already?), without any need to "configure/enable"
this by installing a package.
Right now this seems a bit error prone and kinda abusing the package
management for what should be rather configuration.

And if someone would have wanted to disable cryptsetup's initramfs-
tools integration, one could have simply provided a config file which
is sourced by the hooks and then exit if desired (or even better,
initramfs-tools should provide some masking mechanism,... like systemd
does when one adds symlinks to /dev/null in /etc/... with the same name
as the unit file shipped by the package in /usr/...


Cheers,
Chris.

[Guilhem Moulin]

On Mon, 25 Jun 2018 at 05:19:48 +0200, Christoph Anton Mitterer wrote:
> On Tue, 2018-06-19 at 00:26 +0200, Guilhem Moulin wrote:
>> Thus lowering the bug severity to ‘wishlist’ and retiling the bug
>> accordingly.
>
> Well... it still broke some existing setups... it was always advertised
> that people could make their own keyscripts and they simply used what
> seemed usable and provided by cryptsetup ;-)

Sure, people are welcome to write their own keyscripts. But rather than
using an undocumented interface and assume it'll never break, the right
thing to do is to ask us to document said interface and commit to
maintain it.

> Guess that's why you saw quite a number of reports of people that saw
> their stuff breaking...

Uh, are you referring to the regressions that were filed last week? You
can't compare your own #901795 with these regressions, where we broke
setups following the documented interface…

Beside your bug, the only similar issue we heard of was reported to the
list by Marc Haber, who wrote he was “aware that [he's] using an
internal interface, hence not a bug report but this message”.

That being said, 2 reports doesn't mean there are only 2 users affected.
But I think it's fair to assume that the very vast majority of users
were not using our internal interface.

> Will that process only those crypttab entries which actually require to
> be processed during initramfs?

Yes.

>> But please note that this is subject to change until we document the
>> snippet and close this bug :-P
>
> Any idea already when this "API" is going to be stabilised?

Right now we'd like things to settle a bit, and fixing actual regression
have higher priority. I'll plan to start working on this once the
package enters testing, but I'm not promising anything.

> Wouldn't it have been the best if the cryptsetup initramfs hooks simply
> only add stuff to the initramfs, if this was required for booting (or
> didn't they do this already?), without any need to "configure/enable"
> this by installing a package.
> Right now this seems a bit error prone and kinda abusing the package
> management for what should be rather configuration.

See the changelog entry for 2:2.0.3-1, and the 2 merged bug it closed.

> And if someone would have wanted to disable cryptsetup's initramfs-
> tools integration, one could have simply provided a config file which
> is sourced by the hooks and then exit if desired (or even better,
> initramfs-tools should provide some masking mechanism,... like systemd
> does when one adds symlinks to /dev/null in /etc/... with the same name
> as the unit file shipped by the package in /usr/...

See /etc/cryptsetup-initramfs/conf-hook. We're deprecating
CRYPTSETUP=[y|n] now that the we split the initramfs integration in its
own package, though.

--
Guilhem.

[Christoph Anton Mitterer]

Hey. :-)

On Mon, 2018-06-25 at 14:52 +0200, Guilhem Moulin wrote:
> But rather
> than
> using an undocumented interface and assume it'll never break, the
> right
> thing to do is to ask us to document said interface and commit to
> maintain it.

Well, back then when I "developed" my gnupg keyscript there was one in
cryptsetup which, IIRC, was even dysfunctional.
I tried to get Jonas mine included, but he didn't like it, though I
consider(ed) it pretty sophisticated (but I haven't looked at the
current decrypt_gnupg,... so maybe this is now fine, too).

So I kinda had "upstream contact" but things just got lost over time
;-)

If you like I can send you the full set of scripts&hooks for review.

> Uh, are you referring to the regressions that were filed last
> week? You
> can't compare your own #901795 with these regressions, where we broke
> setups following the documented interface…

There was no real criticism in my words :-) I just wanted to point out
that these things simply happen... people use such stuff if it's
visible ;-)
No blame on you guys!

> > Will that process only those crypttab entries which actually
> > require to
> > be processed during initramfs?
>
> Yes.

Nice :-)

> Right now we'd like things to settle a bit, and fixing actual
> regression
> have higher priority. I'll plan to start working on this once the
> package enters testing, but I'm not promising anything.

I'd now simply start to use the "interface" you suggested me in your
last mail. Question on that:

> . /lib/cryptsetup/functions
>
> [ -s "$DESTDIR/cryptroot/crypttab" ] || return 0

Why is this necessary? I assume when I PREREQ=cryptroot, than
$DESTDIR/cryptroot/crypttab finished(!) and contains all devices needed
to be unlocked during initramfs, right?
And I'd guess this is for the case that there are no such devices and
thus no "$DESTDIR/cryptroot/crypttab" would be even written (and my
hook wouldn't need to do anything either). Right?

> while read CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY CRYPTTAB_OPTIONS; do

Do you guys do any quoting in "$DESTDIR/cryptroot/crypttab"?
Cause read without -r will interpret \ as quoting character... and this
is IMO always a bit dangerous if the same is then used...

> if [ "${CRYPTTAB_NAME#\#}" = "$CRYPTTAB_NAME" ] \
What is this intended for?
(Oh and did you guys notice that this is a bashism? ${var#word} is not POSIX sh compatbile)

> && crypttab_parse_options "$CRYPTTAB_OPTIONS" n; then

I'd assume I don't need this, if I don't use the options like SIZE,
CIPHER and such in my hookscript, right? I basically use only what's in
the 3rd field of crypttab, and that should correspond to CRYPTTAB_KEY
here?!
When I don't actually need crypttab_parse_options, I also shouldn't
need to source /lib/cryptsetup/functions above.


> […]
> fi
> done <"$DESTDIR/cryptroot/crypttab"



So apart from not understanding yet what the "${CRYPTTAB_NAME#\#}" =
"$CRYPTTAB_NAME" is meant for, I'd say it's enough for my initramfs
hook to do:
> while read -r CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY
CRYPTTAB_OPTIONS; do
> ...

assuming each line of "$DESTDIR/cryptroot/crypttab" will continue to
adhere to this format.

> > Wouldn't it have been the best if the cryptsetup initramfs hooks
> > simply
> > only add stuff to the initramfs, if this was required for booting
> > (or
> > didn't they do this already?), without any need to
> > "configure/enable"
> > this by installing a package.
> > Right now this seems a bit error prone and kinda abusing the
> > package
> > management for what should be rather configuration.
>
> See the changelog entry for 2:2.0.3-1, and the 2 merged bug it
> closed.

Hmm... I've looked over it, and it seems the only reason was to be able
to depend on busybox (only if cryptsetup within initramfs is used)?
Cause the other mess Michael Biebl described seems to be rather either
other bugs or somehow solvable at a first glance.

Well in the end I personally still consider such "configuring" packages
rather ugly.
One could have made a warning if initramfs-tools.conf says BUSYBOX=N
but cryptsetup needs to enable them nevertheless.
Or vice versa let cryptsetup not forcibly enable it, and give a
warning/error during initramfs creation if it's not there.

> > And if someone would have wanted to disable cryptsetup's initramfs-
> > tools integration, one could have simply provided a config file
> > which
> > is sourced by the hooks and then exit if desired (or even better,
> > initramfs-tools should provide some masking mechanism,... like
> > systemd
> > does when one adds symlinks to /dev/null in /etc/... with the same
> > name
> > as the unit file shipped by the package in /usr/...
>
> See /etc/cryptsetup-initramfs/conf-hook. We're deprecating
> CRYPTSETUP=[y|n] now that the we split the initramfs integration in
> its
> own package, though.

Well but that boils just down to my point:
The package management system shouldn't be therefore configuration -
config files should.

Anyway,... just cosmetic thoughts from my personal PoV... I'm happy as
it is and with the great efforts you guys do for the benefit of all of
us!


Cheers & thanks,
Chris.

[Guilhem Moulin]

On Tue, 26 Jun 2018 at 03:28:17 +0200, Christoph Anton Mitterer wrote:
> If you like I can send you the full set of scripts&hooks for review.

Just open a wishlist bugs and everybody will be able to look at it? :-)

>> Right now we'd like things to settle a bit, and fixing actual
>> regression
>> have higher priority. I'll plan to start working on this once the
>> package enters testing, but I'm not promising anything.
>
> I'd now simply start to use the "interface" you suggested me in your
> last mail.

I'll repeat it here: it's still subject to change! This thread is not
meant to document the interface, but to understand what your needs are.

> Question on that:
>
>> . /lib/cryptsetup/functions
>>
>> [ -s "$DESTDIR/cryptroot/crypttab" ] || return 0
>
> Why is this necessary? I assume when I PREREQ=cryptroot, than
> $DESTDIR/cryptroot/crypttab finished(!) and contains all devices needed
> to be unlocked during initramfs, right?

No, it's for the case where cryptsetup's initramfs integration is not
processed (because the package isn't installed, because /etc/crypttab is
empty or nonexistent, or because /etc/cryptsetup-initramfs/conf-hook
specifies CRYPTSETUP=n).

>> while read CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_KEY CRYPTTAB_OPTIONS; do
> Do you guys do any quoting in "$DESTDIR/cryptroot/crypttab"?
> Cause read without -r will interpret \ as quoting character... and this
> is IMO always a bit dangerous if the same is then used...

Hmm good point, you can have spaces and tabs (and options values
containing ‘,’) by prefixing them with ‘/’ in your /etc/crypttab, but
lines are unquoted when the hook reads /etc/crypttab, and
$DESTDIR/cryptroot/crypttab is quote-free so special characters are
lost. (Note that the handling of special characters in /etc/crypttab
was not documented — thus not supported — before 2:2.0.3-2.)

>> if [ "${CRYPTTAB_NAME#\#}" = "$CRYPTTAB_NAME" ] \
> What is this intended for?

It removes comments (not necessary currently as the hook removes them
already).

> (Oh and did you guys notice that this is a bashism? ${var#word} is not
> POSIX sh compatbile)

It very much is, see http://pubs.opengroup.org/onlinepubs/9699919799/
sec. 2.6.2 “Parameter Expansion”. Anyway we're not targeting POSIX
shell but dash (which has a handful of features not in POSIX shell) for
the hook files, and busybox's ash (which is a superset of dash) in the
scripts.

--
Guilhem.

[Guilhem Moulin]

Hi,

In the upcoming 2:2.0.3-5 I refactored the crypttab(5) parsing logic [0].
Would the following interface suit your needs?

crypttab_find_entry([--quiet], $target)

Search the crypttab(5) for the given $target and set
CRYPTTAB_NAME, CRYPTTAB_SOURCE, CRYPTTAB_KEY, and
CRYPTTAB_OPTIONS accordingly. (These variables are not exported
to the environment.) If there are duplicates target names then
only the first one is considered. Return 0 if a match is found,
and 1 otherwise.

crypttab_foreach_entry($callback)

Iterate through the crypttab(5) and run the given $callback for
each entry found. The entry currently being processed is
refered to by the values of CRYPTTAB_{NAME,SOURCE,KEY,OPTIONS}.
(These variables are not exported to the environment.)
Note: $callback's return value doesn't affect the loop
currently, but if breaking out is desired it shouldn't be hard
to add.

crypttab_parse_options([--export], [--quiet])

Parse the options of a crypttab(5) mapping, defined by values of
variables CRYPTTAB_{NAME,SOURCE,KEY,OPTIONS}, and set variables
variables CRYPTTAB_OPTION_<option>=<value> accordingly. These
variables are exported to the environment if --export is set.
Return 1 on parsing error, 0 otherwise (incl. if unknown options
were encountered).

The crypttab(5) to use is suitably chosen depending on the context: main
system, initramfs hook scripts, or initramfs boot scripts.

See the `cryptgnupg` hook script for an example of this interface:

https://salsa.debian.org/cryptsetup-team/cryptsetup/blob/master/debian/initramfs/hooks/cryptgnupg

I should also point out that the value of CRYPTTAB_OPTIONS is not
reliable if there are options with values containing ‘,’ characters.
So to get the value of a particular <option> one shouldn't parse
$CRYPTTAB_OPTIONS, but rather use $CRYPTTAB_OPTION_<option>. For
instance if the crypttab(5) line being processed is

target /dev/source none luks,header=/my/header\054swap

(minus the 4 leading spaces) then after parsing options one gets

CRYPTTAB_NAME="target"
CRYPTTAB_SOURCE="/dev/source"
CRYPTTAB_KEY="none"
CRYPTTAB_OPTIONS="luks,header=/my/header,swap"
CRYPTTAB_OPTION_luks="yes"
CRYPTTAB_OPTION_header="/my/header,swap"

--
Guilhem.

[0] https://salsa.debian.org/cryptsetup-team/cryptsetup/commit/cb5985935713deb6bd4fd45c77d1f54cc28b204b

[Christoph Anton Mitterer]

Hey Guilhem.


Looks awesome at a first glance,.. I'll have a more thorough look at it
the next days...

One thing however comes just to my mind, i.e. another use case (that my
hook script would already do):

In my crypttab 3rd field, I allow some option to be included like
"include_key_in_initramfs".
That's basically a mutually exclusive alternative to the
device=...:path=... option where the keyscript loads+gpg-decrypts the
key from that device in that relative path.

As the name implies, it includes the gpg-encrypted key in the initramfs
images at their creation time (taking *just* path=... as its location
in the filesystem).
(For security reasons this is what I'd not suggest, though.)


Now when my initramfs hook copies that to the initramfs, it needs to
store it in some place where it can be later found by the keyscript.
Currently, the path is hardcoded in my hook script (only).
Until the big changes in the recent upgrades to cryptsetup, the hook
then modified the:
.../conf/conf.d/cryptroot
file to contain path=<pathWithinTheInitramfs> in the 3rd field (instead
of the original path=<pathWithinTheNormalFilesystem>.


So the use case is, that people may also wish to modify parts of the
.../cryptroot/crypttab file from their hook scripts... and that it
could be nice to have an interface for that as well.

What do you think? :-)

Cheers,
Chris.

[Guilhem Moulin]

On Sat, 07 Jul 2018 at 01:52:46 +0200, Christoph Anton Mitterer wrote:
> So the use case is, that people may also wish to modify parts of the
> ../cryptroot/crypttab file from their hook scripts... and that it
> could be nice to have an interface for that as well.

That adds too much complexity for the benefit of a too specific use
case, IMHO. Since you already “abuse” CRYPTTAB_KEY and parse/split its
value in your hook and keyscript, you could as well but both the source
and destination path there:

device=…:pathsrc=…:pathdst=…

Or have a static destination path that uniquely depends on $CRYPTTAB_NAME,
for instance [$DESTINATION]/cryptroot/keyfiles/$CRYPTTAB.key.

--
Guilhem.

[Christoph Anton Mitterer]

Hey Guilhem.

I've just wondered whether the way you've mentioned above is still
valid respectively considered "stable" now (as it: for use by
keyscripts)? :-)

And further, did I understand it right, that
$DESTDIR/cryptroot/crypttab would contain one line (in the crypttab
syntax) per device that needs unlocking in the initramfs?

Which could then of course mean, that each line uses a different
keyscript.


Thanks,
Chris.

[Christoph Anton Mitterer]

Oh and perhaps a bit more O:-)

The basic idea, AFAIU, would be the following, for my case:



1) The keyscript is repeatedly invoked for each device and has
CRYPTTAB_* already set (each time with the respective values).
So in that case I don't have to loop over all devices myself, nor to I
have to use anything from /lib/cryptsetup/functions .

The only exception would be, if - in my specific use case - they device
with the gpg-encrypted key file is one of those unlocked during
initramfs, in which I could end up in a deadlock.




2) For the hook the idea is, AFAIU the following:
I (!) have to loop over all the devices listed in
"$DESTDIR/cryptroot/crypttab", which had previously been populated by
the cryptoroot hook (upon which I depend).

I have no options set, but I can use crypttab_parse_options to get
them.

Then for each entry I first need to check, whether it's me (i.e.
whether the $CRYPTTAB_OPTION_keyscript is mine).
Then I can go on and parse CRYPTTAB_KEY, which in turn contains the
various options for my script.

And obviously I should e.g. do the copy_exec only once.

Does that seem about right?


Is TABFILE to be used by 3rd party hooks/keyscripts?


Or is there a better way? I've seen crypttab_foreach_entry()...

Could I use that like this:

myhook()
{
#- parses CRYPTTAB_KEY
#- set variables whether it needs to copy stuff in
}

crypttab_foreach_entry(myhook)

if [ $foo = "yes" ]; then
copy_exec whatever
fi


AFAIU, that function would also automatically detect whether it's in a
hook context, and set the right TABFILE?



Thanks,
Chris.

[Guilhem Moulin]

Hi,

On Thu, 09 Sep 2021 at 00:54:51 +0200, Christoph Anton Mitterer wrote:
> I've just wondered whether the way you've mentioned above is still
> valid respectively considered "stable" now (as it: for use by
> keyscripts)? :-)

Well we've never received any follow-up regarding a stable interface
and/or documented functions, so this bug is still open and the situation
is not more stable than 3 years ago :-P The interface suggested in
https://bugs.debian.org/901795#53 likely still works, but is still
subject to change while this bug is open.

> And further, did I understand it right, that
> $DESTDIR/cryptroot/crypttab would contain one line (in the crypttab
> syntax) per device that needs unlocking in the initramfs?

Yes, however the exact TABFILE value may not be relied upon, and the
file content is mangled to make it suitable for initramfs stage, so it
might not match /etc/crypttab.

> And obviously I should e.g. do the copy_exec only once.

copy_exec() is a no-op when the destination exists.

> Or is there a better way? I've seen crypttab_foreach_entry()...
>
> Could I use that like this:
>
> myhook()
> {
> #- parses CRYPTTAB_KEY
> #- set variables whether it needs to copy stuff in
> }
>
> crypttab_foreach_entry(myhook)
>
> if [ $foo = "yes" ]; then
> copy_exec whatever
> fi
>
> AFAIU, that function would also automatically detect whether it's in a
> hook context, and set the right TABFILE?

Yes, again see https://bugs.debian.org/901795#53 , that's what
/usr/share/initramfs-tools/hooks/cryptgnupg and some of our other hooks
do.

--
Guilhem.

[Christoph Anton Mitterer]

Hey.

Thanks for you reply. :-)

I'm still a bit unsure how to do it right, in certain aspects:

1) crypttab_find_entry()
What would be a use case for that?

I mean in a keyscript, CRYPTTAB_* are anyway already set for the
"current" target, right?
And in a initramfs hook, I anyway need to loop over all of them... or
at least I wouldn't have a particular (target) name to search for?




2) crypttab_foreach_entry()
That's what I'd use in the hook. But you've mentioned that the
callbacks return value is ignored.
Could that be changed perhaps?

In my case it's probably not a big deal:
- if the callback would find that the "current" entry isn't meant for
"my" script, then it could just return without doing anything
- if however an error occurs (e.g. no pathname= set) it would anyway
just exit 1 the whole hook, since it's "catatrophic" failure and an
initramfs level device couldn't be unlocked

Still it might be nice to determine outside of the foreach to determine
what to do.
Of course one could just set a "global" variable, indicating an error
and set from within the callback.


Also in crypttab_foreach_entry(), do I already have CRYPTTAB_OPTION_*?
Or really just the four CRYPTTAB_{NAME,SOURCE,KEY,OPTIONS} and I need
to call crypttab_parse_options to get the split up ones?

But in keyscripts I would already have CRYPTTAB_OPTION_*, too, right?



3) Escaping
My understanding is, that in both, the keyscript and the hook (when
using e.g. crypttab_foreach_entry()) any CRYPTTAB_* is already
unescaped, right?

You also mention above, that CRYPTTAB_OPTIONS is not safe to use, when
values contain ",".
I assume this is because, the unescaped CRYPTTAB_OPTIONS would contain
both, the "," from the values and the "," from the separators?
While CRYPTTAB_OPTION_* would take care of that properly?

So could I do basically something like this for the 4th field:
luks,keyscript=stupid\054name
and I'd get
CRYPTTAB_OPTIONS='luks,keyscript=stupid,name'
but
CRYPTTAB_OPTION_luks='yes'
CRYPTTAB_OPTION_keyscript='stupid,name'


For which fields are the octal escapes handled? The manpage only
mentions them for them for the key/3rd field.



4) Wishlist ;-)
Can we have something like the option splitting for the key/3rd field,
too?

You remember what I did? E.g.:
device=/dev/disk/by-label/boot-usb-stick:pathname=/path-on-that-device/key.gpg

Obviously there's the same issue, if some value would contain my
separator character (I've used : cause I wasn't sure if it troubles the
parsers from crypttab if I use , ... but I'd happily change to
something recommended from upstream).

I think there might be more keyscripts that benefit from this:
The fourth fields is rather for general crypttab options and I don't
think it would be wise if keyscript-specific options would be put in
there (mostly because they could collide with different keyscripts).

To me, the natural place or any options related to retrieving the key
is the 3rd field.
That would also include e.g. hostnames/ports for a keyscript that
retrieves the key via SSH,... or maybe if one uses a smartcard that can
hold multiple keys, the identifier of that cards keyslot.


I guess that would work similar to crypttab_parse_options? Maybe just a
different name crypttab_parse_keyfile_as_options?
The unsetting of variables you do there... that might be difficult to
do, since we have no idea how these options could be named, e.g.
CRYPTTAB_KEYFILEOPTION_device

I also don't think it’s easily possible to unset any
CRYPTTAB_KEYFILEOPTION_* variables.

"set" lists all name=value, but these may contain newlines and I think
it might not be easy to sanitize that.
$ dash
$ set
COLORTERM='truecolor'
DBUS_SESSION_BUS_ADDRESS='unix:path=/run/user/1000/bus'
...
HOME='/home/calestyo'
IFS='
'
LANG='en_DE.UTF-8'


One could have a var like:
FOO='
BAR=baz
'


However,... it might actually work to do this generically:
If dash's syntax is really always:
name=value-with-some-quoting
with the 2nd ' being possibly in another line, we could do e.g.

set | sed -n 's/^\(CRYPTTAB_KEYFILEOPTION_[a-zA-Z_0-9]\+\)=.*$/\1/p'

and unset everything that results.
Sure this could contain some false positives, if e.g. someone had set
BAR='
CRYPTTAB_KEYFILEOPTION_foo='"'"'is not a var'"'"'
'

we would also get CRYPTTAB_KEYFILEOPTION_foo, but who cares? It's "our"
namespace.


Maybe you could also use that for unsetting CRYPTTAB_OPTION_*



In the end,... and you'll probably not like it ^^ ... I'd even suggest
to rename the 3rd filed to something more generic... just KEY or
KEYOPTIONS or so.
Simply to make it clear that this doesn't have to be a file.



Cheers,
Chris.

[Christoph Anton Mitterer]

Oh and one more thing which is a bit unrelated to this, but also a bit
related ;-)

Could you clarify:
tries=<num>
Try to unlock the device <num> before failing. It's particularly
useful when using a passphrase or a keyscript that asks for
interactive input. If you want to disable retries, pass “tries=1”.
Default is “3”. Setting “tries=0” means infinitive retries.


which AFAIU is how often cryptsetup invokes the keyscript and *not* how
often the keyscript itself tries (e.g. asking for a passphrase).

And that's also what should be clarified / "defined", like by saying:
Try to unlock the device <num> before failing. Its how often
the keyscript is invoked when it fails.
If you want to disable retries, pass “tries=1”.
Default is “3”. Setting “tries=0” means infinitive retries.
Note that keyscripts themselves may do their own tries in
addition.


What I describe above makes IMO actually sense, i.e. having two
different kind of tries:
Take my keyscript as an example, which waits for a device, reads a gpg-
enced key from it with passdev, then waits for a passphrase with
askpass and uses that to decrypt the data with gpg.

Currently, when I enter a wrong key (e.g. at boot time) I have to plug
the USB again (retry made by cryptsetup's 4th field tries=0), because
the keyscript exited and the already read stuff is gone.

With an additiones tries=, specific to the keyscript (and set again in
the 3rd field that I abuse so belovedly) I could do the following:

The "internal" tries is e.g. 3, so my own keyscript will already try
reading the passphrase and decrypting the previously read gpg-enced
file 3 times before giving up.

I could surround the asskpass with a timeout, just to make sure that
they keyscipt (with the precious key in memory, allowing for coldboot
attacks) doesn't stay there forever (e.g. if I forgot about the
computer and went shopping).
If the timeout would be e.g. 15s per default, and the internal tries 3,
they keyscript would wait at most 45s... and then exit.

Then the cryptsetup tries=n comes again (for the initramfs it probably
only makes sense with =0), but now, it would need the USB stick again.


Thanks,
Chris.

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 01:31:31 +0200, Christoph Anton Mitterer wrote:
> I mean in a keyscript, CRYPTTAB_* are anyway already set for the
> "current" target, right?
> And in a initramfs hook, I anyway need to loop over all of them... or
> at least I wouldn't have a particular (target) name to search for?

Right.

> 2) crypttab_foreach_entry()
> That's what I'd use in the hook. But you've mentioned that the
> callbacks return value is ignored.
> Could that be changed perhaps?

If desired, yes.

> In my case it's probably not a big deal:
> - if the callback would find that the "current" entry isn't meant for
> "my" script, then it could just return without doing anything
> - if however an error occurs (e.g. no pathname= set) it would anyway
> just exit 1 the whole hook, since it's "catatrophic" failure and an
> initramfs level device couldn't be unlocked

Right, that's what we're doing too.

> Also in crypttab_foreach_entry(), do I already have CRYPTTAB_OPTION_*?
> Or really just the four CRYPTTAB_{NAME,SOURCE,KEY,OPTIONS} and I need
> to call crypttab_parse_options to get the split up ones?

You need to call crypttab_parse_options() in the callback, see the
cryptgnupg keyscript for an example. IIRC this is intentional because
te callback need to have the ability to bail out before option
validation.

> But in keyscripts I would already have CRYPTTAB_OPTION_*, too, right?

That's what's documented in crypttab(5).

> 3) Escaping
> My understanding is, that in both, the keyscript and the hook (when
> using e.g. crypttab_foreach_entry()) any CRYPTTAB_* is already
> unescaped, right?

Yes. FWIW the original unescaped values can be found in _CRYPTTAB_*,
but this is undocumented and thus may not be relied upon.

> You also mention above, that CRYPTTAB_OPTIONS is not safe to use, when
> values contain ",".
> I assume this is because, the unescaped CRYPTTAB_OPTIONS would contain
> both, the "," from the values and the "," from the separators?
> While CRYPTTAB_OPTION_* would take care of that properly?

Correct.

> For which fields are the octal escapes handled? The manpage only
> mentions them for them for the key/3rd field.

My bad, it's supported in all fields.

> 4) Wishlist ;-)
> Can we have something like the option splitting for the key/3rd field,
> too?

That's too much a niche case IMHO. When you use a key script the 3rd
field is an opaque value passed along and you might treat it any way you
see fit.

> In the end,... and you'll probably not like it ^^ ... I'd even suggest
> to rename the 3rd filed to something more generic... just KEY or
> KEYOPTIONS or so.

That would have have been a valid suggestion at the time the interface
was designed, but many releases later I'm afraid renaming is not an
option.

--
Guilhem.

[Christoph Anton Mitterer]

Hey Guilhem.

On Sat, 2021-09-11 at 03:13 +0200, Guilhem Moulin wrote:
> > 2) crypttab_foreach_entry()
> > That's what I'd use in the hook. But you've mentioned that the
> > callbacks return value is ignored.
> > Could that be changed perhaps?
>
> If desired, yes.

Again, it's probably not so important for my own case, but with respect
to getting the whole thing (eventually) stabilised, I'd probably
recommend it.

I'm just not sure what should be returned then:
- always either 0 (all succeeded) or 1 (a failure happened)
or
- always either 0 (all succeeded) or <the non-zero exit status of the
failing callback>

What would you suggest?

> > Also in crypttab_foreach_entry(), do I already have
> > CRYPTTAB_OPTION_*?
> > Or really just the four CRYPTTAB_{NAME,SOURCE,KEY,OPTIONS} and I
> > need
> > to call crypttab_parse_options to get the split up ones?
>
> You need to call crypttab_parse_options() in the callback, see the
> cryptgnupg keyscript for an example.  IIRC this is intentional
> because
> te callback need to have the ability to bail out before option
> validation.
>  
> > But in keyscripts I would already have CRYPTTAB_OPTION_*, too,
> > right?
>
> That's what's documented in crypttab(5).

Yes, than all is correctly documented... I just wasn't sure whether
this might be either a documentation issue or undesired behaviour cause
the two (keyscript / hook) differ, or the one it's already there, for
the others not.

But it's alright then.

> > For which fields are the octal escapes handled? The manpage only
> > mentions them for them for the key/3rd field.
>
> My bad, it's supported in all fields.

Are you going to correct it or shall I provide a patch for it?

> > 4) Wishlist ;-)
> > Can we have something like the option splitting for the key/3rd
> > field,
> > too?
>
> That's too much a niche case IMHO.  When you use a key script the 3rd
> field is an opaque value passed along and you might treat it any way
> you
> see fit.

I would really really ... really ;-) strongly hope you'd reconsider
this:

- With the keyscript we have such a nice and powerful way that let
people nearly arbitrarily extend the functionality.
But since stuff is pre-processed by cryptsetups own scripts (e.g.
escaping and all that stuff) it could happen quite easily, that
keyscripts and hooks break, if they'd all do their own stuff.
Just take my example:
- I already use ":" as separator, making the configuration style
inhomogeneous with the rest.
- Right now I have no escaping support at all, so people using weird
values would just break my script.
- And even if I repeat the unescaping step by using _CRYPTTAB_* I
again use "inofficial" API, which could break, should you ever
decide to change things.

- What are the possible (reasonable) cases of keyscripts, that just
need a plain pathname as 3rd-field parameter?
If such keyscript knows nothings else (which it cannot, because
there's no way to configure it), then the keyfile itself needs
already be readily available in the filesystem, or at least it must
be possible to try through all possible candidates (like e.g. all
slots of a crypto smartcard).

That rules out any keyscript where the key is taken from somewhere
that doesn't look like a file:
- from network (where host/port/remote-server SSH key or so would be
needed)
- from crypto smartcards/etc., when LUKS is *not* used. Here it might
not be possible to determine, whether one had tried the right key.
Sure, there is "check=" but consider a double encrypted plain dm-
crypt volume ... there would be no check there.
- or like my case, where it's read from a non available device, where
some device ID is needed)
and all that would need to be configured somewhere, and should
ideally use the same rules for quoting, separators, etc..

- If cryptsetup would provide a function like crypttab_parse_options
for the 3rd filed, and maybe in addition makes the CRYPTTAB_*
variables stable, I'd also call #901795 done, and everything that
a keyscript maker could possible need, provided via some proper API.

- Also I think the change I'm asking for is not so invasive, or is it?
Would e.g. the following do it already (two questions inside the
code)?


# crypttab_parse_key_as_options([--export], [--quiet])
# Parses $_CRYPTTAB_KEY, as a comma-separated option string from the
# crypttab(5) 3th column, and sets corresponding variables
# CRYPTTAB_KEYOPTION_<option>=<value> (which are added to the environment
# if --export is set).
# For error and warning messages, CRYPTTAB_NAME, (resp. CRYPTTAB_KEY)
# should be set to the (unmangled) mapped device name (resp. key
# option string).
# Return 1 on parsing error, 0 otherwise (incl. if unknown options
# were encountered).
crypttab_parse_key_as_options() {
local quiet="n" export="n"
while [ $# -gt 0 ]; do
case "$1" in
--quiet) quiet="y";;
--export) export="y";;
*) cryptsetup_message "WARNING: crypttab_parse_key_as_options(): unknown option $1"
esac
shift
done

local IFS=',' x OPTION VALUE

# unset any CRYPTTAB_KEYOPTION_* variables
# This may also determine and unset some CRYPTTAB_KEYFILEOPTION_* names which
# were not even set (namely in cases, where such strings were contained in
# variable values with newlines at the right place), but it doesn't harm, since
# we anyway claim the whole CRYPTTAB_KEYOPTION_* "namespace" as "ours".
# Note that [:alnum:] isn't used as it depends on the locale.
unset -v $( set | sed -n 's/^\(CRYPTTAB_KEYFILEOPTION_[a-zA-Z_0-9]\+\)=.*$/\1/p' | tr '\n' ' ' )

# use $_CRYPTTAB_KEY not $CRYPTTAB_KEY as options values may
# contain '\054' which is decoded to ',' in the latter
for x in $_CRYPTTAB_KEY; do
OPTION="${x%%=*}"
VALUE="${x#*=}"
if [ "$x" = "$OPTION" ]; then
unset -v VALUE
else
VALUE="$(printf '%b' "$VALUE")"
###=> is this the place where you unescape?
### then the documentation is wrong, casue %b doesn't only unescape octal sequences, right?
fi
if ! crypttab_validate_option; then
###=> not exactly sure what we should do here:
###
### do you test anywhere whether OPTION is a vaild trailing part of variable names?
### cause that's what I'd do instead of crypttab_validate_option, which wouldn't make sense,
### since we cannot really check the values of keyscript options
### maybe I could either just error out if something not [a-zA-Z_0-9]+ is encountered, or
### replace any of those with "_"?
###
return 1
elif [ -z "${OPTION+x}" ]; then
continue
fi
if [ "$export" = "y" ]; then
export "CRYPTTAB_OPTION_$OPTION"="${VALUE-yes}"
else
eval "CRYPTTAB_OPTION_$OPTION"='${VALUE-yes}'
fi
done
IFS=" "

}





>
> > In the end,... and you'll probably not like it ^^ ... I'd even
> > suggest
> > to rename the 3rd filed to something more generic... just KEY or
> > KEYOPTIONS or so.
>
> That would have have been a valid suggestion at the time the
> interface
> was designed, but many releases later I'm afraid renaming is not an
> option.

Well it's just a name, so I don't care so much about it.
But if you'd choose to accept my above proposal, it would at least make
sense to add to the manpage, that keyscripts might use the 3rd field
not as a keyfile pathname, but also as comma-separated and printf %b
unescaped options.


Thanks,
Chris.

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 17:12:17 +0200, Christoph Anton Mitterer wrote:
>>> For which fields are the octal escapes handled? The manpage only
>>> mentions them for them for the key/3rd field.
>>
>> My bad, it's supported in all fields.
>
> Are you going to correct it or shall I provide a patch for it?

I'll fix it, thanks for the notice!

>>> 4) Wishlist ;-)
>>> Can we have something like the option splitting for the key/3rd
>>> field,
>>> too?
>>
>> That's too much a niche case IMHO.  When you use a key script the 3rd
>> field is an opaque value passed along and you might treat it any way

>> see fit.
>
> I would really really ... really ;-) strongly hope you'd reconsider
> this:

I still stand by what I wrote here 3 years ago, it's a useniche case and
we have no reason to assume that the opaque 3rd field value is
$FOO-delimited. For all I know there might be keyscripts which expect a
JSON string here instead… I wouldn't mind documenting _CRYPTTAB_KEY for
those who need the raw value from crypttab(5), but even without the
ambiguity can easily be eliminated by double-escaping or simply using
other escape sequences: “foo\040bar:ba%3Az” in crypttab yields
CRYPTTAB_KEY="foo bar:baz%3Abar" which you can trivially decode into a
pair ["foo bar", "ba:z"]. Moreover, doing this makes it possible to
manipulate binary strings which is not something we can do with
pre-mangled environment variables.

--
Guilhem.

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 17:12:17 +0200, Christoph Anton Mitterer wrote:
> VALUE="$(printf '%b' "$VALUE")"
> ###=> is this the place where you unescape?
> ### then the documentation is wrong, casue %b doesn't only unescape octal sequences, right?

Not wrong in my view, but incomplete and using undocumented escape
sequences yields unspecified behavior. The intent was to mimic fstab(5)
behavior, and IIRC I noticed at the time that \xHH was supported
although not documented either. Either way, better stick to documented
escape sequences in both files.

--
Guilhem.

[Christoph Anton Mitterer]

On Sat, 2021-09-11 at 17:55 +0200, Guilhem Moulin wrote:
> we have no reason to assume that the opaque 3rd field value is
> $FOO-delimited.  For all I know there might be keyscripts which
> expect a
> JSON string here instead…

Well sure... or it could be base64 encoded, XML or whatever.

Question is though: What makes sense to provide to programmers of
keyscripts?

Will such developer insist on using format XYZ if he's already provided
with out-of-the-box tools for some other format ABC and that's enough
for his needs?

I would have thought that the whole system benefits if one tries to
keep things as homogeneous as possible with an API that handles things
in a stable way and users don't have to adapt their keyscripts
everytime something changes in the back.

I doubt it's so beneficial to treat it as fully opaque and let
everything like escaping be done differently per keyscript.

> I wouldn't mind documenting _CRYPTTAB_KEY for
> those who need the raw value from crypttab(5), but even without the
> ambiguity can easily be eliminated by double-escaping or simply using
> other escape sequences: “foo\040bar:ba%3Az” in crypttab yields
> CRYPTTAB_KEY="foo bar:baz%3Abar" which you can trivially decode into
> a
> pair ["foo bar", "ba:z"].

Well sure I can one can do it the keyscript. I could simply take the
example crypttab_parse_key_as_options() given before and put that into
the keyscript.
My point was rather that this ain't a wheel every keyscript developer
should need to re-invent.

But anyway... I guess that leads to nothing.


I guess documenting _CRYPTTAB_* as stable API would be helpful, from
there all keyscript developers could re-do the some similar kind of
parsing.
But it probably only makes sense if the crypttab_*() functions would
also get ever official, which currently doesn't seem on the near
horizon.



Well, from my side we could probably close the bug or at least I can
unsubscribe it, since there doesn't seem to be any clear path forward
to really stabilise that API and there's no desire to extend it either.

I will just "unofficially" use what's already and adapt should it ever
break :D


Thanks for you help,
Chris.

[Christoph Anton Mitterer]

On Sat, 2021-09-11 at 18:06 +0200, Guilhem Moulin wrote:
> Not wrong in my view, but incomplete and using undocumented escape
> sequences yields unspecified behavior.

Well the problem is simply that anyone who uses in any of the fields
e.g. \n will end up getting a true newline and not the literal \n, as
one would assume from the documentation, which just mentions the octal
escapes.


Btw, there might also be a subtle security issue:

If, for some reason, normal users are allowed to directly or indirectly
control the contents of crypttab, they could probably inject shell code
here:

eval "CRYPTTAB_OPTION_$OPTION"='${VALUE-yes}'

Cheers,
Chris.

[Christoph Anton Mitterer]

On Sat, 2021-09-11 at 17:55 +0200, Guilhem Moulin wrote:

> I wouldn't mind documenting _CRYPTTAB_KEY for
> those who need the raw value from crypttab(5)

btw: it might make sense for you to instead create and document a copy
of the _CRYPTTAB_* e.g. RAW_CRYPTTAB_* .

That would give you the freedom to change/mangle/etc. the underlying
_CRYPTTAB_* should you ever wish to.


Cheers,
Chris.

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 18:31:33 +0200, Christoph Anton Mitterer wrote:
> On Sat, 2021-09-11 at 18:06 +0200, Guilhem Moulin wrote:
>> Not wrong in my view, but incomplete and using undocumented escape
>> sequences yields unspecified behavior.
>
> Well the problem is simply that anyone who uses in any of the fields
> e.g. \n will end up getting a true newline and not the literal \n, as
> one would assume from the documentation, which just mentions the octal
> escapes.

I was about to reply “like fsttab(5)” but it seems fstab-decode(8)
doesn't mangle ‘\xHH’, ‘\t’ or ‘\n’. So either I misremembered testing
this at the time, or something changed meanwhile :-) I'd argue that ‘\’
is a special character which per documentation “needs to be escaped
using octal sequences”, so both ‘\n’ and ‘\xHH’ yield unspecified
behavior, but I guess that can be made explicit.

> Btw, there might also be a subtle security issue:
>
> If, for some reason, normal users are allowed to directly or indirectly
> control the contents of crypttab, they could probably inject shell code
> here:
> eval "CRYPTTAB_OPTION_$OPTION"='${VALUE-yes}'

We assume that unprivileged users do not have write access to
/etc/crypttab (actually, $TABFILE), keyscripts, or initramfs hook/
scripts. Otherwise one can replace askpass with `mail m...@example.net`,
append ‘keyscript=gimme_your_password’ to crypttab entries, or simply
ship compromised executables in the initramfs image.

--
Guilhem.

[Christoph Anton Mitterer]

On Sat, 2021-09-11 at 20:06 +0200, Guilhem Moulin wrote:
> I was about to reply “like fsttab(5)” but it seems fstab-decode(8)
> doesn't mangle ‘\xHH’, ‘\t’ or ‘\n’.

AFAICS, at least \xHH is not specified for that in at least POSIX:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html

dash, doesn't seem to do it either, but bash does.

>   So either I misremembered testing
> this at the time, or something changed meanwhile :-)  I'd argue that
> ‘\’
> is a special character which per documentation “needs to be escaped
> using octal sequences”, so both ‘\n’ and ‘\xHH’ yield unspecified
> behavior, but I guess that can be made explicit.

Maybe the easiest is simply to write e.g.:
Every field of crypttab is unescaped using printf(1)’s “%b” conversion
specification, which unescapes the \-escapes (\n, \t, \0num, etc.) as
provided by the echo utility.

Than we're always automatically on the safe side.

> We assume that unprivileged users do not have write access to
> /etc/crypttab (actually, $TABFILE), keyscripts, or initramfs hook/
> scripts.  Otherwise one can replace askpass with `mail
> m...@example.net`,
> append ‘keyscript=gimme_your_password’ to crypttab entries, or simply
> ship compromised executables in the initramfs image.

I'd still suggest to document that (and not just assume it silently)...
otherwise some smartypants admins might thinkt it's ok to allow users
to just append entries to crypttab in some way they think it would be
secure.



Cheers,
Chris.

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 20:26:57 +0200, Christoph Anton Mitterer wrote:
> On Sat, 2021-09-11 at 20:06 +0200, Guilhem Moulin wrote:
>>   So either I misremembered testing
>> this at the time, or something changed meanwhile :-)  I'd argue that
>> ‘\’
>> is a special character which per documentation “needs to be escaped
>> using octal sequences”, so both ‘\n’ and ‘\xHH’ yield unspecified
>> behavior, but I guess that can be made explicit.
>
> Maybe the easiest is simply to write e.g.:
> Every field of crypttab is unescaped using printf(1)’s “%b” conversion
> specification, which unescapes the \-escapes (\n, \t, \0num, etc.) as
> provided by the echo utility.
>
> Than we're always automatically on the safe side.

The use of `printf %b` to decode escape sequences is an internal
implementation detail; documenting it would tie our hands for
implementation changes…

Also there is no guaranty that /bin/sh is dash; don't forget that we
also run at initramfs stage where /bin/sh is typically `busybox ash`
(but again no guaranty, it can be dash, bash, klibc's sh, or anything
else) for which `printf %b` *does* decode \xHH. In my view
overspecifying is all but ideal here.

>> We assume that unprivileged users do not have write access to
>> /etc/crypttab (actually, $TABFILE), keyscripts, or initramfs hook/
>> scripts.  Otherwise one can replace askpass with `mail
>> m...@example.net`,
>> append ‘keyscript=gimme_your_password’ to crypttab entries, or simply
>> ship compromised executables in the initramfs image.
>
> I'd still suggest to document that (and not just assume it silently)...
> otherwise some smartypants admins might thinkt it's ok to allow users
> to just append entries to crypttab in some way they think it would be
> secure.

Do we warn users not to make /usr/bin, /root or /etc/ld.so.conf writable
by unprivileged users? :-) Or not to remove the sticky bit on /tmp?

--
Guilhem.

[Christoph Anton Mitterer]

On Sat, 2021-09-11 at 21:55 +0200, Guilhem Moulin wrote:
> The use of `printf %b` to decode escape sequences is an internal
> implementation detail; documenting it would tie our hands for
> implementation changes…
>
> Also there is no guaranty that /bin/sh is dash; don't forget that we
> also run at initramfs stage where /bin/sh is typically `busybox ash`
> (but again no guaranty, it can be dash, bash, klibc's sh, or anything
> else) for which `printf %b` *does* decode \xHH.

True.
Well then best is probably to e.g. document the \0xxx and mention that
any other use of \ needs to have that quoted or it may have a special
meaning?

That should neither over- nor under-specify it.


Cheers,
Chris.
> >

[Guilhem Moulin]

On Sat, 11 Sep 2021 at 22:01:52 +0200, Christoph Anton Mitterer wrote:
> Well then best is probably to e.g. document the \0xxx and mention that
> any other use of \ needs to have that quoted or it may have a special
> meaning?

Right, that's what I was hinting at in https://bugs.debian.org/901795#128 :-)

| I'd argue that ‘\’ is a special character which per documentation
| “needs to be escaped using octal sequences”, so both ‘\n’ and ‘\xHH’
| yield unspecified behavior, but I guess that can be made explicit.

--
Guilhem.

[Christoph Anton Mitterer]

Hey again.

Next to stabilising "_CRYPTTAB_*", could you also export it to the
keyscripts?

I can see it in the initramfs hook, when using crypttab_foreach_entry,
but it's not there in the keyscript.

Thus I cannot implement my own unescaping.


Cheers,
Chris.

[Christoph Anton Mitterer]

Seems to be doable with a simply oneliner:

--- a/lib/cryptsetup/functions 2021-09-27 03:30:31.928052985 +0200
+++ b/lib/cryptsetup/functions 2021-09-27 03:30:28.387976358 +0200
@@ -278,6 +278,7 @@
local keyscriptarg="$1" CRYPTTAB_TRIED="$2" keyscript;
export CRYPTTAB_NAME CRYPTTAB_SOURCE CRYPTTAB_OPTIONS
export CRYPTTAB_TRIED
+ export _CRYPTTAB_NAME _CRYPTTAB_SOURCE _CRYPTTAB_KEY _CRYPTTAB_OPTIONS

if [ -n "${CRYPTTAB_OPTION_keyscript+x}" ] && \
[ "$CRYPTTAB_OPTION_keyscript" != "/lib/cryptsetup/askpass" ]; then


But I'm not sure how you'd want to handle _CRYPTTAB_KEY. It seems to be
there at this point, but CRYPTTAB_KEY is set below in the conditional
from $keyscriptarg .

Cheers,
Chris.

[Guilhem Moulin]

On Mon, 27 Sep 2021 at 02:56:26 +0200, Christoph Anton Mitterer wrote:
> Thus I cannot implement my own unescaping.

Why not? _CRYTTAB_* is useful to copy a crypttab snippet to another
location, but as said before you don't need it to produce your own
parsing logic. You can use another character than ‘\’ to start your
escape sequence, or double escape the ‘\’s. And again you'll need
something like that to pass NUL bytes anyway.

I don't mind exporting these but it's incorrect to claim that not having
the verbatim strings are preventing massaging.

--
Guilhem.

[Christoph Anton Mitterer]

On Mon, 2021-09-27 at 17:24 +0200, Guilhem Moulin wrote:
> Why not?  _CRYTTAB_* is useful to copy a crypttab snippet to another
> location, but as said before you don't need it to produce your own
> parsing logic.  You can use another character than ‘\’ to start your
> escape sequence, or double escape the ‘\’s.  And again you'll need
> something like that to pass NUL bytes anyway.

Well it's kinda the same point like when I've asked for a parsing
function for the 3rd field...

crypttab has some given format, which follows that of fstab.
This format is (as you know of course):
- one entry per row
- fields separated by whitespace
- options within a field separated by ","
- options either standalone or with =value
- values quoted with \0ooo

Of course one could somehow hack in anything else too, JSON, XML,
base64 encoded binary ASN1, etc., just as one could double-escape (or
triple?) or us another separator char or ≔ instead of =

But why on earth should one want to do any of that?


It would just make editing of the config files more complex and deviate
from the given format style without any good reason.

> I don't mind exporting these but it's incorrect to claim that not
> having
> the verbatim strings are preventing massaging.

Well at least not a "clean" one that simply follows the standard format
without any hacks and workarounds like double-escaping. :-D


In case you haven't seen, I've made a PR which seems to do that
exporting :-)


Thanks,
Chris.

[Guilhem Moulin]

On Mon, 27 Sep 2021 at 18:21:47 +0200, Christoph Anton Mitterer wrote:
> But why on earth should one want to do any of that?

Because the field is opaque, and the key=value list format might not
make sense for keyscripts.

--
Guilhem.

[Christoph Anton Mitterer]

On Mon, 2021-09-27 at 18:37 +0200, Guilhem Moulin wrote:
> Because the field is opaque, and the key=value list format might not
> make sense for keyscripts.

Well sure you can define it that way... but with respect to the fstab-
like-format that makes simply not that much sense:

fstab quite clearly assumes a format as described above. It also
doesn't

There’s no single filesystem type which would expect any options in
fstab’s fourth field which wouldn't follow the actual main format but
take e.g. suvol={JSON} or so.


Why should crypttab go down this road, when it's anyway not really
possible, as neither filed can ever be truly opaque?!

Without encoding respectively quoting an double-quoting, you cannot
have binary data in it nor you can you have JSON/XML in it.


Actually, if it would be opaque for keyscripts, as you say, then it
wouldn't perform any unencoding on it and:
CRYPTTAB_KEY == _CRYPTTAB_KEY



Anyway... I guess that discussion is moot, my whole point was whether
we can get the raw variable exported?


Cheers,
Chris.

[Guilhem Moulin]

On Mon, 27 Sep 2021 at 19:21:45 +0200, Christoph Anton Mitterer wrote:
> On Mon, 2021-09-27 at 18:37 +0200, Guilhem Moulin wrote:
>> Because the field is opaque, and the key=value list format might not
>> make sense for keyscripts.
>
> Well sure you can define it that way... but with respect to the fstab-
> like-format that makes simply not that much sense:
>
> fstab quite clearly assumes a format as described above.

I agree that fstab's *4th column* (option) does, and crypttab's *4th
column* (option) follow the same format. AFAIK fstab itself makes no
assumption on how the 1st field is formatted; like mount(8)'s ‘device’
argument its interpretation depends on the FS type. Looks pretty opaque
to me.

> Actually, if it would be opaque for keyscripts, as you say, then it
> wouldn't perform any unencoding on it and:
> CRYPTTAB_KEY == _CRYPTTAB_KEY

No because the value may contain space and tabs which are used as field
separator hence need to be escaped. For that field I see no need to use
any other octal sequences other than these two.

> Anyway... I guess that discussion is moot,

Yeah, and frankly also rather tiring.

> my whole point was whether we can get the raw variable exported?

As said in msg#163, yes.

--
Guilhem.