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.

Sign up for GitHub

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

Jump to bottom

pkcs11-tool: send DER for EC public keys with default compilation flags #1287

Merged
merged 1 commit into from
Mar 19, 2018
Merged

pkcs11-tool: send DER for EC public keys with default compilation flags #1287

merged 1 commit into from
Mar 19, 2018

Conversation

aalba6675
Copy link
Contributor

@aalba6675 aalba6675 commented Mar 18, 2018 •
edited

[UPDATE] PR has been modified according to @DDvO's comment.
By default EC_POINT_NO_ASN1_OCTET_STRING is undefined and you will get standards behaviour.
Define this to get the write to send plain bytes.

This PR specifically fixes the default behaviour of pkcs11-tool. Makefile defaults to an undefined EC_POINT_NO_ASN1_OCTET_STRING and therefore EC public keys are written as plain bytes instead of with DER wrapping.

  1. pkcs11_get_ec() assumes DER encodig of pubkeys - fix proposed libp11#79
  2. https://bugzilla.mozilla.org/show_bug.cgi?id=480280
    This will make pkcs11-tool consistent with the standard.
Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • Tested with the following card:
    • tested PKCS#11 (SafeNet HSM)
    • tested Windows Minidriver
    • tested macOS Tokend

@dengert
Copy link
Member

dengert commented Mar 18, 2018 via email

@aalba6675
Copy link
Contributor Author

aalba6675 commented Mar 19, 2018 •
edited

@dengert - SafeNet is doing the right thing by expecting DER intead of plain binary bytes. This PR is about the default compilation behaviour of pkcs11-tool to make it conform to the standard and the rest of OpenSC.
As it stands pkcs11-tool is sending plain binary bytes using default Makefile.

@DDvO in #79 writes "It came to my surprise that PKCS#11 in this case requires an ASN.1 octet (DER) encoding, while for RSA key material this is not the case. I confirmed this from ftp:https://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-30/pkcs-11v2-30m1-d7.pdf. Judging also from the discussion on Bugzilla there are several implementations that are non-compliant in the same way as my card."

@DDvO
Copy link
Contributor

DDvO commented Mar 19, 2018 •
edited

Looking again at the condition I implemented in June 2016:

#ifndef EC_POINT_NO_ASN1_OCTET_STRING

I now think that I did a mistake and I should have written

#ifdef EC_POINT_NO_ASN1_OCTET_STRING

instead; then the default behavior would already have been the standard-compliant one.
So instead of removing the conditional workaround as proposed in this PR,
it can also be kept and just corrected to

#ifdef EC_POINT_NO_ASN1_OCTET_STRING // workaround for non-compliant cards not expecting DER encoding
                gost->public.len   -= header_len;
                gost->public.value += header_len;
#endif

such that the default behavior is the compliant one,
while keeping the possibility to easily get a workaround for non-compliant cards when needed.

@aalba6675
pkcs11-tool: make ECPoint behaviour standards compliant by default
326cde2 
Fixes OpenSC#1286. The behaviour of pkcs11-tool will follow the standard -
send DER. If EC_POINT_NO_ASN1_OCTET_STRING is defined then it will
write plain bytes.
@aalba6675 aalba6675 changed the title pkcs11-tool: send DER for EC public keys unconditionally pkcs11-tool: send DER for EC public keys with default compilation flags Mar 19, 2018
@frankmorgner
Copy link
Member

@DDvO thanks for the clearification.

@frankmorgner frankmorgner mentioned this pull request Mar 19, 2018
Closed
@frankmorgner frankmorgner merged commit ea4baf5 into OpenSC:master Mar 19, 2018
@dengert
Copy link
Member

dengert commented Mar 19, 2018 via email

@DDvO
Copy link
Contributor

DDvO commented Mar 19, 2018

Good point that the driver should be responsible for dealing with any DER encoding incompatibility.
Yet the driver itself could be faulty in this respect.

Back in Aug 2016 I was testing a beta CardOS 5.3 middleware with a beta CardOS 5.0 card.
Meanwhile I'm pretty far away from this topic and cannot say if this still applies and what might have changed in newer driver/module versions.

@dengert
Copy link
Member

dengert commented Mar 19, 2018 via email

@aalba6675 aalba6675 deleted the no-ec-workaround branch March 20, 2018 03:32
@DDvO
Copy link
Contributor

DDvO commented Mar 20, 2018 •
edited

I've just tested writing an EC public key with the latest CardOS PKCS#11 module I have (of Marc 2017):

  V5.4 (Build 07) Beta Version
  CardOS-API-V5-4-7-512-20170222112927

to the latest card I have:

  token model        : CardOS V5.3, 201
  hardware version   : 120.0
  firmware version   : 201.3

Unfortunately, without the workaround I still get:

error: PKCS11 function C_CreateObject failed: rv = CKR_DEVICE_ERROR (0x30)

I do not know if this bug is in the CardOS driver or the card itself.

So thanks @aalba6675 for adapting your PR such that the workaround can be enabled easily when needed.

@dengert
Copy link
Member

dengert commented Mar 20, 2018

I object to the way this is implemented, It is a compile time change to an OpenSC utility to support a non standard PKCS#11 module that is still Beta. It means pkcs11-tool will not work with other complaint modules including the OpenSC modules. I realize that it is for EC only.

The PR should be rewritten to make it an run time option or to base it on the specific version of the module.

@DDvO
Copy link
Contributor

DDvO commented Mar 20, 2018

@dengert, it looks like you misunderstood the nature of the proposed change.
So far, the non-compliant variant has been the default, and by the change proposed here this workaround will not any more be the (compile-time) default.

BTW, the non-compliant behavior of CardOS (and presumably some other cards/drivers, too) has been around for long time in various (also non-beta) versions and their use is apparently wide-spread.

After the change, users who need the workaround will need to compile with EC_POINT_NO_ASN1_OCTET_STRING defined manually.
I don't know if it would be worth adding a run-time option for enabling the workaround.

@dengert
Copy link
Member

dengert commented Mar 20, 2018

What I am worried about is some distro will compile with EC_POINT_NO_ASN1_OCTET_STRING
That is why I would like to make sure this is not a compile time option.

@aalba6675
Copy link
Contributor Author

aalba6675 commented Mar 20, 2018 •
edited

Indeed this has happened — in the opposite sense. RHEL did not define this in its packaging and as a result the pkcs11-tool in RHEL is sending plain bytes instead of DER.

@DDvO
Copy link
Contributor

DDvO commented Mar 21, 2018

Oops. Luckily this only affects sending EC public keys to a token - a pretty rare scenario.
BTW, receiving exportable private keys (of any type) is still not supported at all by the pkcs11-tool.

After the change proposed in this PR, I'd consider the risk that someone accidentally enables the encoding workaround by defining EC_POINT_NO_ASN1_OCTET_STRING pretty low. In particular since this flag is hardly known to anyone.

@DDvO
Copy link
Contributor

DDvO commented Mar 21, 2018

After some other point made above, I just suggest improving the comment
// workaround for non-compliant cards not expecting DER encoding
to
// optional workaround for non-compliant tokens/drivers/modules expecting DER encoding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankmorgner frankmorgner frankmorgner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFE: make EC_POINT_NO_ASN1_OCTET_STRING a runtime option in pkcs11-tool
4 participants
@aalba6675 @dengert @DDvO @frankmorgner