[pkg-cryptsetup-devel] Bug#901795: cryptsetup-initramfs: please provide documented shell functions to validate/sanitize cryptroot entries in 3rd party hook files

[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.