DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa
@ 2019-05-31  6:59 Arek Kusztal
  2019-05-31  6:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature Arek Kusztal
  2019-06-18 12:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Akhil Goyal
  0 siblings, 2 replies; 5+ messages in thread
From: Arek Kusztal @ 2019-05-31  6:59 UTC (permalink / raw)
  To: dev; +Cc: akhil.goyal, fiona.trahe, declan.doherty, Arek Kusztal

In case big number need to be freed, data it contains should be cleared
before especially if it is critical data like private keys.

Fixes: 3e9d6bd447fb ("crypto/openssl: add RSA and mod asym operations")

Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
---
 config/common_base                           |  4 ++--
 drivers/crypto/openssl/rte_openssl_pmd_ops.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/config/common_base b/config/common_base
index 6b96e0e..a3d8e17 100644
--- a/config/common_base
+++ b/config/common_base
@@ -573,7 +573,7 @@ CONFIG_RTE_LIBRTE_PMD_OCTEONTX_CRYPTO=y
 #
 CONFIG_RTE_LIBRTE_PMD_QAT=y
 CONFIG_RTE_LIBRTE_PMD_QAT_SYM=n
-CONFIG_RTE_LIBRTE_PMD_QAT_ASYM=n
+CONFIG_RTE_LIBRTE_PMD_QAT_ASYM=y
 #
 # Max. number of QuickAssist devices, which can be detected and attached
 #
@@ -597,7 +597,7 @@ CONFIG_RTE_LIBRTE_PMD_AESNI_MB=n
 #
 # Compile PMD for Software backed device
 #
-CONFIG_RTE_LIBRTE_PMD_OPENSSL=n
+CONFIG_RTE_LIBRTE_PMD_OPENSSL=y
 
 #
 # Compile PMD for AESNI GCM device
diff --git a/drivers/crypto/openssl/rte_openssl_pmd_ops.c b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
index 40217cf..a307c91 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd_ops.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
@@ -912,14 +912,14 @@ static int openssl_set_asym_session_parameters(
 		asym_session->xfrm_type = RTE_CRYPTO_ASYM_XFORM_RSA;
 		break;
 err_rsa:
-		BN_free(n);
-		BN_free(e);
-		BN_free(d);
-		BN_free(p);
-		BN_free(q);
-		BN_free(dmp1);
-		BN_free(dmq1);
-		BN_free(iqmp);
+		BN_clear_free(n);
+		BN_clear_free(e);
+		BN_clear_free(d);
+		BN_clear_free(p);
+		BN_clear_free(q);
+		BN_clear_free(dmp1);
+		BN_clear_free(dmq1);
+		BN_clear_free(iqmp);
 
 		return -1;
 	}
-- 
2.1.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature
  2019-05-31  6:59 [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Arek Kusztal
@ 2019-05-31  6:59 ` Arek Kusztal
  2019-05-31 14:53   ` Trahe, Fiona
  2019-06-18 12:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Akhil Goyal
  1 sibling, 1 reply; 5+ messages in thread
From: Arek Kusztal @ 2019-05-31  6:59 UTC (permalink / raw)
  To: dev; +Cc: akhil.goyal, fiona.trahe, declan.doherty, Arek Kusztal

ANSI C memcmp is not constant time function per spec so it should
be avoided in cryptography usage.

Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")

Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
---
 drivers/crypto/openssl/rte_openssl_pmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 6504959..73ce383 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -1529,7 +1529,7 @@ process_openssl_auth_op(struct openssl_qp *qp, struct rte_crypto_op *op,
 	}
 
 	if (sess->auth.operation == RTE_CRYPTO_AUTH_OP_VERIFY) {
-		if (memcmp(dst, op->sym->auth.digest.data,
+		if (CRYPTO_memcmp(dst, op->sym->auth.digest.data,
 				sess->auth.digest_length) != 0) {
 			op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
 		}
@@ -1914,7 +1914,7 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
 				"Length of public_decrypt %d "
 				"length of message %zd\n",
 				ret, op->rsa.message.length);
-		if ((ret <= 0) || (memcmp(tmp, op->rsa.message.data,
+		if ((ret <= 0) || (CRYPTO_memcmp(tmp, op->rsa.message.data,
 				op->rsa.message.length))) {
 			OPENSSL_LOG(ERR, "RSA sign Verification failed");
 			cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
-- 
2.1.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature
  2019-05-31  6:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature Arek Kusztal
@ 2019-05-31 14:53   ` Trahe, Fiona
  2019-06-19 14:48     ` Akhil Goyal
  0 siblings, 1 reply; 5+ messages in thread
From: Trahe, Fiona @ 2019-05-31 14:53 UTC (permalink / raw)
  To: Kusztal, ArkadiuszX, dev; +Cc: akhil.goyal, Doherty, Declan, Trahe, Fiona



> -----Original Message-----
> From: Kusztal, ArkadiuszX
> Sent: Friday, May 31, 2019 7:59 AM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Subject: [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature
> 
> ANSI C memcmp is not constant time function per spec so it should
> be avoided in cryptography usage.
> 
> Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  drivers/crypto/openssl/rte_openssl_pmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
> index 6504959..73ce383 100644
> --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> @@ -1529,7 +1529,7 @@ process_openssl_auth_op(struct openssl_qp *qp, struct rte_crypto_op *op,
>  	}
> 
>  	if (sess->auth.operation == RTE_CRYPTO_AUTH_OP_VERIFY) {
> -		if (memcmp(dst, op->sym->auth.digest.data,
> +		if (CRYPTO_memcmp(dst, op->sym->auth.digest.data,
>  				sess->auth.digest_length) != 0) {
>  			op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
>  		}
> @@ -1914,7 +1914,7 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
>  				"Length of public_decrypt %d "
>  				"length of message %zd\n",
>  				ret, op->rsa.message.length);
> -		if ((ret <= 0) || (memcmp(tmp, op->rsa.message.data,
> +		if ((ret <= 0) || (CRYPTO_memcmp(tmp, op->rsa.message.data,
>  				op->rsa.message.length))) {
>  			OPENSSL_LOG(ERR, "RSA sign Verification failed");
>  			cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> --
> 2.1.0
Hadn't heard of that time const fn before so just read up on it - interesting.
Acked-by: Fiona Trahe <fiona.trahe@intel.com>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa
  2019-05-31  6:59 [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Arek Kusztal
  2019-05-31  6:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature Arek Kusztal
@ 2019-06-18 12:59 ` Akhil Goyal
  1 sibling, 0 replies; 5+ messages in thread
From: Akhil Goyal @ 2019-06-18 12:59 UTC (permalink / raw)
  To: Arek Kusztal, dev; +Cc: fiona.trahe, declan.doherty

Hi Arek,

> In case big number need to be freed, data it contains should be cleared
> before especially if it is critical data like private keys.
> 
> Fixes: 3e9d6bd447fb ("crypto/openssl: add RSA and mod asym operations")
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  config/common_base                           |  4 ++--
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c | 16 ++++++++--------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/config/common_base b/config/common_base
> index 6b96e0e..a3d8e17 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -573,7 +573,7 @@ CONFIG_RTE_LIBRTE_PMD_OCTEONTX_CRYPTO=y
>  #
>  CONFIG_RTE_LIBRTE_PMD_QAT=y
>  CONFIG_RTE_LIBRTE_PMD_QAT_SYM=n
> -CONFIG_RTE_LIBRTE_PMD_QAT_ASYM=n
> +CONFIG_RTE_LIBRTE_PMD_QAT_ASYM=y
>  #
>  # Max. number of QuickAssist devices, which can be detected and attached
>  #
> @@ -597,7 +597,7 @@ CONFIG_RTE_LIBRTE_PMD_AESNI_MB=n
>  #
>  # Compile PMD for Software backed device
>  #
> -CONFIG_RTE_LIBRTE_PMD_OPENSSL=n
> +CONFIG_RTE_LIBRTE_PMD_OPENSSL=y
> 
I think these config changes were done by mistake in this patch.

Openssl cannot be enabled by default as it needs external codebase.

Please send fix only for openssl driver as the description says.

Thanks,
Akhil

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature
  2019-05-31 14:53   ` Trahe, Fiona
@ 2019-06-19 14:48     ` Akhil Goyal
  0 siblings, 0 replies; 5+ messages in thread
From: Akhil Goyal @ 2019-06-19 14:48 UTC (permalink / raw)
  To: Trahe, Fiona, Kusztal, ArkadiuszX, dev; +Cc: Doherty, Declan



> 
> 
> > -----Original Message-----
> > From: Kusztal, ArkadiuszX
> > Sent: Friday, May 31, 2019 7:59 AM
> > To: dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Doherty,
> Declan
> > <declan.doherty@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>
> > Subject: [PATCH] crypto/openssl: fix usage of non constant time memcmp for
> mac and signature
> >
> > ANSI C memcmp is not constant time function per spec so it should
> > be avoided in cryptography usage.
> >
> > Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > ---
> >  drivers/crypto/openssl/rte_openssl_pmd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c
> b/drivers/crypto/openssl/rte_openssl_pmd.c
> > index 6504959..73ce383 100644
> > --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> > +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> > @@ -1529,7 +1529,7 @@ process_openssl_auth_op(struct openssl_qp *qp,
> struct rte_crypto_op *op,
> >  	}
> >
> >  	if (sess->auth.operation == RTE_CRYPTO_AUTH_OP_VERIFY) {
> > -		if (memcmp(dst, op->sym->auth.digest.data,
> > +		if (CRYPTO_memcmp(dst, op->sym->auth.digest.data,
> >  				sess->auth.digest_length) != 0) {
> >  			op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
> >  		}
> > @@ -1914,7 +1914,7 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
> >  				"Length of public_decrypt %d "
> >  				"length of message %zd\n",
> >  				ret, op->rsa.message.length);
> > -		if ((ret <= 0) || (memcmp(tmp, op->rsa.message.data,
> > +		if ((ret <= 0) || (CRYPTO_memcmp(tmp, op->rsa.message.data,
> >  				op->rsa.message.length))) {
> >  			OPENSSL_LOG(ERR, "RSA sign Verification failed");
> >  			cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> > --
> > 2.1.0
> Hadn't heard of that time const fn before so just read up on it - interesting.
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>
> 
Modified the subject a bit.
Applied to dpdk-next-crypto

Thanks

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31  6:59 [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Arek Kusztal
2019-05-31  6:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix usage of non constant time memcmp for mac and signature Arek Kusztal
2019-05-31 14:53   ` Trahe, Fiona
2019-06-19 14:48     ` Akhil Goyal
2019-06-18 12:59 ` [dpdk-dev] [PATCH] crypto/openssl: fix inproper freeing of asymmetric crypto keys in rsa Akhil Goyal

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox