Re: [PATCH] Extract bch_crypt_update_passphrase function

[Mae Kasza <admin@badat.dev>]

On 8/14/24 04:21, Hongbo Li wrote:
>
>
> On 2024/8/14 1:37, Mae Kasza wrote:
>> This also fixes a bug where the set-passphrase
>> command failed to derive the key for disks initially
>> formatted with --encrypted and --no_passphrase, due to
>> bch_sb_crypt_init not configuring the KDF params if
>> the passphrase wasn't specified during formatting.
>>
>> Signed-off-by: Mae Kasza <git@badat.dev>
>> ---
>>   c_src/cmd_key.c | 26 ++++++++++----------------
>>   c_src/crypto.c  | 43 ++++++++++++++++++++++++++++++++-----------
>>   c_src/crypto.h  |  3 +++
>>   3 files changed, 45 insertions(+), 27 deletions(-)
>>
>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>> index adb0ac8d..ac8a94a8 100644
>> --- a/c_src/cmd_key.c
>> +++ b/c_src/cmd_key.c
>> @@ -104,24 +104,19 @@ 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");
>>   -    struct bch_encrypted_key new_key;
>> -    new_key.magic = BCH_KEY_MAGIC;
>> -
>> -    int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
>> +    struct bch_key key;
>> +    int ret = bch2_decrypt_sb_key(c, crypt, &key);
>>       if (ret)
>>           die("Error getting current key");
>>         char *new_passphrase = read_passphrase_twice("Enter new 
>> passphrase: ");
>> -    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),
>> -                    &new_key, sizeof(new_key)))
>> -        die("error encrypting key");
>> -    crypt->key = new_key;
>> +    bch_crypt_update_passphrase(sb, crypt, &key, new_passphrase);
>>         bch2_revoke_key(c->disk_sb.sb);
>>       bch2_write_super(c);
>> @@ -142,18 +137,17 @@ int cmd_remove_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");
>>   -    struct bch_encrypted_key new_key;
>> -    new_key.magic = BCH_KEY_MAGIC;
>> -
>> -    int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
>> +    struct bch_key key;
>> +    int ret = bch2_decrypt_sb_key(c, crypt, &key);
>>       if (ret)
>>           die("Error getting current key");
>>   -    crypt->key = new_key;
>> +    bch_crypt_update_passphrase(sb, crypt, &key, NULL);
>>         bch2_write_super(c);
>>       bch2_fs_stop(c);
>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>> index 32671bd8..301dbebe 100644
>> --- a/c_src/crypto.c
>> +++ b/c_src/crypto.c
>> @@ -176,26 +176,47 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>                  struct bch_sb_field_crypt *crypt,
>>                  const char *passphrase)
>>   {
>> +    struct bch_key key;
>> +    get_random_bytes(&key, sizeof(key));
>> +
>>       crypt->key.magic = BCH_KEY_MAGIC;
>> -    get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>> +    crypt->key.key = key;
>>   -    if (passphrase) {
>> +    bch_crypt_update_passphrase(sb, crypt, &key, passphrase);
>> +}
>>   +void bch_crypt_update_passphrase(
>> +            struct bch_sb *sb,
>> +            struct bch_sb_field_crypt *crypt,
>> +            struct bch_key *key,
>> +            const char *new_passphrase)
>> +{
>> +
>> +    struct bch_encrypted_key new_key;
>> +    new_key.magic = BCH_KEY_MAGIC;
>> +    new_key.key = *key;
>> +
>> +    if(!new_passphrase) {
>> +        crypt->key = new_key;
> May be the helper like this is needed. But we could assign the 
> crypt->key outside, which seems more reasonable according to the 
> helper name. Just in my humble opinion.
>
> Thanks,
> Hongbo

We need to assign to crypt->key regardless, since it's what contains the 
key encrypted with passphrase, I don't really see why we would want to 
leave that up to the caller

Thanks,

Mae

>
>> +        return;
>> +    }
>> +
>> +    // If crypt already has an encrypted key reuse it's encryption 
>> params
>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>           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));
>> +    }
>>   -        struct bch_key passphrase_key = derive_passphrase(crypt, 
>> passphrase);
>> -
>> -        assert(!bch2_key_is_encrypted(&crypt->key));
>> +    struct bch_key passphrase_key = derive_passphrase(crypt, 
>> new_passphrase);
>>   -        if (bch2_chacha_encrypt_key(&passphrase_key, 
>> __bch2_sb_key_nonce(sb),
>> -                       &crypt->key, sizeof(crypt->key)))
>> -            die("error encrypting key");
>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>> __bch2_sb_key_nonce(sb),
>> +                    &new_key, sizeof(new_key)))
>> +        die("error encrypting key");
>>   -        assert(bch2_key_is_encrypted(&crypt->key));
>> +    memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>   -        memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>> -    }
>> +    crypt->key = new_key;
>> +    assert(bch2_key_is_encrypted(&crypt->key));
>>   }
>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>> index baea6d86..cd3baac8 100644
>> --- a/c_src/crypto.h
>> +++ b/c_src/crypto.h
>> @@ -19,4 +19,7 @@ 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_update_passphrase(struct bch_sb *sb, struct 
>> bch_sb_field_crypt *crypt,
>> +            struct bch_key *key, const char *new_passphrase);
>> +
>>   #endif /* _CRYPTO_H */