Re: [PATCH] check whether the forced iteration count is out of range

[Milan Broz <gmazyland@gmail.com>]

Hi,

I think you are patching some old version - manuals are now maintained
in adoc format (*.8 files are generated) and that min./max cost mmessage is there,
see MINIMAL AND MAXIMAL PBKDF COSTS section in main branch.

On 1/29/23 04:06, wangzhiqiang (Q) wrote:
>  From 303dee7999ea8297dad2961020d96710ee87714b Mon Sep 17 00:00:00 2001
> From: wangzhiqiang <wangzhiqiang95@huawei.com>
> Date: Sat, 28 Jan 2023 15:09:44 +0800
> Subject: [PATCH] check whether the forced iteration count is out of range
> 
> 1. struct crypt_pbkdf_type has a uint32_t variable iterations, but
>     PKCS5_PBKDF2_HMAC interface of openssl accept int variable, so
>     return fail when it greater than INT_MAX.

yes, but UINT32 limits should be be already checked, see below

> 
> 2. add boundary check for the iterations counts.

CLI arguments are checked in src/tools_parse_arg_value.c, see how
CRYPT_ARG_UINT32 is processed there. It just do not print nice
message but only "invalid value" (and that is enough here).

The PBKDF2 arguments for libcryptsetup API are checked in
lib/utils_pbkdf.c.

Do you have some example, where this is not applied and value overflows?
If so, it should be fixed in these places.

For the OpenSSL - this is a nice find that it used int in PKCS5_PBKDF2_HMAC()
but AFAIK new OSSL_PARAMS OpenSSL3 uses *unsigned* int..
And checking providers PBKDF2 implementation in OpenSSL3 I even see they
use uint64 when parsing params.

I'll ask OpenSSL - this iteration count should be clearly treated as unsigned.
(Cryptsetup has different crypto backends, it must behave the same regarding maximums.)

Please, if you can, use main git branch, create an issue or merge request in cryptsetup project,
discussion about patches is much easier there.

Thanks,
Milan


> 
> Signed-off-by: wangzhiqiang <wangzhiqiang95@huawei.com>
> ---
>   lib/crypto_backend/crypto_openssl.c |  2 +-
>   lib/luks2/luks2_keyslot_luks2.c     |  1 +
>   man/cryptsetup.8                    | 14 ++++++++++++++
>   src/cryptsetup.c                    |  5 +++++
>   4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/crypto_backend/crypto_openssl.c b/lib/crypto_backend/crypto_openssl.c
> index 2edec7b..d76d89f 100644
> --- a/lib/crypto_backend/crypto_openssl.c
> +++ b/lib/crypto_backend/crypto_openssl.c
> @@ -329,7 +329,7 @@ int crypt_pbkdf(const char *kdf, const char *hash,
>   {
>          const EVP_MD *hash_id;
> 
> -       if (!kdf)
> +       if (!kdf || iterations > INT_MAX)
>                  return -EINVAL;
> 
>          if (!strcmp(kdf, "pbkdf2")) {
> diff --git a/lib/luks2/luks2_keyslot_luks2.c b/lib/luks2/luks2_keyslot_luks2.c
> index 156f0c1..fe5f1ed 100644
> --- a/lib/luks2/luks2_keyslot_luks2.c
> +++ b/lib/luks2/luks2_keyslot_luks2.c
> @@ -254,6 +254,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
>                          pbkdf.iterations, pbkdf.max_memory_kb,
>                          pbkdf.parallel_threads);
>          if (r < 0) {
> +               log_err(cd, "Invalid parameter.");
>                  crypt_free_volume_key(derived_key);
>                  return r;
>          }
> diff --git a/man/cryptsetup.8 b/man/cryptsetup.8
> index bc3fff6..f6f1fe0 100644
> --- a/man/cryptsetup.8
> +++ b/man/cryptsetup.8
> @@ -1096,6 +1096,20 @@ Note it can cause extremely long unlocking time. Use only is specified
>   cases, for example, if you know that the formatted device will
>   be used on some small embedded system.
>   In this case, the LUKS PBKDF2 digest will be set to the minimum iteration count.
> +
> +\fBMINIMAL AND MAXIMAL PBKDF COSTS:\fR
> +For \fBPBKDF2\fR, the minimum iteration count is 1000 and
> +maximum is 4294967295 (maximum for 32bit unsigned integer),
> +except openssl, which supports only 2147483647 (maximum for 32bit integer).
> +Memory and parallel costs are unused for PBKDF2.
> +For \fBArgon2i\fR and \fBArgon2id\fR, minimum iteration count (CPU cost) is 4 and
> +maximum is 4294967295 (maximum for 32bit unsigned integer).
> +Minimum memory cost is 32 KiB and maximum is 4 GiB. (Limited by addresable
> +memory on some CPU platforms.)
> +If the memory cost parameter is benchmarked (not specified by a parameter)
> +it is always in range from 64 MiB to 1 GiB.
> +The parallel cost minimum is 1 and maximum 4 (if enough CPUs cores are available,
> +otherwise it is decreased).
>   .TP
>   .B "\-\-iter\-time, \-i <number of milliseconds>"
>   The number of milliseconds to spend with PBKDF passphrase processing.
> diff --git a/src/cryptsetup.c b/src/cryptsetup.c
> index a0dc14b..decb39a 100644
> --- a/src/cryptsetup.c
> +++ b/src/cryptsetup.c
> @@ -3921,6 +3921,11 @@ int main(int argc, const char **argv)
>                  _("PBKDF forced iterations cannot be combined with iteration time option."),
>                  poptGetInvocationName(popt_context));
> 
> +       if (opt_pbkdf_iterations < 0 || opt_pbkdf_iterations > UINT32_MAX)
> +               usage(popt_context, EXIT_FAILURE,
> +                _("the minimum iteration count is 1000 and maximum is 4294967295."),
> +                poptGetInvocationName(popt_context));
> +
>          if (opt_sector_size && strcmp(aname, "reencrypt") && strcmp(aname, "luksFormat") &&
>              (strcmp(aname, "open") || strcmp_or_null(opt_type, "plain")))
>                  usage(popt_context, EXIT_FAILURE,
> --
> 2.27.0
>