Modular BIG_KEYS (was: Re: [PATCH v4] security/keys: rewrite all of big_key crypto)

Modular BIG_KEYS (was: Re: [PATCH v4] security/keys: rewrite all of big_key crypto)

[Geert Uytterhoeven]

Hi Jason,

On Sat, Sep 16, 2017 at 3:00 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> This started out as just replacing the use of crypto/rng with
> get_random_bytes_wait, so that we wouldn't use bad randomness at boot time.
> But, upon looking further, it appears that there were even deeper
> underlying cryptographic problems, and that this seems to have been
> committed with very little crypto review. So, I rewrote the whole thing,
> trying to keep to the conventions introduced by the previous author, to
> fix these cryptographic flaws.
>
> It makes no sense to seed crypto/rng at boot time and then keep
> using it like this, when in fact there's already get_random_bytes_wait,
> which can ensure there's enough entropy and be a much more standard way
> of generating keys. Since this sensitive material is being stored untrusted,
> using ECB and no authentication is simply not okay at all. I find it
> surprising and a bit horrifying that this code even made it past basic
> crypto review, which perhaps points to some larger issues. This patch
> moves from using AES-ECB to using AES-GCM. Since keys are uniquely generated
> each time, we can set the nonce to zero. There was also a race condition in
> which the same key would be reused at the same time in different threads. A
> mutex fixes this issue now. And, some error paths forgot to zero out sensitive
> material, so this patch changes a kfree into a kzfree.
>
> So, to summarize, this commit fixes the following vulnerabilities:
>
>   * Low entropy key generation, allowing an attacker to potentially
>     guess or predict keys.
>   * Unauthenticated encryption, allowing an attacker to modify the
>     cipher text in particular ways in order to manipulate the plaintext,
>     which is is even more frightening considering the next point.
>   * Use of ECB mode, allowing an attacker to trivially swap blocks or
>     compare identical plaintext blocks.
>   * Key re-use.
>   * Faulty memory zeroing.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -45,10 +45,8 @@ config BIG_KEYS
>         bool "Large payload keys"
>         depends on KEYS
>         depends on TMPFS
> -       depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
>         select CRYPTO_AES
> -       select CRYPTO_ECB
> -       select CRYPTO_RNG
> +       select CRYPTO_GCM
>         help
>           This option provides support for holding large keys within the kernel
>           (for example Kerberos ticket caches).  The data may be stored out to

Now this has hit mainline, the "BIG_KEYS" Kconfig symbol appeared on my
radar. Is there any reason this cannot be tristate?

The help text says:

    This option provides support for holding large keys within the kernel
    (for example Kerberos ticket caches).  The data may be stored out to
    swapspace by tmpfs.

    If you are unsure as to whether this is required, answer N.

So to save kernel size, I wan't to save N, but for a distro kernel that might
have Kerberos users, you currently need to say Y, while M would be nicer.

The symbol seems to just control the build of security/keys/big_key.c,
which could use module_init() in the modular case.
I'm not sending a patch to change BIG_KEYS from bool to tristate, as this is
crypto, and I don't understand the full implications.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Modular BIG_KEYS (was: Re: [PATCH v4] security/keys: rewrite all of big_key crypto)

[Eric Biggers]

On Mon, Oct 02, 2017 at 09:14:36AM +0200, Geert Uytterhoeven wrote:
> Now this has hit mainline, the "BIG_KEYS" Kconfig symbol appeared on my
> radar. Is there any reason this cannot be tristate?
> 
> The help text says:
> 
>     This option provides support for holding large keys within the kernel
>     (for example Kerberos ticket caches).  The data may be stored out to
>     swapspace by tmpfs.
> 
>     If you are unsure as to whether this is required, answer N.
> 
> So to save kernel size, I wan't to save N, but for a distro kernel that might
> have Kerberos users, you currently need to say Y, while M would be nicer.
> 
> The symbol seems to just control the build of security/keys/big_key.c,
> which could use module_init() in the modular case.
> I'm not sending a patch to change BIG_KEYS from bool to tristate, as this is
> crypto, and I don't understand the full implications.
> 

It's possible to have a key type in a module.  In fact, some of the existing key
types such as key_type_rxrpc can already be modular.  But I don't think it
really works as intended currently because there is no autoloading of key type
modules.  That is, if big_key was a module and you tried to add a key of type
"big_key", the big_key module would *not* be automatically loaded, so the call
would return -ENODEV.  This could be fixed, I believe.

(I also still need to convince myself that there aren't any race conditions in
key type unregistering.  It's a little weird how it changes the key type to the
".dead" key type, rather than pinning the key type in memory while it's still
used.)

Eric

Re: Modular BIG_KEYS (was: Re: [PATCH v4] security/keys: rewrite all of big_key crypto)

[David Howells]

Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Now this has hit mainline, the "BIG_KEYS" Kconfig symbol appeared on my
> radar. Is there any reason this cannot be tristate?

It was tristate, but it got converted to bool:

	commit 2eaf6b5dcafda2b8c22930eff7f48a364fce1741
	KEYS: Make BIG_KEYS boolean

and then:

	commit a1f2bdf338f15dbad10ee6362891ebf79244858b
	security/keys: make big_key.c explicitly non-modular

> So to save kernel size, I wan't to save N, but for a distro kernel that might
> have Kerberos users, you currently need to say Y, while M would be nicer.

If you want to do that, you'll need to implement demand-loading of key type
modules.

Note that you'd end up using a minimum of a whole page as a module rather than
~2K if built in (I know, that's a compromise you have to decide on).

David

Re: Modular BIG_KEYS (was: Re: [PATCH v4] security/keys: rewrite all of big_key crypto)

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> (I also still need to convince myself that there aren't any race conditions
> in key type unregistering.  It's a little weird how it changes the key type
> to the ".dead" key type, rather than pinning the key type in memory while
> it's still used.)

Keys are converted to the dead type and their destructors called by the gc
whilst it holds a write lock on the key's semaphore, so keyctl() calls
shouldn't be a problem as they all hold a read-lock or a write-lock on the
content.

/proc/keys does things under the key_serial_lock and RCU, so if a key type's
->describe() function looks at the payload, then it should use RCU conditions.
Note that this doesn't affect the keyring_describe() as that only looks at a
value that is stored in the key struct - and the same for user_describe() and
big_key_describe().  asymmetric_key_describe(), OTOH...  Maybe the simplest
thing to do is to take key_serial_lock in key_garbage_collector() around the
code that changes the key type.

Inside the kernel, things are murkier as the kernel can reach inside a key
without taking the semaphore.  If it does so, it must use RCU or some other
mechanism to prevent the key payload from being destroyed under it.

With AF_RXRPC, I think I'm doing this wrong.  I think I need to move the
destruction of the key type to after I've unregistered the network namespace
handling as the latter destroys connections which might be using the key.

David