Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Mae Kasza <git@badat.dev>]

Hi,

Appologies if this is bad etiquette, but I haven't gotten any review 
feedback and it seems like fixing this is relatively important to 
users(https://github.com/koverstreet/bcachefs-tools/pull/329#issuecomment-2336787443). 
Any chance someone(Kent) could look at this and merge the patch?

Thanks,

Mae

On 8/14/24 14:25, Mae Kasza wrote:
>
> On 8/14/24 04:10, Hongbo Li wrote:
>>
>>
>> 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
>
> crypt->key does need to stay constant for the entire lifetime of a 
> partition, since that's what's actually used to encrypt the data on 
> the partition. The passphrase is just used to encrypt the crypt->key
>
> Thanks,
>
> Mae
>
>>
>>> 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
>>>>>>
>>>>>
>>>>>
>>>>