On 01/05/2020 07:39, Eric Biggers wrote:
[...]
Thanks for taking the time to look through this.
>
> This is a good idea, but can you explain exactly what security properties you
> aim to achieve?
My goal is to protect the file-system against offline modifications.
Offline in this context means when the filesystem is not mounted.
This could be a switched off laptop in a hotel room or a container
image, or a powered off embedded system. When the file-system is mounted
normal read/write access is possible.
> As far as I can tell, btrfs hashes each data block individually. There's no
> contextual information about where eaech block is located or which file(s) it
> belongs to. So, with this proposal, an attacker can still replace any data
> block with any other data block. Is that what you have in mind? Have you
> considered including contextual information in the hashes, to prevent this?
>
> What about metadata blocks -- how well are they authenticated? Can they be
> moved around? And does this proposal authenticate *everything* on the
> filesystem, or is there any part that is missed? What about the superblock?
In btrfs every metadata block is started by a checksum (see struct
btrfs_header or struct btrfs_super_block). This checksum protects the
whole meta-data block (minus the checksum field, obviously).
The two main structure of the trees are btrfs_node and btrfs_leaf (both
started by a btrfs_header). struct btrfs_node holds the generation and
the block pointers of child nodes (and leafs). Struct btrfs_leaf holds
the items, which can be inodes, directories, extents, checksums,
block-groups, etc...
As each FS meta-data item, beginning with the super block, downwards to
the meta-data items themselves is protected be a checksum, so the FS
tree (including locations, generation, etc) is protected by a checksum,
for which the attacker would need to know the key to generate.
The checksum for data blocks is saved in a separate on-disk btree, the
checksum tree. The structure of the checksum tree consists of
btrfs_leafs and btrfs_nodes as well, both of which do have a
btrfs_header and thus are protected by the checksums.
>
> You also mentioned preventing replay of filesystem operations, which suggests
> you're trying to achieve rollback protection. But actually this scheme doesn't
> provide rollback protection. For one, an attacker can always just rollback the
> entire filesystem to a previous state.
You're right, replay is the wrong wording there and it's actually
harmful in the used context. What I had in mind was, in order to change
the structure of the filesystem, an attacker would need the key to
update the checksums, otherwise the next read will detect a corruption.
As for a real replay case, an attacker would need to increase the
generation of the tree block, in order to roll back to a older state, an
attacker still would need to modify the super-block's generation, which
is protected by the checksum as well.
> This feature would still be useful even with the above limitations. But what is
> your goal exactly? Can this be made better?
>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index d10c7be10f3b..fe403fb62178 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -17,6 +17,7 @@
>> #include <linux/error-injection.h>
>> #include <linux/crc32c.h>
>> #include <linux/sched/mm.h>
>> +#include <keys/user-type.h>
>> #include <asm/unaligned.h>
>> #include <crypto/hash.h>
>> #include "ctree.h"
>> @@ -339,6 +340,7 @@ static bool btrfs_supported_super_csum(u16 csum_type)
>> case BTRFS_CSUM_TYPE_XXHASH:
>> case BTRFS_CSUM_TYPE_SHA256:
>> case BTRFS_CSUM_TYPE_BLAKE2:
>> + case BTRFS_CSUM_TYPE_HMAC_SHA256:
>> return true;
>> default:
>> return false;
>> @@ -2187,6 +2189,9 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
>> {
>> struct crypto_shash *csum_shash;
>> const char *csum_driver = btrfs_super_csum_driver(csum_type);
>> + struct key *key;
>> + const struct user_key_payload *ukp;
>> + int err = 0;
>>
>> csum_shash = crypto_alloc_shash(csum_driver, 0, 0);
>>
>> @@ -2198,7 +2203,53 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
>>
>> fs_info->csum_shash = csum_shash;
>>
>> - return 0;
>> + /*
>> + * if we're not doing authentication, we're done by now. Still we have
>> + * to validate the possible combinations of BTRFS_MOUNT_AUTH_KEY and
>> + * keyed hashes.
>> + */
>> + if (csum_type == BTRFS_CSUM_TYPE_HMAC_SHA256 &&
>> + !btrfs_test_opt(fs_info, AUTH_KEY)) {
>> + crypto_free_shash(fs_info->csum_shash);
>> + return -EINVAL;
>
> Seems there should be an error message here that says that a key is needed.
>
>> + } else if (btrfs_test_opt(fs_info, AUTH_KEY)
>> + && csum_type != BTRFS_CSUM_TYPE_HMAC_SHA256) {
>> + crypto_free_shash(fs_info->csum_shash);
>> + return -EINVAL;
>
> The hash algorithm needs to be passed as a mount option. Otherwise the attacker
> gets to choose it for you among all the supported keyed hash algorithms, as soon
> as support for a second one is added. Maybe use 'auth_hash_name' like UBIFS
> does?
Can you elaborate a bit more on that? As far as I know, UBIFS doesn't
save the 'auth_hash_name' on disk, whereas 'BTRFS_CSUM_TYPE_HMAC_SHA256'
is part of the on-disk format. As soon as we add a 2nd keyed hash, say
BTRFS_CSUM_TYPE_BLAKE2B_KEYED, this will be in the superblock as well,
as struct btrfs_super_block::csum_type.
>
>> + } else if (!btrfs_test_opt(fs_info, AUTH_KEY)) {
>> + /*
>> + * This is the normal case, if noone want's authentication and
>> + * doesn't have a keyed hash, we're done.
>> + */
>> + return 0;
>> + }
>> +
>> + key = request_key(&key_type_logon, fs_info->auth_key_name, NULL);
>> + if (IS_ERR(key))
>> + return PTR_ERR(key);
>
> Seems this should print an error message if the key can't be found.
>
> Also, this should enforce that the key description uses a service prefix by
> formatting it as kasprintf("btrfs:%s", fs_info->auth_key_name). Otherwise
> people can abuse this feature to use keys that were added for other kernel
> features. (This already got screwed up elsewhere, which makes the "logon" key
> type a bit of a disaster. But there's no need to make it worse.)
OK, will do.
