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

[Herbert Xu]

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

[Herbert Xu]

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

[Stephan Mueller]

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

[Stephan Mueller]

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

[Herbert Xu]

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

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

[Herbert Xu]

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

[Stephan Müller]

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

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

[Stephan Müller]

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