Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenPGP card small fixes #864

Merged
merged 4 commits into from
Sep 20, 2016

Conversation

maciejsszmigiero
Copy link
Contributor

This pull request contains small fixes related to OpenPGP driver, picked from bigger pull request #850,
as @frankmorgner has suggested.

Specifically it contains the following changes:

  1. Remove raw RSA algo flag from OpenPGP card since it doesn't support it.

  2. Make OpenPGP card user/signature PIN order match expected by _get_auth_object_by_name() function in OpenSC PKCS#11 framework.

  3. Strip execute permission bits from two .c files that have them set,

  4. Improve handling of OpenPGP card PIN change and unblock commands,
    provide more debug messages for user in unsupported cases and use correct
    reference to PW1-mode 2 (82) in these commands.

_get_auth_object_by_name() in pkcs11/framework-pkcs15.c needs user PIN
to be the first one and then next one can be signature PIN, but OpenPGP
card had it reversed.

Signed-off-by: Maciej S. Szmigiero <[email protected]>
According to descriptions of commands "PSO: COMPUTE DIGITAL SIGNATURE",
"PSO: DECIPHER" and "INTERNAL AUTHENTICATE" in OpenPGP card spec (versions
1.1 and 2.1.1) the card adds / strips and checks PKCS#1 padding
automatically.
There is no documented way to perform raw RSA operations on this card so
SC_ALGORITHM_RSA_RAW flag shouldn't be set.

Signed-off-by: Maciej S. Szmigiero <[email protected]>
Some .c files had execute permission bit set needlessly.

Signed-off-by: Maciej S. Szmigiero <[email protected]>
"CHANGE REFERENCE DATA" (PIN change) and "RESET RETRY COUNTER"
(PIN unblock) commands in OpenPGP card have various limitations.
These also depend on whether the card is version 1.x or 2.x.

Provide helpful debug messages for user in case he is trying to do
a PIN command in a way that isn't supported by the card.

Also, take into account that version 2.x cards don't support references to
PW1-mode 2 (82) in these commands - change them to PW1 (81).

Signed-off-by: Maciej S. Szmigiero <[email protected]>
@frankmorgner
Copy link
Member

Looks good. However, I can't tell whether the internal magic of changing the pin length and pin reference is breaking something. @hongquan could you please verify the changes?

@hongquan
Copy link
Contributor

hongquan commented Sep 1, 2016

Looks good to me.

@frankmorgner
Copy link
Member

frankmorgner commented Sep 1, 2016

Is there any negative impact by changing the pin length and pin reference in your opinion/tests?

@maciejsszmigiero
Copy link
Contributor Author

Is there any negative impact by changing the pin length and pin reference in your opinion/tests?

PIN length is zeroed for version 1.x cards since they don't support explicit PIN in change or unblock
commands and would fail if you try to supply it with command (they rely on implicit result of last
verify command instead).

With regard to changing PIN reference:
Existing code has already changed PIN reference by or'ing it with 0x80 (unconditionally).

If you try to do PIN change or unblock with PIN reference of 0x82 on version 2.x card it will reject
such command, so this use case wasn't working anyway.

At the same time, encryption and authentication keys are protected by this PIN and its reference
needs to be specified as 0x82 during verify command (if you verify it as 0x81 these keys won't get
unlocked) so you can't simply remove this PIN reference and use only 0x81.

@@ -216,7 +216,7 @@ sc_pkcs15emu_openpgp_init(sc_pkcs15_card_t *p15card)

pin_info.auth_type = SC_PKCS15_PIN_AUTH_TYPE_PIN;
pin_info.auth_id.len = 1;
pin_info.auth_id.value[0] = i + 1;
pin_info.auth_id.value[0] = pin_cfg[i].reference;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain me more about this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Nguyễn,

Could you please explain me more about this change?

Did you read commit message for this change?
It's commit 0a6c1c4 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants