Hi Eric,
Thanks for the constructive feedback, pls see inline below.
On 09/17/2016 02:58 PM, Eric Biggers wrote:
On Tue, Sep 13, 2016 at 09:39:46PM +0800, Anand Jain wrote:
This patchset adds btrfs encryption support.
Hi Anand,
I'm part of a team that will be maintaining and improving ext4 encryption.
Because f2fs now shares much of the code, it will benefit from the ext4
encryption work too. It would be really nice if btrfs could be in the same
boat, since that would avoid redundant work and help prevent bugs and design
flaws. So I strongly suggest that you explore how btrfs encryption might reuse
the existing code (and maybe extend it if there is very good reason to).
In fact my first attempt was using f2fs/ext4, found its too complicated,
further couldn't stable it, so re-wrote completely to a version where I
won't worry too much on the cipher mode _at the moment_, so this version
came with a caveat as mentioned.
Now looking to integrate with fs/crypto, however have the following
concerns,
fs/crypto:
.
fscrypt_context:master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
should it go to the disk ? its just a descriptor which might
change at the user end and still may contain the right key
in the payload.
btrfs keeps it only in-memory and key hash goes to the disk.
Further in the long we need an integration with key management
system as well.
.
ext4/f2fs allows per file keys, when we are talking about large
data center FS for cloud services, it would need key services
to scale along with the FS. And there will be a lot of resources
which is allocated but not used.
.
system keyring already defines struct user_key_payload
probably we should have used it instead of
71 struct fscrypt_key {
72 u32 mode;
73 u8 raw[FS_MAX_KEY_SIZE];
74 u32 size;
75 } __packed;
.
Some of key derivation functions should have been part of
crypto library rather.
.
page->index based IV won't suite btrfs, so it uses a random, but yes
it needs crypto hardened. I am kind of opinion that, for a real need
of retrievable random number we are replacing it with sector(truecrypt)
/page-offset number, I think we aren't addressing problem in a right
way ? If that that's the best solution we could achieve, yet I am not
sure how to solve the need of FS independent decrypt ? I am yet to
look at gpg.
.
Yes needs AEAD. But at the same time we need
- MAC to be separated from the ciphertext and ciphertext-size
== plaintext-size.
To make sure for sync,dio we do create extents and IO which matches
with the application IO. So that performance tuning will be things
as usual.
There also needs to be a proper design document for btrfs encryption. This is
especially true if for some (very, very good) reason you can't reuse the
infrastructure from ext4 and f2fs. There also could be unique challenges for
btrfs such as encryption keys and/or IVs being reused in reflinked extents.
You will also not get a proper review without a proper design document which
details things like the threat model and the security properties provided. But
I did take a short look at the code anyway because I was interested.
Thank You !! I should have sent the code when doc is ready rather.
Sorry about that.
The
results were not pretty. As far as I can see the current proposal is fatally
flawed as it does not provide confidentiality of file contents against a basic
attack.
The main two flaws are:
1. Use of CTR mode of operation
2. Reuse of same (key, IV) pair for all pages of a given inode
CTR mode is well known to fall over completely when used with repeated (key, IV)
pairs. This makes the encryption nearly worthless. In more detail: suppose I
am an attacker who has access to a file's ciphertext. By the definition of CTR
mode, each ciphertext block is given by: C = E(ctr) XOR P, where C and P denote
the ciphertext and plaintext blocks respectively, E denotes encryption with the
block cipher using the secret key, and 'ctr' denotes the counter value. Due to
flaw (2) the ctr values repeat every page. Consequently, if I can correctly
guess the plaintext P1 of *any* page in the file and I want to know the
plaintext P2 of some other page, I can trivially compute P2 = P1 XOR C1 XOR C2.
No secret key needed.
Essentially: if there is any part of a file which is easily guessable, such as
a header or even a zeroed region, then the whole file is revealed.
Yes this will be fixed. No TFM is claimed to be btrfs default
as of now.
The solution is to use a less brittle mode of operation such as XTS in
combination with per-page IVs (or "tweaks") and derived per-file keys. This is
already done in ext4 and f2fs, where the per-page IV is just the page offset.
Note that per-file keys were needed to prevent the same (key, IV) pair from
being used in multiple places. So if you could reuse the fs/crypto
infrastructure, you could take advantage of the fact that this problem was
already solved.
Note: even better would be an authenticated encryption mode. That isn't yet
done by ext4 or f2fs --- I think because there wasn't a good place to store a
per-page authentication tag. It would be interesting to know whether this would
be possible for btrfs.
Yes. That's possible in btrfs. Encoders integration was planned quite
early in the design. There are reserved spaces in the extent items.
I should attempt straight to GCM AEAD then.
I also noticed that unlike ext4 and f2fs, filenames and symlinks are not being
encrypted in btrfs. I know it may seem somewhat ad-hoc that filenames are
encrypted but not other metadata, but apparently filenames were considered
quite important and a lot of work went into making it possible to encrypt them
in ext4.
Can we use inode number as file name when there is no key ?
And save real file name as encrypted attribute, a bit neater
though.
(Apologies if I misunderstood anything. The proposal would be easier to review
with a design document, as mentioned.)
Nope your understanding is correct. Apologies I should have
sent code later when doc is ready.
Thanks, Anand
Hope this is helpful,
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
