Fix C_Login() crashing when using uninitialized card

[llogar]

pkcs11-tool --init-pin ... crashes if the card hasn't been initialized beforehand with pkcs11-tool --init-token ... (or by any other means) as p15card == NULL

Tested on IsoApplet javacard.

Checklist

• 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

[Jakuje]

The code change looks good.

At this moment, the tests for iso-applet are not very extensive, but would it make sense to try to add a regression test with a reproducer?

The least work would be dumping few commands into the .travis.yml, but much better would be creating some bash script containing these tests with comments that would be invoked from the .travis.yml. You are certainly more familiar with ISO applet so I believe you would be able to come up with few more use cases that would be nice to test.

[frankmorgner]

Did you check the other PKCS#11 entry points for this possible problem?

[llogar]

@Jakuje:

At this moment, the tests for iso-applet are not very extensive, but would it make sense to try to add a regression test with a reproducer?

I don't know. I've only noticed this as I am using a modified version of IsoApplet where among other improvements I can use PKCS#11 functions to initialise the token (C_InitToken & C_InitPin) instead of the IsoApplet "official" pkcs15-init --create-pkcs15 .... And also due to the issue below you actually couldn't invoke a C_InitToken() on an applet running in a simulator (and catch this bug):

ATR used in jcardsim config (3B:80:80:01:01) clashes with SmartCard-HSM with fingerprint sensor and PIN pad from opensc.conf which can have some undesired side effects - for example pkcs11_enable_InitToken setting is not honored in card_detect(). It would probably be wise to change it to something else.

The least work would be dumping few commands into the .travis.yml, but much better would be creating some bash script containing these tests with comments that would be invoked from the .travis.yml. You are certainly more familiar with ISO applet so I believe you would be able to come up with few more use cases that would be nice to test.

Perhaps adding pin change and pin unblock tests?

pkcs15-tool --change-pin --pin 123456 --new-pin 654321
pkcs15-tool --unblock-pin --puk 0123456789abcdef --new-pin 123456

Apart from these, I think the basics are pretty much covered...

@frankmorgner: No, but now that you have mentioned it, I have tried a couple more possible problematic scenarios (very unsystematic, I know) and it didn't crash. So I hope this was the only one...

[Jakuje]

Can you add these two cheks to the test and rebase on current master to get the CI green?

[Jakuje]

Good to see the results green and check added. Can you rebase the change on top of current master (instead of the merge commit you did?). Just git rebase upstream/master in your branch should do that.

[frankmorgner]

Why do you choose this ATR? I think, IsoApplet doesn't require any specific one...

[llogar]

I chose this specific ATR for no reason, any unique one would do.
As I explained in my answer to @Jakuje, If you leave the stock ATR, then you can not initialize the token using PKCS#11 functions (C_InitToken & C_InitPin) as pkcs11_enable_InitToken is taken from the card_atr 3B:80:80:01:01 section which "belongs" to one of the SmartCard-HSMs, unless of course you add pkcs11_enable_InitToken=yes to that section...

[frankmorgner]

Sigh, this seems to be a generic problem. Almost nowhere there's a check for p15_card != NULL 🙄

Also, I think we should to change the return code to CKR_TOKEN_NOT_RECOGNIZED

[frankmorgner]

I think I've found all the missing checks. Could you check if #2049 works as well? I've also changed back the ATR; having the same ATR for different cards with different applets should not be a problem.

[llogar]

Also, I think we should to change the return code to CKR_TOKEN_NOT_RECOGNIZED

per specification CKR_TOKEN_NOT_RECOGNIZED is not among the possible return codes of C_Login(). I don't know how strict are we about this...

[frankmorgner]

ATR used in jcardsim config (3B:80:80:01:01) clashes with SmartCard-HSM with fingerprint sensor and PIN pad from opensc.conf which can have some undesired side effects - for example pkcs11_enable_InitToken setting is not honored in card_detect(). It would probably be wise to change it to something else.

3B:80:80:01:01 is an unspecific ATR, which is why we're catching this ATR in card-sc-hsm.c only for specific stuff, but not for locking this into a specific card type. We have removed this from the default configuration. I think currently there's no way to avoid modifying opensc.conf for initializing a token via PKCS#11. This would mean that you can use any (specific or unspecific ATR to do this), right?

[frankmorgner]

per specification CKR_TOKEN_NOT_RECOGNIZED is not among the possible return codes of C_Login(). I don't know how strict are we about this...

Thanks for the pointer, I've now checked the return of CKR_TOKEN_NOT_RECOGNIZED to match the specified return values.

[llogar]

3B:80:80:01:01 is an unspecific ATR, which is why we're catching this ATR in card-sc-hsm.c only for specific stuff, but not for locking this into a specific card type. We have removed this from the default configuration. I think currently there's no way to avoid modifying opensc.conf for initializing a token via PKCS#11. This would mean that you can use any (specific or unspecific ATR to do this), right?

Right. And this brings me back to #1543.