FINeID: Initial FINeID v3 support #1608

enyone · 2019-02-16T07:29:04Z

New driver fineid, signing tested, decipher untested
Add custom 0x30 asn1 sequence parsing as struct
Improve asn1 parse logging when choice type not resolved
ACL parsing not implemented (hardcoded as select/read only)
Fixes No PKCS11 support for Finnish ID Card version 3 #1504
Documentation is added or updated
New files have a LGPL 2.1 license statement
PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

- New driver fineid, signing tested, decipher untested - Add custom 0x30 asn1 sequence parsing as struct - Improve asn1 parse logging when choice type not resolved - ACL parsing not implemented (hardcoded as select/read only)

enyone · 2019-02-16T08:58:14Z

Environment: VSVER=12, DO_PUSH_ARTIFACT=yes; Configuration: Release; Platform: x86
https://ci.appveyor.com/project/LudovicRousseau/opensc/builds/22418217/job/4wic3mom71r658fv

Build started
git clone -q https://github.com/OpenSC/OpenSC.git C:\projects\opensc
fatal: unable to access 'https://github.com/OpenSC/OpenSC.git/': Could not resolve host: github.com
Command exited with code 128

frankmorgner

Hi, thanks for your contribution! Please don't be overwhelmed by the quantity of the comments, but your code is in raw shape and needs some refinement.

Looks like your card comes from Oberthur, but does the Applet as well? If the Applet is a standalone Java Card implementation, then you should get rid of card-oberthur.c's habits and do your own thing. If not, then you should reuse the Oberthur card driver as good as possible.

As general advice, ...

please use the internal capabilities that are present as good as possible
don't add code for stuff that wasn't tested

frankmorgner · 2019-02-18T11:43:36Z

src/libopensc/asn1.c

- return NULL;
- } else
- if (tag_in & SC_ASN1_CONS)
+ if(tag_in != SC_ASN1_TAG_SEQUENCE_2) {


The parser is capable of parsing sequences, why are you modifying the code here?

If you want to parse the tag without being split up into its parts, use SC_ASN1_TAG_SEQUENCE|SC_ASN1_CONS with SC_ASN1_OCTET_STRING, see https://github.com/OpenSC/OpenSC/blob/master/src/tools/goid-tool.c#L87

frankmorgner · 2019-02-18T12:14:52Z

src/libopensc/card-fineid.c

+ NULL, 0, NULL
+};
+
+/* static int fineid_get_pin_reference (struct sc_card *card,


please remove comments, that are not needed

frankmorgner · 2019-02-18T12:22:50Z

src/libopensc/card-fineid.c

+
+static const struct sc_aid fineid_cm_aid = {
+ {0xA0, 0x00, 0x00, 0x00, 0x63, 0x50, 0x4B, 0x43, 0x53, 0x2D, 0x31, 0x35}, 12
+};


please avoid data duplication, e.g. by using

static const struct sc_aid fineid_cm_aid = { aid_FINEID, lenAid_FINEID};

frankmorgner · 2019-02-18T12:32:56Z

src/libopensc/card-fineid.c

+
+
+int
+fineid_select_card_manager(struct sc_card *card, const struct sc_aid *aid)


You're using an older version of card-oberthur.c as basis. Please have a look at the current implementation. Here, card-oberthur.c uses gp_select_card_manager.

Additionally sc_select_file allows selection via AID (SC_PATH_TYPE_DF_NAME) without the need for so much boiler plate, see iso7816_select_file.

frankmorgner · 2019-02-18T12:34:24Z

src/libopensc/card-fineid.c

+
+
+static int
+fineid_select_aid(struct sc_card *card)


function naming is misleading.

frankmorgner · 2019-02-20T13:05:13Z

src/libopensc/card-fineid.c

+ return SC_ERROR_INVALID_ARGUMENTS;
+ }
+
+ else if (ilen > 96) {


use a speaking name for magic constants

frankmorgner · 2019-02-20T13:05:21Z

src/libopensc/card-fineid.c

+
+ /* If no algorithm flags given, try to figure flags out from pkcs1 prefix */
+ if(fineid_get_algo(_driver_data->algorithm_flags) == FINEID_ALGO_HIGH_NA &&
+ ilen != 20 && ilen != 28 && ilen != 32 && ilen != 48 && ilen != 64) {


use a speaking name or a comment for magic constants

frankmorgner · 2019-02-20T13:08:55Z

src/libopensc/card-fineid.c

+ rv = sc_transmit_apdu(card, &apdu);
+ LOG_TEST_RET(card->ctx, rv, "APDU transmit failed");
+ rv = sc_check_sw(card, apdu.sw1, apdu.sw2);
+ LOG_TEST_RET(card->ctx, rv, "Block send with offset failed");


is this code working? If the input is a hash, then you're hashing the hash, which will be the wrong input for the signature.

frankmorgner · 2019-02-20T13:13:20Z

src/libopensc/card-fineid.c

+ switch (cmd) {
+ case SC_CARDCTL_GET_DEFAULT_KEY:
+ return fineid_get_default_key(card,
+ (struct sc_cardctl_default_key *) ptr);


did you test pkcs15-init with your card? If not, don't implement SC_CARDCTL_GET_DEFAULT_KEY

frankmorgner · 2019-02-20T13:14:40Z

src/libopensc/card-fineid.c

+ rv = iso7816_logout(card, FINEID_PIN_PUK);
+ LOG_TEST_RET(card->ctx, rv, "PUK PIN logout failed");
+
+ LOG_FUNC_RETURN(card->ctx, SC_SUCCESS);


You should first logout for all PINs, collect the errors and then return otherwise you may leave some PINs authenticated.

enyone · 2019-02-20T20:50:16Z

Thanks for the comments.

I shall consider (personally) priorities as follows:

error checking, possible memory leaks, that pin logout thing; prio 1
dead code, code duplication, reuse of existing implementations; prio 2
magic values and other readability; prio 3

Will stick to prio 1 things for now. Asking more hands to deal prio 2 and 3 things from all others who are willing to get this pr in.

lindi2 · 2019-04-30T11:30:12Z

Nice work, I am able to file my taxes with this code :) Specifically the following combination works:

card with "--name" being "FINeID v3" that has already been activated
opensc fbca962
Debian 9, amd64
Firefox with /lib/pkcs11/onepin-opensc-pkcs11.so
suomi.fi authentication portal

LachlanGunn · 2019-10-06T04:10:55Z

Hello. Is there anything in particular that I can do to help this move along?

frankmorgner

Although the initial work was done and may work in some cases, the code is not easily maintainable and changes the behavior of the core code. I've made a couple of code comments that need to be resolved before this PR can be merged, see above.

enyone · 2020-04-04T07:10:44Z

Also I am still alive. Now that this quarantine thing has opened much more free time, it is probably a time to take a look of all the mess I've made during past few years to this planet. This being one among them.

The implementation is MVP for sure. I try to crawl down the comments from Frank again (thanks Frank). One big problem initially was that there were not enough documentation over is this card even an Oberthur card but I picked that as a base anyhow as it looked a closest match.

Should I have decided to refactor that already existing Oberthur driver, maybe, that is one topic to argue for sure, but I picked to create a new driver as no access to any Oberthur hardware to make sure no regression caused there. Of course the community could help us out over regression testing.

One question before I start doing anything; should we decide to continue with this dedicated driver even if it causes some imbricated and overlapping code-base to form? Or should we decide to continue investigating what already existing driver is the one to refactor further? I don't have good arguments to propose it to be Oberthur.

lindi2 · 2020-04-20T10:25:38Z

If it helps at all I can report that I have been using the code now for bit over a year almost daily without any major problems.

frankmorgner · 2020-04-20T19:40:44Z

@enyone please have a look at my individual comments.

Jakuje · 2020-04-23T17:38:37Z

src/libopensc/card-fineid.c

+fineid_check_sw(struct sc_card *card, unsigned int sw1, unsigned int sw2)
+{
+ return iso_ops->check_sw(card, sw1, sw2);
+}


No need to reimplement this with default ISO operation. Just remove the fineid_ops.check_sw = fineid_check_sw; too.

frankmorgner · 2020-11-25T08:35:08Z

Hi! What's the status of this pull request?

frankmorgner · 2021-01-08T15:30:28Z

Closing this issue due to inactivity. Please re-open the ticket if more input is available.

FINeID: Initial FINeID v3 support

fbca962

- New driver fineid, signing tested, decipher untested - Add custom 0x30 asn1 sequence parsing as struct - Improve asn1 parse logging when choice type not resolved - ACL parsing not implemented (hardcoded as select/read only)

frankmorgner requested changes Feb 20, 2019

View reviewed changes

frankmorgner requested changes Jan 6, 2020

View reviewed changes

Jakuje reviewed Apr 23, 2020

View reviewed changes

frankmorgner closed this Jan 8, 2021

jpakkane mentioned this pull request Jun 9, 2022

FINeID: Initial FINeID v3 support #2567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINeID: Initial FINeID v3 support #1608

FINeID: Initial FINeID v3 support #1608

enyone commented Feb 16, 2019 •

edited by frankmorgner

enyone commented Feb 16, 2019 •

edited

frankmorgner left a comment

frankmorgner Feb 18, 2019

frankmorgner Feb 18, 2019

frankmorgner Feb 18, 2019

frankmorgner Feb 18, 2019

frankmorgner Feb 18, 2019

frankmorgner Feb 20, 2019

frankmorgner Feb 20, 2019

frankmorgner Feb 20, 2019

frankmorgner Feb 20, 2019

frankmorgner Feb 20, 2019

enyone commented Feb 20, 2019

lindi2 commented Apr 30, 2019

LachlanGunn commented Oct 6, 2019 •

edited

frankmorgner left a comment

enyone commented Apr 4, 2020 •

edited

lindi2 commented Apr 20, 2020

frankmorgner commented Apr 20, 2020

Jakuje Apr 23, 2020

frankmorgner commented Nov 25, 2020

frankmorgner commented Jan 8, 2021



		int
		fineid_select_card_manager(struct sc_card card, const struct sc_aid aid)

FINeID: Initial FINeID v3 support #1608

FINeID: Initial FINeID v3 support #1608

Conversation

enyone commented Feb 16, 2019 • edited by frankmorgner

enyone commented Feb 16, 2019 • edited

frankmorgner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enyone commented Feb 20, 2019

lindi2 commented Apr 30, 2019

LachlanGunn commented Oct 6, 2019 • edited

frankmorgner left a comment

Choose a reason for hiding this comment

enyone commented Apr 4, 2020 • edited

lindi2 commented Apr 20, 2020

frankmorgner commented Apr 20, 2020

Choose a reason for hiding this comment

frankmorgner commented Nov 25, 2020

frankmorgner commented Jan 8, 2021

enyone commented Feb 16, 2019 •

edited by frankmorgner

enyone commented Feb 16, 2019 •

edited

LachlanGunn commented Oct 6, 2019 •

edited

enyone commented Apr 4, 2020 •

edited