Re: [PATCH] crypto: AF_ALG - limit mask and type

[Stephan Mueller]

Am Dienstag, 12. Dezember 2017, 09:57:37 CET schrieb Eric Biggers:

Hi Eric,

> Hi Stephan,
> 
> On Tue, Dec 12, 2017 at 07:09:08AM +0100, Stephan Müller wrote:
> > Hi Herbert,
> > 
> > you see the reported problem by simply using
> > 
> > sa.salg_mask = 0xffffffff;
> > 
> > Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and
> > CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all
> > that user space should reach is potentially the ASYNC flag and the
> > cipher types flags.
> > 
> > ---8<---
> > 
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. Only a subset of the type and mask is
> > considered relevant to be set by user space if needed at all.
> > 
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> > 
> > Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx>
> 
> The syzkaller reproducer triggered a crash in crypto_remove_spawns().  Is it
> possible the bug is still there somewhere, while this patch just makes it
> inaccessible through AF_ALG?

I think the issue is that the syzkaller generates a vast amount of registered 
ciphers. At one point in time, I would think that some implied limit is 
overflown. But I cannot say for sure.

Yet, it is definitely a bug to have more than one instance of the same cipher 
implementation registered.

> 
> Anyway, we definitely should expose as few algorithm flags to AF_ALG as
> possible.  There are just way too many things that can go wrong with
> exposing arbitrary flags.

Absolutely, I would even say that we should not expose any mask/type at all. 
I.e. the patch I offered here should be changed to set the mask/type to zero 
in all cases.
> 
> However, why do the check in every af_alg_type.bind() method instead of just
> once in alg_bind()?

You are quite right, that is the right place to add this code as it contains 
already some verification there.
> 
> If it can be done without breaking users, it also would be nice if we would
> actually validate the flags and return -EINVAL if unknown flags are
> specified. Otherwise users cannot test for whether specific flags are
> supported.

If we (and we need to hear Herbert) conclude that these values should not be 
exposed in the first place, I think we should not return any error but simply 
set it to zero.

If Herbert concludes that some flags are necessary, we should build a white-
list and return an error for any flag that is not in the white list.
> 
> Also, note that even after this fix there are still ways to register an
> arbitrarily large number of algorithms.  There are two classes of problems.
> 
> First, it can happen that a template gets instantiated for a request but the
> resulting algorithm does not exactly match the original request, so making
> the same request again will instantiate the template again.  This could
> happen by specifically requesting an untested algorithm (type=0,
> mask=CRYPTO_ALG_TESTED), which your patch fixes. However this can also
> happen in cases where neither the final ->cra_name nor the final
> ->cra_driver_name matches what was requested. For example asking for
> "cryptd(sha1)" results in .cra_name = "sha1" and .cra_driver_name =
> "cryptd(sha1-avx2)", or asking for "xts(ecb(aes))" results in .cra_name =
> "xts(aes)" and .cra_driver_name = "xts(ecb-aes-aesni)".
> 
> Probably the crypto API needs to be taught how to find the instantiated
> templates correctly.

Maybe a name mangling should be removed. A template/cipher has only two names, 
period. Either you use exactly these names or you will not find a cipher.
> 
> Second, you can just keep choosing different combinations of algorithms when
> instantiating templates, taking advantage of the fact that templates can be
> nested and some take multiple parameters, so the number of possible
> combinations grows exponentially.  I don't know how to easily solve this. 
> Perhaps crypto_free_skcipher(), crypto_free_ahash(), etc. should unregister
> the algorithm if it was created from a template and nothing else is using
> it; then the number of algorithms someone could instantiate via AF_ALG at a
> given time would be limited by their number of file descriptors.

There could be a large set of permutations of ciphers, I agree. However, do 
you think that in case all of them are registered, we have an issue? The goal 
is that if one template/cipher combo is registered once, any subsequent 
allocation of that combo should reuse the registered instance.

PS: The cipher allocation function has another long-standing bug which could 
be viewed as a DoS via AF_ALG: Assume you do not yet have gcm(aes) allocated. 
Now, AF_ALG allocates gcm_base(ctr(aes), ghash), the registered cipher 
instance will have *both*, the name and the driver name to be set to 
gcm_base(ctr(aes), ghash). Any subsequent allocation of gcm(aes) (e.g. by 
IPSEC) will fail with -ENOENT even though the cipher is allocated. Note, 
gcm(aes) here is only an example -- this issue is a general problem.


Ciao
Stephan