After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785234 - gcr encodes CKA_EC_POINT as a raw bigint instead of DER encoded as required by PKCS#11
gcr encodes CKA_EC_POINT as a raw bigint instead of DER encoded as required b...
Status: RESOLVED FIXED
Product: gcr
Classification: Core
Component: General
git master
Other Linux
: Normal normal
: ---
Assigned To: GNOME keyring maintainer(s)
GNOME keyring maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-07-21 14:37 UTC by Jakub Jelen
Modified: 2019-02-22 11:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (4.89 KB, patch)
2017-07-23 19:30 UTC, Jakub Jelen
none Details | Review
Properly encode and decode CKA_EC_POINT in spk (3.03 KB, patch)
2017-11-27 14:25 UTC, Jakub Jelen
none Details | Review
Properly encode and decode CKA_EC_POINT (6.34 KB, patch)
2017-11-27 14:50 UTC, Jakub Jelen
committed Details | Review

Description Jakub Jelen 2017-07-21 14:37:39 UTC
While working on gnome-keyring ECDSA support, I noticed that I was getting wrongly formatted PKCS#11 objects.

The problem is that the gcr just puts into CKA_EC_POINT the bignum

    if (!read_buffer_mpi (buffer, offset, builder, CKA_EC_POINT))

but PKCS#11 says it should be

    DER-encoding of ANSI X9.62 ECPoint value Q

This is fairly easy to fix in the code, but I was not able to find the way how to update the data, which are used to compare against the encoded values. I manually checked that the only changes are the lengths and this specific attribute.

The patch is in github PR:

https://github.com/GNOME/gcr/pull/5
Comment 1 Jakub Jelen 2017-07-23 19:30:08 UTC
Created attachment 356240 [details] [review]
proposed patch
Comment 2 Daiki Ueno 2017-11-13 14:54:44 UTC
Review of attachment 356240 [details] [review]:

This patch seems to touch only the decoding behavior of ASN.1 to PKCS#11, not the other way around (PKCS#11 to ASN.1):
https://git.gnome.org/browse/gcr/tree/gcr/gcr-subject-public-key.c#n774

Would it make sense to update it as well?
Comment 3 Jakub Jelen 2017-11-13 15:15:59 UTC
Certainly yes. But I see it hard to orient in the gcr code, especially in the parts about the certificates, that are used around this code.

I will probably not be able to handle that now, but I will try to find some time for that.
Comment 4 Jakub Jelen 2017-11-27 14:25:07 UTC
Created attachment 364502 [details] [review]
Properly encode and decode CKA_EC_POINT in spk

This is an addition to the previous patch to handle the encoding and decoding also in the spk level. All the tests are passing for me now.

Reviewable also on github (both patches rebased to current master)

https://github.com/GNOME/gcr/compare/master...Jakuje:ecdsa-pkcs11
Comment 5 Daiki Ueno 2017-11-27 14:41:50 UTC
Review of attachment 356240 [details] [review]:

Looks fine.

::: egg/pk.asn
@@ +120,3 @@
 
+-- bogus attribute used to encode Q into PKCS#11 CKA_EC_POINT
+ECKeyQ ::= OCTET STRING

nit: can't we reuse "ECPoint" defined above?
Comment 6 Daiki Ueno 2017-11-27 14:42:41 UTC
Review of attachment 364502 [details] [review]:

Looks good, thanks!
Comment 7 Jakub Jelen 2017-11-27 14:50:36 UTC
Created attachment 364504 [details] [review]
Properly encode and decode CKA_EC_POINT

Sure. I did not notice the ECPoint (mainly because it was not in gnome-keyring). Good catch. Thanks for review.

I squashed previous commits + and removed the ECKeyQ
Comment 8 Daiki Ueno 2017-11-27 14:56:45 UTC
Thank you, pushed it.