This was prompted by the caam RSA submission where a lot of work
was done just to strip the RSA output of leading zeroes. This is
in fact completely pointless because the only user of RSA in the
kernel then promptly puts them back.
This patch series resolves this madness by simply leaving any
leading zeroes in place. Note that we're not requiring authors
to add leading zeroes, even though that is encouraged if it is
easy to do. In practice you'd only run into this every 2^32 or
2^64 operations so please don't overdo it.
I've also taken the opportunity to cleanup the pkcs1pad code.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi Herbert,
On 06/22/2016 03:14 AM, Herbert Xu wrote:
> This was prompted by the caam RSA submission where a lot of work
> was done just to strip the RSA output of leading zeroes. This is
> in fact completely pointless because the only user of RSA in the
> kernel then promptly puts them back.
>
> This patch series resolves this madness by simply leaving any
> leading zeroes in place.
The reason why mpi_write_to_sgl() strips the leading zeros is only
because we said that it needs to work in the same way as the
mpi_read_buffer(), which does remove it for whatever reason.
So should we now change the mpi_read_buffer() as well?
The mpi_read_buffer() is called from mpi_get_buffer(), which is used
only by lib/digsig.c
We also need to change the qat rsa implementation because it does remove
zeros as well, but it will be very easy to do.
Thanks,
--
TS
Am Mittwoch, 22. Juni 2016, 18:14:32 schrieb Herbert Xu:
Hi Herbert,
Something breaks with this patch set in public_key_verify_signature
I get tons of these:
[ 1.838720] PKCS#7 signature not signed with a trusted key
Furthermore, my CAVS testing with public_key_verify_signature always EINVAL.
SigGen using the kernel crypto API interfaces work though.
Ciao
Stephan
On Fri, Jun 24, 2016 at 09:27:12AM +0200, Stephan Mueller wrote:
> Am Mittwoch, 22. Juni 2016, 18:14:32 schrieb Herbert Xu:
>
> Hi Herbert,
>
> Something breaks with this patch set in public_key_verify_signature
>
> I get tons of these:
>
> [ 1.838720] PKCS#7 signature not signed with a trusted key
>
>
> Furthermore, my CAVS testing with public_key_verify_signature always EINVAL.
>
> SigGen using the kernel crypto API interfaces work though.
Hi Stephan:
Can you bisect this down to a specific patch please?
Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Am Freitag, 24. Juni 2016, 16:41:47 schrieb Herbert Xu:
Hi Herbert,
> On Fri, Jun 24, 2016 at 09:27:12AM +0200, Stephan Mueller wrote:
> > Am Mittwoch, 22. Juni 2016, 18:14:32 schrieb Herbert Xu:
> >
> > Hi Herbert,
> >
> > Something breaks with this patch set in public_key_verify_signature
> >
> > I get tons of these:
> >
> > [ 1.838720] PKCS#7 signature not signed with a trusted key
> >
> >
> > Furthermore, my CAVS testing with public_key_verify_signature always
> > EINVAL.
> >
> > SigGen using the kernel crypto API interfaces work though.
>
> Hi Stephan:
>
> Can you bisect this down to a specific patch please?
Will do.
>
> Thanks!
Ciao
Stephan
Am Freitag, 24. Juni 2016, 16:41:47 schrieb Herbert Xu:
Hi Herbert,
> On Fri, Jun 24, 2016 at 09:27:12AM +0200, Stephan Mueller wrote:
> > Am Mittwoch, 22. Juni 2016, 18:14:32 schrieb Herbert Xu:
> >
> > Hi Herbert,
> >
> > Something breaks with this patch set in public_key_verify_signature
> >
> > I get tons of these:
> >
> > [ 1.838720] PKCS#7 signature not signed with a trusted key
> >
> >
> > Furthermore, my CAVS testing with public_key_verify_signature always
> > EINVAL.
> >
> > SigGen using the kernel crypto API interfaces work though.
>
> Hi Stephan:
>
> Can you bisect this down to a specific patch please?
Patch 2 introduces the bug.
Note, with patch 2, there is also a compile warning with crypto/dh.c.
>
> Thanks!
Ciao
Stephan
On Fri, Jun 24, 2016 at 11:23:06AM +0200, Stephan Mueller wrote:
>
> Patch 2 introduces the bug.
>
> Note, with patch 2, there is also a compile warning with crypto/dh.c.
Oh I see. It's a conflict with the kpp stuff that didn't exist
when I wrote this.
Let me respin the patches on top of kpp.
Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Jun 23, 2016 at 08:25:05AM -0700, Tadeusz Struk wrote:
>
> The reason why mpi_write_to_sgl() strips the leading zeros is only
> because we said that it needs to work in the same way as the
> mpi_read_buffer(), which does remove it for whatever reason.
> So should we now change the mpi_read_buffer() as well?
Didn't we add mpi_read_buffer specifically for akcipher before
we switched over to SGs? If nobody is using it we should just
delete it.
> We also need to change the qat rsa implementation because it does remove
> zeros as well, but it will be very easy to do.
The way it's done with my patches you don't have to do the conversion
right away because it'll cope with either stripping or not stripping
leading zeroes. But yes it should simplify the code in qat so please
send in patches when you have time.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 06/24/2016 07:28 AM, Herbert Xu wrote:
> Didn't we add mpi_read_buffer specifically for akcipher before
> we switched over to SGs? If nobody is using it we should just
> delete it.
Yes, but now mpi_get_buffer() calls mpi_read_buffer() and it is also
used by security/keys/dh.c
Thanks,
--
TS
On Fri, Jun 24, 2016 at 08:25:02AM -0700, Tadeusz Struk wrote:
> Yes, but now mpi_get_buffer() calls mpi_read_buffer() and it is also
> used by security/keys/dh.c
It appears to be using mpi_read_buffer gratuitously. It should
simply use mpi_get_buffer.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi:
This was prompted by the caam RSA submission where a lot of work
was done just to strip the RSA output of leading zeroes. This is
in fact completely pointless because the only user of RSA in the
kernel then promptly puts them back.
This patch series resolves this madness by simply leaving any
leading zeroes in place. Note that we're not requiring authors
to add leading zeroes, even though that is encouraged if it is
easy to do. In practice you'd only run into this every 2^32 or
2^64 operations so please don't overdo it.
I've also taken the opportunity to cleanup the pkcs1pad code.
v2 fixes the newly added dh to use the new MPI SG interface.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi:
This was prompted by the caam RSA submission where a lot of work
was done just to strip the RSA output of leading zeroes. This is
in fact completely pointless because the only user of RSA in the
kernel then promptly puts them back.
This patch series resolves this madness by simply leaving any
leading zeroes in place. Note that we're not requiring authors
to add leading zeroes, even though that is encouraged if it is
easy to do. In practice you'd only run into this every 2^32 or
2^64 operations so please don't overdo it.
I've also taken the opportunity to cleanup the pkcs1pad code.
v2 fixes the newly added dh to use the new MPI SG interface.
v3 adds a patch that went AWOL in v2.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi:
This was prompted by the caam RSA submission where a lot of work
was done just to strip the RSA output of leading zeroes. This is
in fact completely pointless because the only user of RSA in the
kernel then promptly puts them back.
This patch series resolves this madness by simply leaving any
leading zeroes in place. Note that we're not requiring authors
to add leading zeroes, even though that is encouraged if it is
easy to do. In practice you'd only run into this every 2^32 or
2^64 operations so please don't overdo it.
I've also taken the opportunity to cleanup the pkcs1pad code.
v4 fixes the newly added dh to use the new MPI SG interface.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Am Mittwoch, 29. Juni 2016, 19:31:25 CEST schrieb Herbert Xu:
Hi Herbert,
I re-tested that patch set and I still see the same issues as before, namely
that sigver does not work:
Kernel log:
PKCS#7 signature not signed with a trusted key
And my CAVS harness also fails.
Is there any prerequisite to that patch that I have to consider?
> Hi:
>
> This was prompted by the caam RSA submission where a lot of work
> was done just to strip the RSA output of leading zeroes. This is
> in fact completely pointless because the only user of RSA in the
> kernel then promptly puts them back.
>
> This patch series resolves this madness by simply leaving any
> leading zeroes in place. Note that we're not requiring authors
> to add leading zeroes, even though that is encouraged if it is
> easy to do. In practice you'd only run into this every 2^32 or
> 2^64 operations so please don't overdo it.
>
> I've also taken the opportunity to cleanup the pkcs1pad code.
>
> v4 fixes the newly added dh to use the new MPI SG interface.
>
> Cheers,
Ciao
Stephan
Am Samstag, 2. Juli 2016, 19:55:59 CEST schrieb Stephan Mueller:
Hi Herbert,
> Am Mittwoch, 29. Juni 2016, 19:31:25 CEST schrieb Herbert Xu:
>
> Hi Herbert,
>
> I re-tested that patch set and I still see the same issues as before, namely
> that sigver does not work:
>
> Kernel log:
>
> PKCS#7 signature not signed with a trusted key
>
> And my CAVS harness also fails.
>
> Is there any prerequisite to that patch that I have to consider?
Note, I see that even with the patch set that went into your cryptodev-2.6
tree.
Ciao
Stephan
On Sat, Jul 02, 2016 at 07:55:59PM +0200, Stephan Mueller wrote:
>
> I re-tested that patch set and I still see the same issues as before, namely
> that sigver does not work:
Oops, somehow I conflated this problem with KPP. Does this patch
fix it for you?
---8<---
Subject: crypto: rsa-pkcs1pad - Fix regression from leading zeros
As the software RSA implementation now produces fixed-length
output, we need to eliminate leading zeros in the calling code
instead.
This patch does just that for pkcs1pad signature verification.
Fixes: 9b45b7bba3d2 ("crypto: rsa - Generate fixed-length output")
Reported-by: Stephan Mueller <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 8ccfdd7..880d3db 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -456,49 +456,55 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)
struct akcipher_instance *inst = akcipher_alg_instance(tfm);
struct pkcs1pad_inst_ctx *ictx = akcipher_instance_ctx(inst);
const struct rsa_asn1_template *digest_info = ictx->digest_info;
+ unsigned int dst_len;
unsigned int pos;
-
- if (err == -EOVERFLOW)
- /* Decrypted value had no leading 0 byte */
- err = -EINVAL;
+ u8 *out_buf;
if (err)
goto done;
- if (req_ctx->child_req.dst_len != ctx->key_size - 1) {
- err = -EINVAL;
+ err = -EINVAL;
+ dst_len = req_ctx->child_req.dst_len;
+ if (dst_len < ctx->key_size - 1)
goto done;
+
+ out_buf = req_ctx->out_buf;
+ if (dst_len == ctx->key_size) {
+ if (out_buf[0] != 0x00)
+ /* Decrypted value had no leading 0 byte */
+ goto done;
+
+ dst_len--;
+ out_buf++;
}
err = -EBADMSG;
- if (req_ctx->out_buf[0] != 0x01)
+ if (out_buf[0] != 0x01)
goto done;
- for (pos = 1; pos < req_ctx->child_req.dst_len; pos++)
- if (req_ctx->out_buf[pos] != 0xff)
+ for (pos = 1; pos < dst_len; pos++)
+ if (out_buf[pos] != 0xff)
break;
- if (pos < 9 || pos == req_ctx->child_req.dst_len ||
- req_ctx->out_buf[pos] != 0x00)
+ if (pos < 9 || pos == dst_len || out_buf[pos] != 0x00)
goto done;
pos++;
- if (memcmp(req_ctx->out_buf + pos, digest_info->data,
- digest_info->size))
+ if (memcmp(out_buf + pos, digest_info->data, digest_info->size))
goto done;
pos += digest_info->size;
err = 0;
- if (req->dst_len < req_ctx->child_req.dst_len - pos)
+ if (req->dst_len < dst_len - pos)
err = -EOVERFLOW;
- req->dst_len = req_ctx->child_req.dst_len - pos;
+ req->dst_len = dst_len - pos;
if (!err)
sg_copy_from_buffer(req->dst,
sg_nents_for_len(req->dst, req->dst_len),
- req_ctx->out_buf + pos, req->dst_len);
+ out_buf + pos, req->dst_len);
done:
kzfree(req_ctx->out_buf);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Am Sonntag, 3. Juli 2016, 10:46:11 CEST schrieb Herbert Xu:
Hi Herbert,
> On Sat, Jul 02, 2016 at 07:55:59PM +0200, Stephan Mueller wrote:
> > I re-tested that patch set and I still see the same issues as before,
> > namely
> > that sigver does not work:
> Oops, somehow I conflated this problem with KPP. Does this patch
> fix it for you?
Yes, that patch fixes the module signature check issue and the CAVS test shows
a pass as well.
Ciao
Stephan