Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

Hongbo Li
2024-08-14T02:10:08Z

On 2024/8/13 21:16, Mae Kasza wrote:
>
> On 8/12/24 04:50, Hongbo Li wrote:
>>
>>
>> On 2024/8/12 9:41, Kent Overstreet wrote:
>>> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>>>> From: mae <git@badat.dev>
>>>>
>>>> The set-passphrase command failed to derive the key for disks initially
>>>> formatted with --encrypted and --no_passphrase.
>>>>
>>>> This happened because bch_sb_crypt_init only configures the KDF params
>>>> if a passphrase is specified.
>>>> This commit makes the command initialize the KDF with the same
>>>> parameters
>>>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>>>
>>>> Signed-off-by: Mae Kasza <git@badat.dev>
>>>
>>> This looks good to me, but I'm in the middle of a 20 hour drive and not
>>> feeling super confident to review crypto stuff tonight, so maybe someone
>>> else can go over it too before I pull it in..
>>>
>>>> ---
>>>>   c_src/cmd_key.c |  9 +++++++--
>>>>   c_src/crypto.c  | 13 ++++++++-----
>>>>   c_src/crypto.h  |  1 +
>>>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>>>> index adb0ac8d..2da83758 100644
>>>> --- a/c_src/cmd_key.c
>>>> +++ b/c_src/cmd_key.c
>>>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>       if (IS_ERR(c))
>>>>           die("Error opening %s: %s", argv[1],
>>>> bch2_err_str(PTR_ERR(c)));
>>>>   -    struct bch_sb_field_crypt *crypt =
>>>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>>>> +    struct bch_sb *sb = c->disk_sb.sb;
>>>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>>>       if (!crypt)
>>>>           die("Filesystem does not have encryption enabled");
>>>>   @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>           die("Error getting current key");
>>>>         char *new_passphrase = read_passphrase_twice("Enter new
>>>> passphrase: ");
>>>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>>> +        bch_crypt_default_kdf_init(crypt);
>> It works, but using bch_sb_crypt_init to reinitialize crypt may be
>> better. In bch_sb_crypt_init, it has initialized the crypt and also
>> cleaned new_passphrase.
>>
>> Thanks
>> Hongbo
>>
> bch_sb_crypt_init also reinitializes the crypt->key, so I can't just
> reuse it here.
>
Yeah, it will change crypt->key. It seems the main point is not clean
the password.
Do we need keep the crypt->key even the password is changed? Or does the
crypt->key like a random seed and need to be kept in the whole
lifecycle? Sorry I am a bit confused about this.

Thanks,
Hongbo

> I could extract it into a bch_crypt_update_passphrase function.
> Originally I wanted to avoid doing that to limit the scope of this
> patchset, but I'll give it a shot
>
> Thanks for the review!
>
> Mae
>
>>>> +    }
>>>> +
>>>>       struct bch_key passphrase_key = derive_passphrase(crypt,
>>>> new_passphrase);
>>>>   -    if (bch2_chacha_encrypt_key(&passphrase_key,
>>>> __bch2_sb_key_nonce(c->disk_sb.sb),
>>>> +    if (bch2_chacha_encrypt_key(&passphrase_key,
>>>> __bch2_sb_key_nonce(sb),
>>>>                       &new_key, sizeof(new_key)))
>>>>           die("error encrypting key");
>>>>       crypt->key = new_key;
>>>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>>>> index 32671bd8..30ad92d4 100644
>>>> --- a/c_src/crypto.c
>>>> +++ b/c_src/crypto.c
>>>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>       get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>>>         if (passphrase) {
>>>> -
>>>> -        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>> -        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>> -        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>> -        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>> +        bch_crypt_default_kdf_init(crypt);
>>>>             struct bch_key passphrase_key = derive_passphrase(crypt,
>>>> passphrase);
>>>>   @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>           memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>>>       }
>>>>   }
>>>> +
>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>>>> +        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>> +        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>> +        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>> +        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>> +}
>>>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>>>> index baea6d86..846a8931 100644
>>>> --- a/c_src/crypto.h
>>>> +++ b/c_src/crypto.h
>>>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const
>>>> char *,
>>>>   void bch2_add_key(struct bch_sb *, const char *, const char *,
>>>> const char *);
>>>>   void bch_sb_crypt_init(struct bch_sb *sb, struct
>>>> bch_sb_field_crypt *,
>>>>                  const char *);
>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>>>     #endif /* _CRYPTO_H */
>>>> --
>>>> 2.45.2
>>>>
>>>
>>>
>>