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
![]() |