linux-ext4 - Re: [PATCH v2 5/5] fscrypt: fix race where ->lookup() marks plaintext dentry as ciphertext

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190417142435.GE4686@mit.edu>
Date:   Wed, 17 Apr 2019 10:24:35 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, linux-unionfs@...r.kernel.org,
        Sarthak Kukreti <sarthakkukreti@...omium.org>,
        Gao Xiang <gaoxiang25@...wei.com>
Subject: Re: [PATCH v2 5/5] fscrypt: fix race where ->lookup() marks
 plaintext dentry as ciphertext

On Wed, Mar 20, 2019 at 11:39:13AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> ->lookup() in an encrypted directory begins as follows:
> 
> 1. fscrypt_prepare_lookup():
>     a. Try to load the directory's encryption key.
>     b. If the key is unavailable, mark the dentry as a ciphertext name
>        via d_flags.
> 2. fscrypt_setup_filename():
>     a. Try to load the directory's encryption key.
>     b. If the key is available, encrypt the name (treated as a plaintext
>        name) to get the on-disk name.  Otherwise decode the name
>        (treated as a ciphertext name) to get the on-disk name.
> 
> But if the key is concurrently added, it may be found at (2a) but not at
> (1a).  In this case, the dentry will be wrongly marked as a ciphertext
> name even though it was actually treated as plaintext.
> 
> This will cause the dentry to be wrongly invalidated on the next lookup,
> potentially causing problems.  For example, if the racy ->lookup() was
> part of sys_mount(), then the new mount will be detached when anything
> tries to access it.  This is despite the mountpoint having a plaintext
> path, which should remain valid now that the key was added.
> 
> Of course, this is only possible if there's a userspace race.  Still,
> the additional kernel-side race is confusing and unexpected.
> 
> Close the kernel-side race by changing fscrypt_prepare_lookup() to also
> set the on-disk filename (step 2b), consistent with the d_flags update.
> 
> Fixes: 28b4c263961c ("ext4 crypto: revalidate dentry after adding or removing the key")
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>

Looks good, applied.

					- Ted