[Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1759791

== Bionic Justification ==
Mainline commit 9fa68f620041 introduced a regression in Bionic.  Bionic
got this commit with the 4.15.4 updates as commit 46e8d06.  

This bug causes the NFS mounts with kerberos set up to stopped working.
 
A proper patch is being discussed upstream, but it has not landed in mainline
as of yet:
https://patchwork.kernel.org/patch/10311831/


== Fix ==
A revert of commit:
9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")

Joseph Salisbury (1):
  Revert "crypto: hash - prevent using keyed hashes without setting key"

 crypto/ahash.c         | 22 +++++----------------
 crypto/algif_hash.c    | 52 +++++++++++++++++++++++++++++++++++++++-----------
 crypto/shash.c         | 25 ++++--------------------
 include/crypto/hash.h  | 34 ++++++++++-----------------------
 include/linux/crypto.h |  2 --
 5 files changed, 60 insertions(+), 75 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic][PATCH 1/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1759791

This reverts commit 9fa68f620041be04720d0cbfb1bd3ddfc6310b24.
---
 crypto/ahash.c         | 22 +++++----------------
 crypto/algif_hash.c    | 52 +++++++++++++++++++++++++++++++++++++++-----------
 crypto/shash.c         | 25 ++++--------------------
 include/crypto/hash.h  | 34 ++++++++++-----------------------
 include/linux/crypto.h |  2 --
 5 files changed, 60 insertions(+), 75 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 266fc1d..d2c8895 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -193,18 +193,11 @@ int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
  unsigned int keylen)
 {
  unsigned long alignmask = crypto_ahash_alignmask(tfm);
- int err;
 
  if ((unsigned long)key & alignmask)
- err = ahash_setkey_unaligned(tfm, key, keylen);
- else
- err = tfm->setkey(tfm, key, keylen);
-
- if (err)
- return err;
+ return ahash_setkey_unaligned(tfm, key, keylen);
 
- crypto_ahash_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
- return 0;
+ return tfm->setkey(tfm, key, keylen);
 }
 EXPORT_SYMBOL_GPL(crypto_ahash_setkey);
 
@@ -375,12 +368,7 @@ EXPORT_SYMBOL_GPL(crypto_ahash_finup);
 
 int crypto_ahash_digest(struct ahash_request *req)
 {
- struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
-
- if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
- return crypto_ahash_op(req, tfm->digest);
+ return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
 }
 EXPORT_SYMBOL_GPL(crypto_ahash_digest);
 
@@ -462,6 +450,7 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
  struct ahash_alg *alg = crypto_ahash_alg(hash);
 
  hash->setkey = ahash_nosetkey;
+ hash->has_setkey = false;
  hash->export = ahash_no_export;
  hash->import = ahash_no_import;
 
@@ -476,8 +465,7 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 
  if (alg->setkey) {
  hash->setkey = alg->setkey;
- if (!(alg->halg.base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY))
- crypto_ahash_set_flags(hash, CRYPTO_TFM_NEED_KEY);
+ hash->has_setkey = true;
  }
  if (alg->export)
  hash->export = alg->export;
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 6c9b192..76d2e71 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -34,6 +34,11 @@ struct hash_ctx {
  struct ahash_request req;
 };
 
+struct algif_hash_tfm {
+ struct crypto_ahash *hash;
+ bool has_key;
+};
+
 static int hash_alloc_result(struct sock *sk, struct hash_ctx *ctx)
 {
  unsigned ds;
@@ -302,7 +307,7 @@ static int hash_check_key(struct socket *sock)
  int err = 0;
  struct sock *psk;
  struct alg_sock *pask;
- struct crypto_ahash *tfm;
+ struct algif_hash_tfm *tfm;
  struct sock *sk = sock->sk;
  struct alg_sock *ask = alg_sk(sk);
 
@@ -316,7 +321,7 @@ static int hash_check_key(struct socket *sock)
 
  err = -ENOKEY;
  lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
- if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+ if (!tfm->has_key)
  goto unlock;
 
  if (!pask->refcnt++)
@@ -407,17 +412,41 @@ static struct proto_ops algif_hash_ops_nokey = {
 
 static void *hash_bind(const char *name, u32 type, u32 mask)
 {
- return crypto_alloc_ahash(name, type, mask);
+ struct algif_hash_tfm *tfm;
+ struct crypto_ahash *hash;
+
+ tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+ if (!tfm)
+ return ERR_PTR(-ENOMEM);
+
+ hash = crypto_alloc_ahash(name, type, mask);
+ if (IS_ERR(hash)) {
+ kfree(tfm);
+ return ERR_CAST(hash);
+ }
+
+ tfm->hash = hash;
+
+ return tfm;
 }
 
 static void hash_release(void *private)
 {
- crypto_free_ahash(private);
+ struct algif_hash_tfm *tfm = private;
+
+ crypto_free_ahash(tfm->hash);
+ kfree(tfm);
 }
 
 static int hash_setkey(void *private, const u8 *key, unsigned int keylen)
 {
- return crypto_ahash_setkey(private, key, keylen);
+ struct algif_hash_tfm *tfm = private;
+ int err;
+
+ err = crypto_ahash_setkey(tfm->hash, key, keylen);
+ tfm->has_key = !err;
+
+ return err;
 }
 
 static void hash_sock_destruct(struct sock *sk)
@@ -432,10 +461,11 @@ static void hash_sock_destruct(struct sock *sk)
 
 static int hash_accept_parent_nokey(void *private, struct sock *sk)
 {
- struct crypto_ahash *tfm = private;
- struct alg_sock *ask = alg_sk(sk);
  struct hash_ctx *ctx;
- unsigned int len = sizeof(*ctx) + crypto_ahash_reqsize(tfm);
+ struct alg_sock *ask = alg_sk(sk);
+ struct algif_hash_tfm *tfm = private;
+ struct crypto_ahash *hash = tfm->hash;
+ unsigned len = sizeof(*ctx) + crypto_ahash_reqsize(hash);
 
  ctx = sock_kmalloc(sk, len, GFP_KERNEL);
  if (!ctx)
@@ -448,7 +478,7 @@ static int hash_accept_parent_nokey(void *private, struct sock *sk)
 
  ask->private = ctx;
 
- ahash_request_set_tfm(&ctx->req, tfm);
+ ahash_request_set_tfm(&ctx->req, hash);
  ahash_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
    crypto_req_done, &ctx->wait);
 
@@ -459,9 +489,9 @@ static int hash_accept_parent_nokey(void *private, struct sock *sk)
 
 static int hash_accept_parent(void *private, struct sock *sk)
 {
- struct crypto_ahash *tfm = private;
+ struct algif_hash_tfm *tfm = private;
 
- if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+ if (!tfm->has_key && crypto_ahash_has_setkey(tfm->hash))
  return -ENOKEY;
 
  return hash_accept_parent_nokey(private, sk);
diff --git a/crypto/shash.c b/crypto/shash.c
index 5d732c6..e849d3e 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -58,18 +58,11 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
 {
  struct shash_alg *shash = crypto_shash_alg(tfm);
  unsigned long alignmask = crypto_shash_alignmask(tfm);
- int err;
 
  if ((unsigned long)key & alignmask)
- err = shash_setkey_unaligned(tfm, key, keylen);
- else
- err = shash->setkey(tfm, key, keylen);
-
- if (err)
- return err;
+ return shash_setkey_unaligned(tfm, key, keylen);
 
- crypto_shash_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
- return 0;
+ return shash->setkey(tfm, key, keylen);
 }
 EXPORT_SYMBOL_GPL(crypto_shash_setkey);
 
@@ -188,9 +181,6 @@ int crypto_shash_digest(struct shash_desc *desc, const u8 *data,
  struct shash_alg *shash = crypto_shash_alg(tfm);
  unsigned long alignmask = crypto_shash_alignmask(tfm);
 
- if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
  if (((unsigned long)data | (unsigned long)out) & alignmask)
  return shash_digest_unaligned(desc, data, len, out);
 
@@ -370,8 +360,7 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
  crt->digest = shash_async_digest;
  crt->setkey = shash_async_setkey;
 
- crypto_ahash_set_flags(crt, crypto_shash_get_flags(shash) &
-    CRYPTO_TFM_NEED_KEY);
+ crt->has_setkey = alg->setkey != shash_no_setkey;
 
  if (alg->export)
  crt->export = shash_async_export;
@@ -386,14 +375,8 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
 static int crypto_shash_init_tfm(struct crypto_tfm *tfm)
 {
  struct crypto_shash *hash = __crypto_shash_cast(tfm);
- struct shash_alg *alg = crypto_shash_alg(hash);
-
- hash->descsize = alg->descsize;
-
- if (crypto_shash_alg_has_setkey(alg) &&
-    !(alg->base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY))
- crypto_shash_set_flags(hash, CRYPTO_TFM_NEED_KEY);
 
+ hash->descsize = crypto_shash_alg(hash)->descsize;
  return 0;
 }
 
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 3880793..0ed31fd 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -210,6 +210,7 @@ struct crypto_ahash {
       unsigned int keylen);
 
  unsigned int reqsize;
+ bool has_setkey;
  struct crypto_tfm base;
 };
 
@@ -409,6 +410,11 @@ static inline void *ahash_request_ctx(struct ahash_request *req)
 int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
  unsigned int keylen);
 
+static inline bool crypto_ahash_has_setkey(struct crypto_ahash *tfm)
+{
+ return tfm->has_setkey;
+}
+
 /**
  * crypto_ahash_finup() - update and finalize message digest
  * @req: reference to the ahash_request handle that holds all information
@@ -481,12 +487,7 @@ static inline int crypto_ahash_export(struct ahash_request *req, void *out)
  */
 static inline int crypto_ahash_import(struct ahash_request *req, const void *in)
 {
- struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
-
- if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
- return tfm->import(req, in);
+ return crypto_ahash_reqtfm(req)->import(req, in);
 }
 
 /**
@@ -502,12 +503,7 @@ static inline int crypto_ahash_import(struct ahash_request *req, const void *in)
  */
 static inline int crypto_ahash_init(struct ahash_request *req)
 {
- struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
-
- if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
- return tfm->init(req);
+ return crypto_ahash_reqtfm(req)->init(req);
 }
 
 /**
@@ -859,12 +855,7 @@ static inline int crypto_shash_export(struct shash_desc *desc, void *out)
  */
 static inline int crypto_shash_import(struct shash_desc *desc, const void *in)
 {
- struct crypto_shash *tfm = desc->tfm;
-
- if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
- return crypto_shash_alg(tfm)->import(desc, in);
+ return crypto_shash_alg(desc->tfm)->import(desc, in);
 }
 
 /**
@@ -880,12 +871,7 @@ static inline int crypto_shash_import(struct shash_desc *desc, const void *in)
  */
 static inline int crypto_shash_init(struct shash_desc *desc)
 {
- struct crypto_shash *tfm = desc->tfm;
-
- if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
- return -ENOKEY;
-
- return crypto_shash_alg(tfm)->init(desc);
+ return crypto_shash_alg(desc->tfm)->init(desc);
 }
 
 /**
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 29c4257..72e47b3 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -115,8 +115,6 @@
 /*
  * Transform masks and values (for crt_flags).
  */
-#define CRYPTO_TFM_NEED_KEY 0x00000001
-
 #define CRYPTO_TFM_REQ_MASK 0x000fff00
 #define CRYPTO_TFM_RES_MASK 0xfff00000
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NAK: [Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Seth Forshee
In reply to this post by Joseph Salisbury-3
On Tue, Apr 03, 2018 at 01:47:23PM -0400, Joseph Salisbury wrote:

> BugLink: http://bugs.launchpad.net/bugs/1759791
>
> == Bionic Justification ==
> Mainline commit 9fa68f620041 introduced a regression in Bionic.  Bionic
> got this commit with the 4.15.4 updates as commit 46e8d06.  
>
> This bug causes the NFS mounts with kerberos set up to stopped working.
>  
> A proper patch is being discussed upstream, but it has not landed in mainline
> as of yet:
> https://patchwork.kernel.org/patch/10311831/
>
>
> == Fix ==
> A revert of commit:
> 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")

The patch being reverted is a security fix, so I'd prefer to not revert
it. It looks like there's a fix in linux-next already:

commit 190b22eedd032c14cbc2b9e13d112f039460522c
Author: Eric Biggers <[hidden email]>
Date:   Wed Mar 28 10:57:22 2018 -0700

    sunrpc: remove incorrect HMAC request initialization

Can we try using this patch instead?

Thanks,
Seth

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: NAK: [Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Joseph Salisbury-3
On 04/03/2018 02:45 PM, Seth Forshee wrote:

> On Tue, Apr 03, 2018 at 01:47:23PM -0400, Joseph Salisbury wrote:
>> BugLink: http://bugs.launchpad.net/bugs/1759791
>>
>> == Bionic Justification ==
>> Mainline commit 9fa68f620041 introduced a regression in Bionic.  Bionic
>> got this commit with the 4.15.4 updates as commit 46e8d06.  
>>
>> This bug causes the NFS mounts with kerberos set up to stopped working.
>>  
>> A proper patch is being discussed upstream, but it has not landed in mainline
>> as of yet:
>> https://patchwork.kernel.org/patch/10311831/
>>
>>
>> == Fix ==
>> A revert of commit:
>> 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
> The patch being reverted is a security fix, so I'd prefer to not revert
> it. It looks like there's a fix in linux-next already:
>
> commit 190b22eedd032c14cbc2b9e13d112f039460522c
> Author: Eric Biggers <[hidden email]>
> Date:   Wed Mar 28 10:57:22 2018 -0700
>
>     sunrpc: remove incorrect HMAC request initialization
>
> Can we try using this patch instead?
>
> Thanks,
> Seth

I'll have that patch tested.  Thanks, Seth.


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: NAK: [Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Joseph Salisbury-3
In reply to this post by Seth Forshee
On 04/03/2018 02:45 PM, Seth Forshee wrote:

> On Tue, Apr 03, 2018 at 01:47:23PM -0400, Joseph Salisbury wrote:
>> BugLink: http://bugs.launchpad.net/bugs/1759791
>>
>> == Bionic Justification ==
>> Mainline commit 9fa68f620041 introduced a regression in Bionic.  Bionic
>> got this commit with the 4.15.4 updates as commit 46e8d06.  
>>
>> This bug causes the NFS mounts with kerberos set up to stopped working.
>>  
>> A proper patch is being discussed upstream, but it has not landed in mainline
>> as of yet:
>> https://patchwork.kernel.org/patch/10311831/
>>
>>
>> == Fix ==
>> A revert of commit:
>> 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
> The patch being reverted is a security fix, so I'd prefer to not revert
> it. It looks like there's a fix in linux-next already:
>
> commit 190b22eedd032c14cbc2b9e13d112f039460522c
> Author: Eric Biggers <[hidden email]>
> Date:   Wed Mar 28 10:57:22 2018 -0700
>
>     sunrpc: remove incorrect HMAC request initialization
>
> Can we try using this patch instead?
>
> Thanks,
> Seth

Testing of that commit is confirmed to resolve the bug by two separate
users that were affected.  Can we pull that commit into Bionic instead
of the revert?


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: NAK: [Bionic][PATCH 0/1] Revert "crypto: hash - prevent using keyed hashes without setting key"

Seth Forshee
On Tue, Apr 10, 2018 at 11:10:36AM -0400, Joseph Salisbury wrote:

> On 04/03/2018 02:45 PM, Seth Forshee wrote:
> > On Tue, Apr 03, 2018 at 01:47:23PM -0400, Joseph Salisbury wrote:
> >> BugLink: http://bugs.launchpad.net/bugs/1759791
> >>
> >> == Bionic Justification ==
> >> Mainline commit 9fa68f620041 introduced a regression in Bionic.  Bionic
> >> got this commit with the 4.15.4 updates as commit 46e8d06.  
> >>
> >> This bug causes the NFS mounts with kerberos set up to stopped working.
> >>  
> >> A proper patch is being discussed upstream, but it has not landed in mainline
> >> as of yet:
> >> https://patchwork.kernel.org/patch/10311831/
> >>
> >>
> >> == Fix ==
> >> A revert of commit:
> >> 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
> > The patch being reverted is a security fix, so I'd prefer to not revert
> > it. It looks like there's a fix in linux-next already:
> >
> > commit 190b22eedd032c14cbc2b9e13d112f039460522c
> > Author: Eric Biggers <[hidden email]>
> > Date:   Wed Mar 28 10:57:22 2018 -0700
> >
> >     sunrpc: remove incorrect HMAC request initialization
> >
> > Can we try using this patch instead?
> >
> > Thanks,
> > Seth
>
> Testing of that commit is confirmed to resolve the bug by two separate
> users that were affected.  Can we pull that commit into Bionic instead
> of the revert?

Done, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team