Re: races in ll_splice_alias() and elsewhere (ext4, ocfs2)

[Theodore Ts'o <tytso@mit.edu>]

On Fri, Mar 11, 2016 at 03:18:51AM +0000, Al Viro wrote:
> On Thu, Mar 10, 2016 at 06:55:43PM -0500, Theodore Ts'o wrote:
> 
> > The ext4_d_revalidate() function was an attempt to square the circle,
> > but yes, I've been coming to the conclusion that it doesn't work all
> > that well.  One thing I've been considering is to make access to the
> > keys a global property.  So the first time a user with access to the
> > key tries to access an encrypted file, we import the key into mounted
> > file system's ext4_sb_info structure, and we bump a generation
> > counter, and then ext4_d_revalidate() simply returns "invalid" for all
> > denrty entries which (a) refer to an encrypted file or directory, and
> > (b) whose generation number is less than the current generation
> > number.
> 
> That sounds rather DoSable, if I'm not misparsing you...

Well, it means that someone who can add and remove a key can force all
file system accesses for encrypted files to go through ext4_lookup(),
yes.  As I said, "non-optimal".  I suppose I could make the generation
number be one on a per-key-id basis to mitigate this problem.

> 
> > Similarly, if we invalidate a key, we remove the key from tthe keyring
> > hanging off of the mounted file system's sb_info structure, and then
> > bump the generation number, which will invalidate the dentries for all
> > encrypted files/directories on that file system.
> > 
> > This is a bit non-optimal, but I don't see any other way of solving
> > the problem.  Al, do you have any suggestions?
> 
> In any case, the current variant needs at least dget_parent()/dput() - blind
> access of ->d_parent is simply broken.  As for using ->d_revalidate() for
> that stuff...  I'm not sure.  How should those directories look like for
> somebody who doesn't have a key?  Should e.g. getdents(2) results depend on
> who's calling it, or who'd opened the directory, or...?

>From a security perspective, we are assuming that the kernel is
trusted, and that if the key is present, the kernel can be counted
upon to use Unix permissions to enforce access controls.  The use of
encryption is to prevent off-line attacks, and to limit the exposure
such that when a user is logged out (and their key is not present),
their data is not at risk.  Only the data for the logged-in user(s)
would be at risk.

Right now we do make the getdents(2) results depend on who opened the
directory --- although we probably should make it be based on who is
calling getdents(2) --- mainly because we don't want a inconsistencies
where getdents(2) returns the decrypted file names to a user who
doesn't have the key, and so attempts do a lookup fail (for example).

Or inconsistencies where the user tried to do a lookup for a file,
failed because the key hadn't been established yet at that point in
the login sequence, but then after the key has been loaded, since we
have a negative dentry caching the previous failure, future attempts
to lookup that file fail even though they now have the key.  This was
what the ext4_d_revalidate() was trying to address....

I realize that at some level we're doing something that the dentry
cache was never designed to handle; it fundamentally assumes that
there is a single global view of a file system hierarchy.  This is why
I've about making the availability (or not) of keys a global property,
and when that changes, we will need to revoke the cache since the
cached results might no longer be correct.

							- Ted