[PATCH v2][RESEND] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
This is a resend of a patch that was previously submitted in one series
with CCP driver changes since this particular patch should go through
the security (rather than crypto) tree.
Changes from v1: Change '!' to '== 0'.
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index 7d81e6bb461a..b6cabac4b62b 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (strcmp(ctx->cert->sig->pkey_algo, "rsa") == 0) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
Re: [PATCH v2] X.509: unpack RSA signatureValue field from BIT STRING
On 17.04.2018 17:07, Kamil Konieczny wrote: > > > On 17.04.2018 15:39, Maciej S. Szmigiero wrote: >> The signatureValue field of a X.509 certificate is encoded as a BIT STRING. >> For RSA signatures this BIT STRING is of so-called primitive subtype, which >> contains a u8 prefix indicating a count of unused bits in the encoding. >> >> We have to strip this prefix from signature data, just as we already do for >> key data in x509_extract_key_data() function. >> >> This wasn't noticed earlier because this prefix byte is zero for RSA key >> sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero >> prefixes has no bearing on its value. >> >> The signature length, however was incorrect, which is a problem for RSA >> implementations that need it to be exactly correct (like AMD CCP). >> >> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name> > > your e-mail address looks incorrect > > [...] > What's wrong with it? Maciej
[PATCH v2] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
This is a resend of a patch that was previously submitted in one series
with CCP driver changes since this particular patch should go through
the security (rather than crypto) tree.
Changes from v1: Change '!' to '== 0'.
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index 7d81e6bb461a..b6cabac4b62b 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (strcmp(ctx->cert->sig->pkey_algo, "rsa") == 0) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
Re: [PATCH] X.509: unpack RSA signatureValue field from BIT STRING
On 07.03.2018 18:56, Maciej S. Szmigiero wrote:
> On 07.03.2018 16:44, David Howells wrote:
>> Maciej S. Szmigiero <m...@maciej.szmigiero.name> wrote:
>>
>>> + if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
>>
>> I'm going to change this to '== 0' rather than '!'.
>
> No problem.
I cannot find this patch in any tree that I have looked at.
Are you going to pick it up later or am I not looking at the right
place?
Thanks,
Maciej
Re: [PATCH] crypto/ccp: Validate buffer lengths for copy operations
On 07.03.2018 18:31, Gary R Hook wrote: > The CCP driver copies data between scatter/gather lists and DMA buffers. > The length of the requested copy operation must be checked against > the available destination buffer length. > > Reported-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name> > Signed-off-by: Gary R Hook <gary.h...@amd.com> This patch looks okay to me. I can confirm that CCP on 4.15.7 with this patch applied passes testmgr tests and verifies the wireless regulatory database RSA signature correctly. Maciej
Re: [PATCH] X.509: unpack RSA signatureValue field from BIT STRING
On 07.03.2018 16:44, David Howells wrote:
> Maciej S. Szmigiero <m...@maciej.szmigiero.name> wrote:
>
>> +if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
>
> I'm going to change this to '== 0' rather than '!'.
No problem.
> David
>
Thanks,
Maciej
[PATCH] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
This is a resend without changes of a patch that was previously
submitted in one series with CCP driver changes since this particular
patch should go through the security (rather than crypto) tree.
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index ce2df8c9c583..88c26a4538ae 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
Re: [PATCH 2/3] crypto: ccp - return an actual key size from RSA max_size callback
On 03.03.2018 00:49, Hook, Gary wrote:
> On 3/2/2018 5:15 PM, Maciej S. Szmigiero wrote:
>> On 02.03.2018 17:44, Herbert Xu wrote:
>>> On Sat, Feb 24, 2018 at 05:03:21PM +0100, Maciej S. Szmigiero wrote:
>>>> rsa-pkcs1pad uses a value returned from a RSA implementation max_size
>>>> callback as a size of an input buffer passed to the RSA implementation for
>>>> encrypt and sign operations.
>>>>
>>>> CCP RSA implementation uses a hardware input buffer which size depends only
>>>> on the current RSA key length, so it should return this key length in
>>>> the max_size callback, too.
>>>> This also matches what the kernel software RSA implementation does.
>>>>
>>>> Previously, the value returned from this callback was always the maximum
>>>> RSA key size the CCP hardware supports.
>>>> This resulted in this huge buffer being passed by rsa-pkcs1pad to CCP even
>>>> for smaller key sizes and then in a buffer overflow when ccp_run_rsa_cmd()
>>>> tried to copy this large input buffer into a RSA key length-sized hardware
>>>> input buffer.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
>>>> Fixes: ceeec0afd684 ("crypto: ccp - Add support for RSA on the CCP")
>>>> Cc: sta...@vger.kernel.org
>>>
>>> Patch applied. Thanks.
>>
>> Thanks.
>>
>> However, what about the first patch from this series?
>> Without it, while it no longer should cause a buffer overflow, in-kernel
>> X.509 certificate verification will still fail with CCP driver loaded
>> (since CCP RSA implementation has a higher priority than the software
>> RSA implementation).
>>
>> Maciej
>>
>
>
> I commented on that one here:
> https://marc.info/?l=linux-crypto-vger=151986452422791=2
>
> Effectively a NACK. We are a reviewing a proposed patch right now.
Your earlier comment referred to the third patch from this series.
My message above was about the first one.
Maciej
Re: [PATCH 2/3] crypto: ccp - return an actual key size from RSA max_size callback
On 02.03.2018 17:44, Herbert Xu wrote:
> On Sat, Feb 24, 2018 at 05:03:21PM +0100, Maciej S. Szmigiero wrote:
>> rsa-pkcs1pad uses a value returned from a RSA implementation max_size
>> callback as a size of an input buffer passed to the RSA implementation for
>> encrypt and sign operations.
>>
>> CCP RSA implementation uses a hardware input buffer which size depends only
>> on the current RSA key length, so it should return this key length in
>> the max_size callback, too.
>> This also matches what the kernel software RSA implementation does.
>>
>> Previously, the value returned from this callback was always the maximum
>> RSA key size the CCP hardware supports.
>> This resulted in this huge buffer being passed by rsa-pkcs1pad to CCP even
>> for smaller key sizes and then in a buffer overflow when ccp_run_rsa_cmd()
>> tried to copy this large input buffer into a RSA key length-sized hardware
>> input buffer.
>>
>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
>> Fixes: ceeec0afd684 ("crypto: ccp - Add support for RSA on the CCP")
>> Cc: sta...@vger.kernel.org
>
> Patch applied. Thanks.
Thanks.
However, what about the first patch from this series?
Without it, while it no longer should cause a buffer overflow, in-kernel
X.509 certificate verification will still fail with CCP driver loaded
(since CCP RSA implementation has a higher priority than the software
RSA implementation).
Maciej
[PATCH 2/3] crypto: ccp - return an actual key size from RSA max_size callback
rsa-pkcs1pad uses a value returned from a RSA implementation max_size
callback as a size of an input buffer passed to the RSA implementation for
encrypt and sign operations.
CCP RSA implementation uses a hardware input buffer which size depends only
on the current RSA key length, so it should return this key length in
the max_size callback, too.
This also matches what the kernel software RSA implementation does.
Previously, the value returned from this callback was always the maximum
RSA key size the CCP hardware supports.
This resulted in this huge buffer being passed by rsa-pkcs1pad to CCP even
for smaller key sizes and then in a buffer overflow when ccp_run_rsa_cmd()
tried to copy this large input buffer into a RSA key length-sized hardware
input buffer.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: ceeec0afd684 ("crypto: ccp - Add support for RSA on the CCP")
Cc: sta...@vger.kernel.org
---
drivers/crypto/ccp/ccp-crypto-rsa.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/ccp/ccp-crypto-rsa.c
b/drivers/crypto/ccp/ccp-crypto-rsa.c
index e6db8672d89c..05850dfd7940 100644
--- a/drivers/crypto/ccp/ccp-crypto-rsa.c
+++ b/drivers/crypto/ccp/ccp-crypto-rsa.c
@@ -60,10 +60,9 @@ static int ccp_rsa_complete(struct crypto_async_request
*async_req, int ret)
static unsigned int ccp_rsa_maxsize(struct crypto_akcipher *tfm)
{
- if (ccp_version() > CCP_VERSION(3, 0))
- return CCP5_RSA_MAXMOD;
- else
- return CCP_RSA_MAXMOD;
+ struct ccp_ctx *ctx = akcipher_tfm_ctx(tfm);
+
+ return ctx->u.rsa.n_len;
}
static int ccp_rsa_crypt(struct akcipher_request *req, bool encrypt)
[PATCH 3/3] crypto: ccp - protect RSA implementation from too large input data
CCP RSA implementation uses a hardware input buffer which size depends only
on the current RSA key length. Key modulus and a message to be processed
is then copied to this buffer based on their own lengths.
Since the price for providing too long input data is a buffer overflow and
there already has been a case when this has happened let's better reject
such oversized input data and log an error message in this case so we know
what is going on.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
drivers/crypto/ccp/ccp-ops.c | 24
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
index 406b95329b3d..517aeee30abf 100644
--- a/drivers/crypto/ccp/ccp-ops.c
+++ b/drivers/crypto/ccp/ccp-ops.c
@@ -1770,10 +1770,6 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q,
struct ccp_cmd *cmd)
if (!rsa->exp || !rsa->mod || !rsa->src || !rsa->dst)
return -EINVAL;
- memset(, 0, sizeof(op));
- op.cmd_q = cmd_q;
- op.jobid = CCP_NEW_JOBID(cmd_q->ccp);
-
/* The RSA modulus must precede the message being acted upon, so
* it must be copied to a DMA area where the message and the
* modulus can be concatenated. Therefore the input buffer
@@ -1785,6 +1781,26 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q,
struct ccp_cmd *cmd)
o_len = 32 * ((rsa->key_size + 255) / 256);
i_len = o_len * 2;
+ if (rsa->mod_len > o_len) {
+ dev_err(cmd_q->ccp->dev,
+ "RSA modulus of %u bytes too large for key size of %u
bits\n",
+ (unsigned int)rsa->mod_len,
+ (unsigned int)rsa->key_size);
+ return -EINVAL;
+ }
+
+ if (rsa->src_len > o_len) {
+ dev_err(cmd_q->ccp->dev,
+ "RSA data of %u bytes too large for key size of %u
bits\n",
+ (unsigned int)rsa->src_len,
+ (unsigned int)rsa->key_size);
+ return -EINVAL;
+ }
+
+ memset(, 0, sizeof(op));
+ op.cmd_q = cmd_q;
+ op.jobid = CCP_NEW_JOBID(cmd_q->ccp);
+
sb_count = 0;
if (cmd_q->ccp->vdata->version < CCP_VERSION(5, 0)) {
/* sb_count is the number of storage block slots required
[PATCH 1/3] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index ce2df8c9c583..88c26a4538ae 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
