linux-ext4 - Re: [PATCH] fscrypt / ext4: make test_dummy

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: <20170105151059.egz6an7wfirygdr6@thunk.org>
Date:   Thu, 5 Jan 2017 10:10:59 -0500
From:   Theodore Ts'o <tytso@....edu>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     linux-ext4@...r.kernel.org, Jaegeuk Kim <jaegeuk@...nel.org>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        linux-fsdevel@...r.kernel.org, Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] fscrypt / ext4: make test_dummy_encryption require a
 keyring key

On Wed, Jan 04, 2017 at 04:16:06PM -0800, Eric Biggers wrote:
> I'm fine with your proposed version, though I'm not convinced it's really any
> better than mine, since it basically just moves the "hack" from
> fscrypt_inherit_context() to fscrypt_get_encryption_info().  The reason I
> preferred it in fscrypt_inherit_context() was that allowing
> fscrypt_get_encryption_info() to work on unencrypted files is kind of weird and
> could allow for confusing scenarios where a previously existing unencrypted file
> is accidentally treated as an encrypted one --- though that would require a
> missing ext4_encrypted_inode() check of course.

Except that you *always* need to call ext4_encrypt_inode() before you
call fscrypt_get_encryption_info(), because otherwise it becomes a
performance disaster in the no encryption case, because we would be
constantly doing failing xattr lookups.

It also made for some especially tangled logic, which I noticed when
you had to make a change in fs/ext4/namei.c:

        if ((ext4_encrypted_inode(dir) ||  <-----------------------
             DUMMY_ENCRYPTION_ENABLED(EXT4_SB(dir->i_sb))) &&
            (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))) {
-               err = fscrypt_get_encryption_info(dir);
-               if (err)
-                       return ERR_PTR(err);
-               if (!fscrypt_has_encryption_key(dir))
-                       return ERR_PTR(-EPERM);
+               if (ext4_encrypted_inode(dir)) {   <-------------------
+                       err = fscrypt_get_encryption_info(dir);
+                       if (err)
+                               return ERR_PTR(err);
+                       if (!fscrypt_has_encryption_key(dir))
+                               return ERR_PTR(-EPERM);
+               }
                if (!handle)


Your patch required the addition of a *second* call to
ext4_encrypted_inode() or else the call to
fscrypt_get_encryption_info() would fail in the test_dummy_encryption
case.

Not having hacks in the fscrypt_inherit_context() case also has the
happy advantage that we test the normal context inheritance code path
when creating files in the (unencrypted) root directory.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html