Date: Wed, 10 Jan 2018 20:48:01 -0800
From: Eric Biggers <ebiggers3@gmail.com>
To: =?iso-8859-1?Q?Andr=E9?= Draszik <git@andred.net>
Cc: linux-kernel@vger.kernel.org,
        Mimi Zohar <zohar@linux.vnet.ibm.com>,
        David Howells <dhowells@redhat.com>,
        James Morris <james.l.morris@oracle.com>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        "Theodore Y. Ts'o" <tytso@mit.edu>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Kees Cook <keescook@chromium.org>,
        linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH 3/3] encrypted-keys: document new fscrypt key format
Message-ID: <20180111044801.GB943@zzz.localdomain>
References: <20180110124418.24385-1-git@andred.net>
 <20180110124418.24385-3-git@andred.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20180110124418.24385-3-git@andred.net>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org

Hi Andr?,

On Wed, Jan 10, 2018 at 12:44:18PM +0000, Andr? Draszik wrote:
> diff --git a/Documentation/security/keys/fscrypt.rst b/Documentation/security/keys/fscrypt.rst
> new file mode 100644
> index 000000000000..e4a29592513e
> --- /dev/null
> +++ b/Documentation/security/keys/fscrypt.rst
> @@ -0,0 +1,67 @@
> +========================================
> +Encrypted keys for the fscrypt subsystem
> +========================================

There is now documentation for fscrypt in Documentation/filesystems/fscrypt.rst;
see in particular the "Adding keys" section.  The documentation for any new ways
to add keys should go in there.

> +
> +fscrypt allows file systems to implement transparent encryption and decryption
> +of files, similar to eCryptfs, using keys derived from a master key descriptor.

Note that the master key *descriptor* refers to the hex string used in the
keyring key description.  It is not the same as the master key itself, which is
stored in the payload.  The cryptography is done with the master key, not with
the master key *descriptor*.

> +In order to avoid known-plaintext attacks, the datablob obtained through
> +commands 'keyctl print' or 'keyctl pipe' does not contain the overall
> +fscrypt_key, the contents of which is well known, but only the master key
> +descriptor itself in encrypted form.
> +
> +The fscrypt subsystem may really benefit from using encrypted keys in that the
> +required key can be securely generated by an Administrator and provided at boot
> +time after the unsealing of a 'trusted' key in order to perform the mount in a
> +controlled environment.  Another advantage is that the key is not exposed to
> +threats of malicious software, because it is available in clear form only at
> +kernel level.

Please be very clear about exactly what security properties are achieved by
using encrypted-keys.  Note that such keys are present in the clear in kernel
memory, so they will be exposed by any exploit that compromises the kernel, or
even just finds a way to read its memory.  (And if you've been paying attention
in the last week, you may be aware that certain CPU vendors have "helpfully"
made reading kernel memory quite easy.)  So, it's definitely *not* categorically
true that "the key is not exposed to threats of malicious software".

Also note that fscrypt is already using the "logon" key type which cannot be
read by userspace (without exploits).  This is different from eCryptfs which
uses the "user" key type.

> +Usage::
> +
> +   keyctl add encrypted fscrypt:policy "new fscrypt key-type:master-key-name keylen" ring
> +   keyctl add encrypted fscrypt:policy "load hex_blob" ring
> +   keyctl update keyid "update key-type:master-key-name"
> +
> +Where::
> +
> +	policy:= '<16 hexadecimal characters>'
> +	key-type:= 'trusted' | 'user'
> +	keylen:= 16 | 32 | 64
> +
> +
> +Example of encrypted key usage with the fscrypt subsystem:
> +
> +Create an encrypted key "1234567890123456" of length 64 bytes with format
> +'fscrypt' and save it using a previously loaded user key "test"::
> +
> +    $ keyctl add encrypted fscrypt:1234567890123456 "new fscrypt user:test 64" @u
> +    1023935199
> +
> +    $ keyctl print 1023935199
> +    fscrypt user:test 64 e5606689fdc25d78a787249f4069fb3b007e992f4b21d0eda60
> +    c97986fc2e3326b5542e2b32216fc5007d9fd19cd3cb6668fa9850e954d2ba25e1b8a331
> +    1b0c1f20666c
> +
> +    $ keyctl pipe 1023935199 > fscrypt.blob

What is the point of having the kernel wrap a key with the "user" key type?  It
seems pointless; userspace could just do it instead.

Note that if you use keyctl_read() to read from an encrypted-key, you can
actually request that the key be encrypted using an arbitrary key of the type
with which the key is supposed to be wrapped.  This can be done by adding a key
to your thread/process/session keyring whose type and description matches the
intended wrapping key.  Thus, any "encrypted" key wrapped with a "user" key can
effectively be retrieved in the clear by calling keyctl_read(), then decrypting
the ciphertext in userspace.

Perhaps that's actually a bug; I don't know.  But using "user" wrapping keys
seems pretty pointless anyway.

I think it's really only "trusted" wrapping keys where this new feature would
have any useful security properties.  So the documentation needs to explain
that, and use that in the examples.

Eric