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

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

[wangzhiqiang (Q)]

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.

2. add boundary check for the iterations counts.

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

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

[Milan Broz]

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
> 

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

[wangzhiqiang (Q)]

在 2023/1/29 16:36, Milan Broz 写道:
> 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.

Yes. I use the old version(cryptsetup-2.3.3) and openssl version less than 3.
MINIMAL AND MAXIMAL PBKDF COSTS section in main branch is well.
> 
> 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).

In main branch, tools_parse_arg_value can process UINT32 limits.
> 
> The PBKDF2 arguments for libcryptsetup API are checked in
> lib/utils_pbkdf.c.

Function verify_pbkdf_params in lib/utils_pbkdf.c has check PBKDF2 arguments.
> 
> Do you have some example, where this is not applied and value overflows?
> If so, it should be fixed in these places.
> 
I run follow command
cryptsetup -c aes-xts-plain64 --key-size=512 --hash sha256 luksFormat --pbkdf=pbkdf2 --pbkdf-force-iterations=4294967295 /dev/sdb
The program is over soon, it is not good because the iterations set to UINT32_MAX,
the program should take a long time.
> 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.)
> 
All crypto backends should remain the same, but the program cannot be encrypted as expected due to
type conversion in the environment where OpenSSL of an earlier version is installed.
> Please, if you can, use main git branch, create an issue or merge request in cryptsetup project,
> discussion about patches is much easier there.
> 
Now i have fixed the patch base on main branch.

struct crypt_pbkdf_type has a uint32_t variable iterations, but
PKCS5_PBKDF2_HMAC accept int variable in openssl(major version
less than 3), so return EINVAL when it greater than INT_MAX.

Signed-off-by: wangzhiqiang <wangzhiqiang95@huawei.com>
---
v1->v2:
- delete changes of manuals
- delete check of UINT32 limits

 lib/crypto_backend/crypto_openssl.c | 2 +-
 lib/luks2/luks2_keyslot_luks2.c     | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/crypto_backend/crypto_openssl.c b/lib/crypto_backend/crypto_openssl.c
index 24a49549..39dd13a1 100644
--- a/lib/crypto_backend/crypto_openssl.c
+++ b/lib/crypto_backend/crypto_openssl.c
@@ -571,7 +571,7 @@ static int openssl_pbkdf2(const char *password, size_t password_length,
        EVP_KDF_free(pbkdf2);
 #else
        const EVP_MD *hash_id = EVP_get_digestbyname(crypt_hash_compat_name(hash));
-       if (!hash_id)
+       if (!hash_id || iterations > INT_MAX)
                return -EINVAL;

        r = PKCS5_PBKDF2_HMAC(password, (int)password_length, (const unsigned char *)salt,
diff --git a/lib/luks2/luks2_keyslot_luks2.c b/lib/luks2/luks2_keyslot_luks2.c
index 78f74242..f01e196b 100644
--- a/lib/luks2/luks2_keyslot_luks2.c
+++ b/lib/luks2/luks2_keyslot_luks2.c
@@ -264,6 +264,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
                        pbkdf.parallel_threads);
        free(salt);
        if (r < 0) {
+               log_err(cd, "Invalid parameter.");
                crypt_free_volume_key(derived_key);
                return r;
        }
--
2.33.0
> 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
>>

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

[Milan Broz]

I tried to fix it a little bit differently,
please see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/481

Does this solve the issue for you?

This is really misconception in older OpenSSL API, so I limit it only there,
new version supports unsigned iteration count (as other libraries).


Thanks for the report!

Milan

On 1/29/23 13:59, wangzhiqiang (Q) wrote:
> 在 2023/1/29 16:36, Milan Broz 写道:
>> 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.
> 
> Yes. I use the old version(cryptsetup-2.3.3) and openssl version less than 3.
> MINIMAL AND MAXIMAL PBKDF COSTS section in main branch is well.
>>
>> 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).
> 
> In main branch, tools_parse_arg_value can process UINT32 limits.
>>
>> The PBKDF2 arguments for libcryptsetup API are checked in
>> lib/utils_pbkdf.c.
> 
> Function verify_pbkdf_params in lib/utils_pbkdf.c has check PBKDF2 arguments.
>>
>> Do you have some example, where this is not applied and value overflows?
>> If so, it should be fixed in these places.
>>
> I run follow command
> cryptsetup -c aes-xts-plain64 --key-size=512 --hash sha256 luksFormat --pbkdf=pbkdf2 --pbkdf-force-iterations=4294967295 /dev/sdb
> The program is over soon, it is not good because the iterations set to UINT32_MAX,
> the program should take a long time.
>> 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.)
>>
> All crypto backends should remain the same, but the program cannot be encrypted as expected due to
> type conversion in the environment where OpenSSL of an earlier version is installed.
>> Please, if you can, use main git branch, create an issue or merge request in cryptsetup project,
>> discussion about patches is much easier there.
>>
> Now i have fixed the patch base on main branch.
> 
> struct crypt_pbkdf_type has a uint32_t variable iterations, but
> PKCS5_PBKDF2_HMAC accept int variable in openssl(major version
> less than 3), so return EINVAL when it greater than INT_MAX.
> 
> Signed-off-by: wangzhiqiang <wangzhiqiang95@huawei.com>
> ---
> v1->v2:
> - delete changes of manuals
> - delete check of UINT32 limits
> 
>   lib/crypto_backend/crypto_openssl.c | 2 +-
>   lib/luks2/luks2_keyslot_luks2.c     | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/crypto_backend/crypto_openssl.c b/lib/crypto_backend/crypto_openssl.c
> index 24a49549..39dd13a1 100644
> --- a/lib/crypto_backend/crypto_openssl.c
> +++ b/lib/crypto_backend/crypto_openssl.c
> @@ -571,7 +571,7 @@ static int openssl_pbkdf2(const char *password, size_t password_length,
>          EVP_KDF_free(pbkdf2);
>   #else
>          const EVP_MD *hash_id = EVP_get_digestbyname(crypt_hash_compat_name(hash));
> -       if (!hash_id)
> +       if (!hash_id || iterations > INT_MAX)
>                  return -EINVAL;
> 
>          r = PKCS5_PBKDF2_HMAC(password, (int)password_length, (const unsigned char *)salt,
> diff --git a/lib/luks2/luks2_keyslot_luks2.c b/lib/luks2/luks2_keyslot_luks2.c
> index 78f74242..f01e196b 100644
> --- a/lib/luks2/luks2_keyslot_luks2.c
> +++ b/lib/luks2/luks2_keyslot_luks2.c
> @@ -264,6 +264,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
>                          pbkdf.parallel_threads);
>          free(salt);
>          if (r < 0) {
> +               log_err(cd, "Invalid parameter.");
>                  crypt_free_volume_key(derived_key);
>                  return r;
>          }
> --
> 2.33.0
>> 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
>>>

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

[wangzhiqiang (Q)]

Hi, Milan

This solves my problem well.

Thanks
wangzhiqiang

在 2023/2/1 3:50, Milan Broz 写道:
> I tried to fix it a little bit differently,
> please see https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/481
> 
> Does this solve the issue for you?
> 
> This is really misconception in older OpenSSL API, so I limit it only there,
> new version supports unsigned iteration count (as other libraries).
> 
> 
> Thanks for the report!
> 
> Milan
> 
> On 1/29/23 13:59, wangzhiqiang (Q) wrote:
>> 在 2023/1/29 16:36, Milan Broz 写道:
>>> 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.
>>
>> Yes. I use the old version(cryptsetup-2.3.3) and openssl version less than 3.
>> MINIMAL AND MAXIMAL PBKDF COSTS section in main branch is well.
>>>
>>> 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).
>>
>> In main branch, tools_parse_arg_value can process UINT32 limits.
>>>
>>> The PBKDF2 arguments for libcryptsetup API are checked in
>>> lib/utils_pbkdf.c.
>>
>> Function verify_pbkdf_params in lib/utils_pbkdf.c has check PBKDF2 arguments.
>>>
>>> Do you have some example, where this is not applied and value overflows?
>>> If so, it should be fixed in these places.
>>>
>> I run follow command
>> cryptsetup -c aes-xts-plain64 --key-size=512 --hash sha256 luksFormat --pbkdf=pbkdf2 --pbkdf-force-iterations=4294967295 /dev/sdb
>> The program is over soon, it is not good because the iterations set to UINT32_MAX,
>> the program should take a long time.
>>> 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.)
>>>
>> All crypto backends should remain the same, but the program cannot be encrypted as expected due to
>> type conversion in the environment where OpenSSL of an earlier version is installed.
>>> Please, if you can, use main git branch, create an issue or merge request in cryptsetup project,
>>> discussion about patches is much easier there.
>>>
>> Now i have fixed the patch base on main branch.
>>
>> struct crypt_pbkdf_type has a uint32_t variable iterations, but
>> PKCS5_PBKDF2_HMAC accept int variable in openssl(major version
>> less than 3), so return EINVAL when it greater than INT_MAX.
>>
>> Signed-off-by: wangzhiqiang <wangzhiqiang95@huawei.com>
>> ---
>> v1->v2:
>> - delete changes of manuals
>> - delete check of UINT32 limits
>>
>>   lib/crypto_backend/crypto_openssl.c | 2 +-
>>   lib/luks2/luks2_keyslot_luks2.c     | 1 +
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/crypto_backend/crypto_openssl.c b/lib/crypto_backend/crypto_openssl.c
>> index 24a49549..39dd13a1 100644
>> --- a/lib/crypto_backend/crypto_openssl.c
>> +++ b/lib/crypto_backend/crypto_openssl.c
>> @@ -571,7 +571,7 @@ static int openssl_pbkdf2(const char *password, size_t password_length,
>>          EVP_KDF_free(pbkdf2);
>>   #else
>>          const EVP_MD *hash_id = EVP_get_digestbyname(crypt_hash_compat_name(hash));
>> -       if (!hash_id)
>> +       if (!hash_id || iterations > INT_MAX)
>>                  return -EINVAL;
>>
>>          r = PKCS5_PBKDF2_HMAC(password, (int)password_length, (const unsigned char *)salt,
>> diff --git a/lib/luks2/luks2_keyslot_luks2.c b/lib/luks2/luks2_keyslot_luks2.c
>> index 78f74242..f01e196b 100644
>> --- a/lib/luks2/luks2_keyslot_luks2.c
>> +++ b/lib/luks2/luks2_keyslot_luks2.c
>> @@ -264,6 +264,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
>>                          pbkdf.parallel_threads);
>>          free(salt);
>>          if (r < 0) {
>> +               log_err(cd, "Invalid parameter.");
>>                  crypt_free_volume_key(derived_key);
>>                  return r;
>>          }
>> -- 
>> 2.33.0
>>> 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
>>>>