Re: [PATCH v2] crypto: AF_ALG - limit mask and type
On Fri, Dec 22, 2017 at 08:41:10AM +0100, Stephan Mueller wrote: > > Shouldn't we then rather use a white list instead of a black list? > > > > Most other problems however would be bugs in the template code. > > The first thing a template does when it creates an instance is > > to check whether the resulting algorithm would fulfil the requested > > type/mask using crypto_check_attr_type. So if that's not working > > then we should fix it there as it may also be triggered via other > > code paths that can create instances. > > I think I understand that, but does user space need to utilize this logic? I'm open to a white list as well. But we should certainly fix the underlying bugs rather than just papering over them as they could come back via other channels. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] crypto: AF_ALG - limit mask and type
Am Freitag, 22. Dezember 2017, 08:36:07 CET schrieb Herbert Xu: Hi Herbert, > On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote: > > The user space interface allows specifying the type and the mask field > > used to allocate the cipher. As user space can precisely select the > > desired cipher by using either the name or the driver name, additional > > selection options for cipher are not considered necessary and relevant > > for user space. > > > > 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 > > Cc: > > Signed-off-by: Stephan Mueller > > This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY. I think > we should add CRYPTO_ALG_TESTED to the blacklist since there is > no sane reason to use it here. Shouldn't we then rather use a white list instead of a black list? > > Most other problems however would be bugs in the template code. > The first thing a template does when it creates an instance is > to check whether the resulting algorithm would fulfil the requested > type/mask using crypto_check_attr_type. So if that's not working > then we should fix it there as it may also be triggered via other > code paths that can create instances. I think I understand that, but does user space need to utilize this logic? > > Thanks, Ciao Stephan
Re: [PATCH v2] crypto: AF_ALG - limit mask and type
On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote: > The user space interface allows specifying the type and the mask field > used to allocate the cipher. As user space can precisely select the > desired cipher by using either the name or the driver name, additional > selection options for cipher are not considered necessary and relevant > for user space. > > 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 > Cc: > Signed-off-by: Stephan Mueller This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY. I think we should add CRYPTO_ALG_TESTED to the blacklist since there is no sane reason to use it here. Most other problems however would be bugs in the template code. The first thing a template does when it creates an instance is to check whether the resulting algorithm would fulfil the requested type/mask using crypto_check_attr_type. So if that's not working then we should fix it there as it may also be triggered via other code paths that can create instances. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH v2] crypto: AF_ALG - limit mask and type
The user space interface allows specifying the type and the mask field used to allocate the cipher. As user space can precisely select the desired cipher by using either the name or the driver name, additional selection options for cipher are not considered necessary and relevant for user space. 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 Cc: Signed-off-by: Stephan Mueller --- crypto/af_alg.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crypto/af_alg.c b/crypto/af_alg.c index 1e5353f62067..4f4cfc5a7ef3 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -150,7 +150,6 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent); static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) { - const u32 forbidden = CRYPTO_ALG_INTERNAL; struct sock *sk = sock->sk; struct alg_sock *ask = alg_sk(sk); struct sockaddr_alg *sa = (void *)uaddr; @@ -176,9 +175,12 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) if (IS_ERR(type)) return PTR_ERR(type); - private = type->bind(sa->salg_name, -sa->salg_feat & ~forbidden, -sa->salg_mask & ~forbidden); + /* +* The use of the salg_feat and salg_mask are forbidden as they expose +* too much of the low-level handling which is not suitable for +* hostile code. +*/ + private = type->bind(sa->salg_name, 0, 0); if (IS_ERR(private)) { module_put(type->owner); return PTR_ERR(private); -- 2.14.3