On Tue, Apr 18, 2017 at 06:42:09PM -0700, Jaegeuk Kim wrote:
> Hi Eric,
>
> On 04/18, Eric Biggers wrote:
> > On Tue, Apr 18, 2017 at 04:01:36PM -0700, Eric Biggers wrote:
> > >
> > > Strangely, f2fs and ubifs do not use the bytes from the filename at all when
> > > trying to find a specific directory entry in this case. So this patch doesn't
> > > really affect them. This seems unreliable; perhaps we should introduce a
> > > function like "fscrypt_name_matches()" which all the filesystems could call?
> > > Can any of the f2fs and ubifs developers explain why they don't look at any
> > > bytes from the filename?
> > >
>
> The fscrypt_setup_filename sets fname->hash in the bigname case, but doesn't
> give fname->disk_name. If it's not such the bigname case, we check its name
> since fname->hash is zero.
>
Yes, that's what it does now. The question is, in the "bigname" case why
doesn't f2fs check the 16 bytes of ciphertext in fname->crypto_buf too? f2fs
doesn't even use 'minor_hash'; it can't possibly be the case that there are
never any collisions of a 32-bit hash in a directory, can it?
I actually tested it, and it definitely happens if you put a lot of files in an
encrypted directory on f2fs. An example with 100000 files:
# seq -f "edir/abcdefghijklmnopqrstuvwxyz012345%.0f" 100000 | xargs touch
# find edir -type f | xargs stat -c %i | sort | uniq | wc -l
100000
# sync
# echo 3 > /proc/sys/vm/drop_caches
# keyctl new_session
# find edir -type f | xargs stat -c %i | sort | uniq | wc -l
99999
So when I tried accessing the encrypted directory without the key, two dentries
showed the same inode, due to a hash collision.
Actually, checking the last 16 bytes of ciphertext currently wouldn't even help
for those filenames since it's all the same, as they share a long common prefix:
# ls -1 edir | head -n 4
_++09VCAAAAgsQQf6Q5YgLgoO4f3PPSfb
_++1UWDAAAAgsQQf6Q5YgLgoO4f3PPSfb
_++2HAAAAAAgsQQf6Q5YgLgoO4f3PPSfb
_++4UxBAAAAgsQQf6Q5YgLgoO4f3PPSfb
But that's the bug, since the last two AES blocks are swapped when using
ciphertext stealing. We should at least be using the second-to-last block in
which case we'd see:
# ls -1 edir | head -n 4
_++09VCAAAAw9VONwQEXOVv3RR,kOAKwB
_++1UWDAAAAAHDi7c3QZxbiltjOo1m0,F
_++2HAAAAAAAfd1Vx0oC31SmhzYpaYfwz
_++4UxBAAAAwZxcWjzORdAef50FB9sKY4
(In either case there are still a few A's at the beginning since f2fs doesn't
set 'minor_hash'. That's okay, but only if collisions are ruled out by other
means.)
> > - /* encrypted case */
> > - de_name.name = d->filename[bit_pos];
> > - de_name.len = le16_to_cpu(de->name_len);
> > -
> > - /* show encrypted name */
> > - if (fname->hash) {
> > - if (de->hash_code == cpu_to_le32(fname->hash))
> > - goto found;
> > - } else if (de_name.len == name->len &&
> > - de->hash_code == namehash &&
> > - !memcmp(de_name.name, name->name, name->len))
> > + if ((fname->hash == 0 ||
> > + fname->hash == le32_to_cpu(de->hash_code)) &&
> > + fscrypt_name_matches(fname, d->filename[bit_pos],
> > + le16_to_cpu(de->name_len)))
>
> BTW, this slips checking namehash?
>
Yes that's a mistake. Actually it seems that 'namehash' is the same as
'fname->hash' when 'fname->hash' is nonzero, so the code should just be:
if (de->hash_code == namehash &&
fscrypt_name_matches(fname, d->filename[bit_pos],
le16_to_cpu(de->name_len)))
goto found;
- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|