OpenPGP Card v3 ECC support #1506
Conversation
…cc-support-merge
There was a problem hiding this comment.
Hi,
I leave my review as "Comment", because I do not think I have the "moral right" to "Request changes".
I am merely "begging for them".
Some additional clarifications.
- Even if I added comments sometimes only to one instance that I consider not ideal, they - of course - should be applied to all other occurrences of the same type.
- github does not show trailing spaces and other little issues, so I could not comment here.
But I found issues when I cloned your repo and looked at the branch locally. - The one point where I am stringly disappointed, is the - in my opinion - absolutely pointless change of converting the unnamed union to a named one. I is standard C since C11 and supported by gcc & clang for decades.
I consider this change especially rude, as I asked you in a comment to issue OpenPGP Card 3.3: pkcs11-tool does not work with ECC keys #1351 to have a look at outstanding patches based on top of master in my cloned repo, and you only then started to push this PR.
src/libopensc/card-openpgp.c
Outdated
| memcpy(key_info->u.rsa.modulus, part, len); | ||
| } | ||
| else { | ||
| sc_log(card->ctx, "Error: key_info->u.rsa.modulus is NULL"); |
There was a problem hiding this comment.
I thought long about that function.
If you do not pass the modulus and exponent as explicit arguments to other functions, may be it's best to malloc() the necessary memory and copy those elements in any case.
@hongquan What was your intention here?
There was a problem hiding this comment.
Honestly, I am not sure, if I understand the issue here.
Do you mean in case the card response is different than requested? Like, the modulus length differs from what was inserted in the private key template? In this case I would prefer an error message instead.
Would it be sufficient to add
if (key_info->rsa.modulus && len == key_info->rsa.modulus_len){
as the value stored in key_info->rsa.modulus_len should be the size of the allocated memory (like it is done for the ec_point)?
There was a problem hiding this comment.
Sorry, @marschap, I'm not understand what you mean here. Could you please give an example?
There was a problem hiding this comment.
@hongquan
I was referring to the whole code section starting in line 2627
if (tag == 0x0081) {
/* set the output data */
if (key_info->u.rsa.modulus) {
memcpy(key_info->u.rsa.modulus, part, len);
}
else {
sc_log(card->ctx, "Error: key_info->u.rsa.modulus is NULL");
}
/* always set output for modulus_len */
key_info->u.rsa.modulus_len = len*8;
}
I am wondering why the modulus parsed (in variable part) is only copied to the key_info structure if the key_info->u.rsa.modulus pointer is not NULL.
- Shouldn't it be copied in any case?
I.e. shouldn't the memory be allocated it it is NULL?
Especially with the change now that the explicitmodulusvariable & parameter topgp_calculate_and_store_fingerprint()got (IMHO correctly) removed by @alex-nitrokey 's patch. - Furthermore, shouldn't it be checked that the size allocated for
key_info->u.rsa.modulusis large enough to keep the contents of the parsed modulus.
E.g. when going from a 2k modulus to a 4k modulus,key_info->u.rsa.modulusmay perhaps be allocated but too small.
The same reasoning of course also applies to the exponent a few lines later (tag 0x0082)
@alex-nitrokey I think your code catches at least one potential error case and reports error on it.
The issues above can be left for a later commit after @honquan clarified.
There was a problem hiding this comment.
I didn't think that key_info->u.rsa.modulus can be less than enough, because it was malloc with the size of expected key length.
But I'm happy that you add more checking that I might miss.
There was a problem hiding this comment.
I agree with @marschap , this whole section needs length checking! The length is obtained from sc_asn1_read_tag, which may initialize len to something beyond the length of the buffer, because you're not erroring on SC_ERROR_ASN1_END_OF_CONTENTS.
There was a problem hiding this comment.
I mostly reused previous code. I trusted this code more than myself. I try to stop this behaviour 😉
I tried to add some (hopefully) proper checks now. Please have a look at it.
SC_ERROR_ASN1_END_OF_CONTENTS is imho included in
r = sc_asn1_read_tag((const u8**)&part, data_len - (in - data), &cla, &tag, &len);
if (part == NULL)
r = SC_ERROR_ASN1_OBJECT_NOT_FOUND;
LOG_TEST_RET(card->ctx, r, "Unexpected end of contents.");
|
Hi, thanks a lot for your valuable review. I try to look at all your hints today. I never meant to be rude and therefore I am truly sorry if my PR looked impolite to you in any way! Please let me shortly explain, why I submitted this PR with the named union (although I knew, that you decided to use a unnamed one).
I would like to either keep the named union as is in this PR or to change it in the whole project. So if the project is okay with me changing the named union in the whole code base (e.g. for sc_pkcs15_pubkey as well), I'd do it. Again, I would prefer if you see my actions rather as unprofessional than rude. It's never my intention to upset anyone. |
|
I agree that we should keep the code base consistent. OpenSC uses named unions and therefor this style should also be used for new code. Don't take changes of the code personally. If you want to avoid changes to your code, then please don't suggest them for merging into master. To get feedback, make a pull request marked as work in progress and we'll only merge it when all of the code is ready. |
|
@frankmorgner
that hurt my pride ;-) Despite some nitpicking on details, I think this PR is very valuable, and I look forward to having it in master. Additional question: Would you find a patch set acceptable that changes the code style to anonymous unions for all of OpenSC? |
There was a problem hiding this comment.
I'd prefer using a named union, because it is more consistent with the rest of the code.
I'd also prefer real division over bit shifts, because it's more readable (the compiler will do the optimization). (Please also re-check for corner cases, if the the length is not a multiple of the divisor.)
|
@marschap do you have more functional changes in your branch that need to be considered for this PR? |
|
@frankmorgner I haven't compared in detail, but I'd rather have this PR accepted before I start rebasing my patches and see which parts have been superseded by this PR. |
* replace loop copy with memcpy() * use ushort2bebytes() to set RSA modulus & exponent * use symbolic name SC_OPENPGP_KEYFORMAT_RSA_STD for the key import format
* check for RSA modulus & exponent lengths not being a multiple of 8 * make sure RSA modulus & exponent lengths are always set * remove a left-over RSA setting from the EC code
|
There are about 22 other places in libopensc that do (x+7)/8. If you create
a macro consider replacing these.
How many other places where this should also be done?
…On Thu, Nov 1, 2018, 1:30 AM Peter Marschall ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pkcs15init/pkcs15-openpgp.c
<#1506 (comment)>:
> if (pubkey->u.rsa.exponent.data == NULL)
- goto out;
- memcpy(pubkey->u.rsa.exponent.data, key_info.rsa.exponent, key_info.rsa.exponent_len>>3); /* 1/8 */
-
-out:
- if (key_info.rsa.modulus)
- free(key_info.rsa.modulus);
- if (key_info.rsa.exponent)
- free(key_info.rsa.exponent);
+ goto err;
+ memcpy(pubkey->u.rsa.exponent.data, key_info.u.rsa.exponent, key_info.u.rsa.exponent_len>>3); /* 1/8 */
@alex-nitrokey <https://github.com/alex-nitrokey> To make sure there are
enough bytes available even for bit lengths which are not a multiple of 8,
simply use the formula: number_of_bytes = (number_of_bits + 7) / 8.
Maybe a macro can help to increase code legibility, e.g.:
#define BYTES4BITS(num) (((num) + 7) / 8) /* number of bytes necessary to hold 'num' bits */
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1506 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA00MYP21jgfM0jMeZPUOS9dKjYKuSddks5uqrEzgaJpZM4XZ5Ad>
.
|
In theory it could be useful to renew the codebase of OpenSC. However, I think this would be like opening Pandora's box since there are many things that newer C code would do differently. I think that there are other things that are more pressing and equally work intensive. For example, having a good documentation would be more useful than just having a nice look of the code. |
|
I am sorry that I couldn't work on this for some weeks, but now I feel like I solved most of the issues. As I told before this PR (how it is now) should make all the OpenPGP part compatible with ECC keys. Nevertheless, OpenSC is (as far I can see) not ready yet to:
Therefore, I can not fully test the implementations. This PR is still an improvement even before these operations are supported because:
|
a few cleanups
|
ECC keys can not be used to decrypt. They can only sign ECDASA or derive ECDH The closest thing to "decryption" is derivation where some form of ECDH is done on both sides. OpenSC has support for ECDH. See NIST.SP.800-56Ar3.pdf pkcs11-tool can be used to do key derivation, mostly as an example. Each party does the following Given the certificate of the other party, as $FILE, extract the public key from the certificate: in a script do somrting like: Both parties will produce the same string. |
src/pkcs15init/pkcs15-openpgp.c
Outdated
| key_info.u.ec.oid.value[key_info.u.ec.oid_len] = -1; | ||
|
|
||
| /* Prepare buffer */ | ||
| key_info.u.ec.ecpoint_len = (required->field_length>>2) + 1; |
There was a problem hiding this comment.
Could you explain why you need exactly this length?
There was a problem hiding this comment.
Honestly, you really got me here... I needed to think about this for a really long time and realized, that I probably just understood something wrong when I did this. Funnily I reversed it in card-openpgp.c afterwards anyway as you can see in the commit.
I looked through the usage of ecpoint_len very carefully and now I am quite confident that the issue is fixed.
Thanks for looking at the code thorougly.
There was a problem hiding this comment.
Does this mean that this code fragment had an undetected bug? Did you verify the correctness with your new change?
There was a problem hiding this comment.
In part, I would say it did.
The problem is that I am not able to fully test the correctness of the results as e.g. storing an ec key fails on behalf of OpenSC. Key generation on card worked and works fine though. This is due to the fact that I first divided the length by four and then tricked myself by multiplying 4 again (as seen in the last commit). I am sorry for this mistake.
There was a problem hiding this comment.
I see. Then, however, I'm asking myself if we need this code at all if nobody ever looks into what you've encoded on the card. Could this be verified with gpg?
There was a problem hiding this comment.
When I started to implement ECC key support I assumed that importing ECC keys is working in OpenSC. Thus I implemented both, key generation and import alike with all needed subfunction. We could disable the key import but I felt like it would not hurt to have the already implemented code until the import is working for OpenSC.
In general I - of course - excpect the code to work. I am not so sure about the fingerprint function and there might be some other issues though.
I can not compare/verify with gpg as long as I can not import keys. Even then it is a tricky task as there is no ready-to-go way to move a S/MIME based key to OpenPGP standard. (Currently, I am looking for a way to get this a bit more fluid in the long run.)
There was a problem hiding this comment.
If you plan to add the missing features in the future, then we can already merge the prerequisite now.
There was a problem hiding this comment.
That would be great. I can not promise that I'll be able to do this anytime soon though, I am afraid.
* pgp: initialize ecc keys for OPC3 * Add supported ECC algorithms by card version * Add tasks identified so far * pgp: Recognize ECC set on card * pgp: get_pubkey_pem read ECC pubkey from card * pgp: minor code changes for ECC compatibility * pgp: expand sc_cardctl_openpgp_keygen_info to hold ec info * Fix segfault problem in pkcs15-pubkey.c * pgp: enable key generation with pkcs15-init and ECC * pgp: adapt calculate_and_store_fingerprint to accept ECC * pgp: adapt rest of pgp_gen_key and subfunctions to accept ECC * pgp: add kdf parameters for ECDH fingerprint calculation * pgp: enable key import with pkcs15-init and ECC * pkcs15-pubkey: fix_ec_parameters onlz accpets explicit data or named_curve * Fix some mistakes during merge * More clean up for PR * Fix some ugly alignments * Improve code readability * Prevent unitialized variable by using FUNC_RETURN * OpenPGP: add length check * pgp: save exponent length in bits for sc_cardctl_openpgp_keystore_info_t * pgp: length checks and reallocations * pgp: oid init added * OpenPGP: slightly re-factor pgp_update_new_algo_attr() * replace loop copy with memcpy() * use ushort2bebytes() to set RSA modulus & exponent * use symbolic name SC_OPENPGP_KEYFORMAT_RSA_STD for the key import format * OpenPGP: slighly re-factor pgp_parse_and_set_pubkey_output() * check for RSA modulus & exponent lengths not being a multiple of 8 * make sure RSA modulus & exponent lengths are always set * remove a left-over RSA setting from the EC code * pgp: adding BYTES4BITS * pgp: initialization of values in pgp_build_extended_header_list based on key type * pgp: add BYTES4BITS and remove unnecessary tests * Fix broken pgp_update_new_algo_attr * pgp: fix the ecpoint_len variable
This enables basic ECC support for OpenPGP Card v3 and hereby fixes #1351.
While key generation works without problems, the import is still not working. This seems to be based on missing support in OpenSC (the key itself it rejected) and will be further tested/analysed). This code includes all changes needed for the OpenPGP Card specific parts of OpenSC though.
As everything expected to use RSA keys only in the past, some major changes needed to be made.
The code is tested with OpenPGP Card v2 and v3 and will be further tested by me the next weeks. Thus, this is a first main PR, some minor ones may follow.