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

Fix reading EstEID certificates with T=0 #1193

Merged
merged 6 commits into from
Nov 17, 2017
Merged

Fix reading EstEID certificates with T=0 #1193

merged 6 commits into from
Nov 17, 2017

Conversation

metsma
Copy link
Contributor

@metsma metsma commented Nov 9, 2017

Fixes #1190

Signed-off-by: Raul Metsma [email protected]

Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • Tested with the following card: EstEID
    • tested PKCS#11
    • tested Windows Minidriver
    • tested macOS Tokend

@frankmorgner
Copy link
Member

Why do you need the global flag SC_CARD_CAP_READ_BINARY_NO_BREAK if you solve the problem with GET RESPONSE at the card driver level?

@metsma
Copy link
Contributor Author

metsma commented Nov 9, 2017

#1190 (comment)
As dengert suggested to avoid spliting sc_read_binary

if (count > max_le && !(card->caps & SC_CARD_CAP_READ_BINARY_NO_BREAK)) {
Then your version of read_binary would get control with the original count and would simplify your drivers version of read_binary.
So it looks like you need a read_binary routine in your card driver. The problem is sc_read_binary will try and break up any read_binary into multiple read_binary commands where the second one will have an index != 0. sc_read_binary before you get control. max_le = sc_get_max_recv_size(card);

@dengert
Copy link
Member

dengert commented Nov 9, 2017

@frankmorgner The name SC_CARD_CAP_READ_BINARY_NO_BREAK could be changed if you want.
I was surprised how many card drivers have their own version of read_binary. read_binary is not a standard as one might expect. The SC_CARD_CAP_READ_BINARY_NO_BREAK tells sc_read_binary to just call the card's version.

@metsma
Copy link
Contributor Author

metsma commented Nov 9, 2017

Any suggestion? I copied from dengert comment

@frankmorgner
Copy link
Member

A long time ago I thought about putting all this GET RESPONSE magic from apdu.c into iso7816.c. If i remember correctly, there are many more extensions from ISO 7816 that seeped through to the generic layers of OpenSC. With such separation, you would have an easier job of modifying the card specific behavior before some ISO 7816 mechanism kicks in. I think this would be the right way to go in terms of establishing long term software quality. However, this has a lot of potential to break things.

Introducing more complexity (read: hacks) doesn't solve the problem in the long run. I'd suggest to either start working on the refactoring now, which could then be included in 0.19.0 or to solve this problem exclusively on the card driver level.

@dengert
Copy link
Member

dengert commented Nov 9, 2017

The problem the card driver does not get control until after sc_read_binary has screwed thing up.

And sc_read_binary is called from non-card driver files card-.c, pkcs15-.c and pkcs15init/* ./libopensc/pkcs15.c calls it 5 times, ./libopensc/dir.c, /libopensc/iso7816.c, ./libopensc/card.c, ./tools/opensc-tool.c, ./tools/netkey-tool.c,
./tools/cryptoflex-tool.c, ./tools/westcos-tool.c and ./tools/opensc-explorer.c.
It is also called from card driver files: card-.c, pkcs15-.c and pkcs15init/*

So, refracting looks like a big project.

@frankmorgner
Copy link
Member

What happens if you use max_recv_len = 255 in opensc.conf (or in the code for the card's read_binary)? Does the card still respond with 61 00?

@dengert
Copy link
Member

dengert commented Nov 14, 2017

Is this an issue with T=0 and T=1 or the reader. IIRC, with T=0 the card does not see the Le, only the
reader does. Would a PCSC trace show more?

https://github.com/metsma, to you have another reader that does T=0 , or can you force T=0 to see if it fails or works?

@metsma
Copy link
Contributor Author

metsma commented Nov 15, 2017

Yes seems like it helps
Default: https://www.dropbox.com/s/4oavydmuuxm4hc2/opensc-debug.log.broken?dl=0
Modified config: https://www.dropbox.com/s/igfqkp4pdp9f5rl/opensc-debug.log.config?dl=0

frankmorgner
frankmorgner requested changes Nov 16, 2017
View reviewed changes
src/libopensc/card-mcrd.c Outdated
apdu.data = EstEID_v35_AID;
apdu.datalen = sizeof(EstEID_v35_AID);
apdu.resplen = 0;
apdu.le = 0;
Copy link
Member

Choose a reason for hiding this comment

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

please fix the indenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is fixing indenting, before there was spaces and tabs mixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or you mean leave the mixed indenting as it is?

Copy link
Member

Choose a reason for hiding this comment

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

There are too many tabs. Look at GitHub's diff, you'll see the problem.

src/libopensc/card-mcrd.c Outdated
sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "SELECT AID: %02X%02X", apdu.sw1, apdu.sw2);
if (apdu.sw1 == 0x90 && apdu.sw2 == 0x00) {
if (card->reader && card->reader->active_protocol == SC_PROTO_T0)
card->max_recv_size = 255;
Copy link
Member

Choose a reason for hiding this comment

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

please add a comment why you're doing the workaround

src/libopensc/card-mcrd.c Outdated
SC_TEST_RET(card->ctx, SC_LOG_DEBUG_NORMAL, r, "APDU transmit failed");
sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "SELECT AID: %02X%02X", apdu.sw1, apdu.sw2);
if (apdu.sw1 != 0x90 && apdu.sw2 != 0x00) {
SC_TEST_RET(card->ctx, SC_LOG_DEBUG_NORMAL, r, "APDU transmit failed");
Copy link
Member

Choose a reason for hiding this comment

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

indenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, spaces and tabs mixed before and do I leave as it is?

@metsma
Add comment
a860292 
Signed-off-by: Raul Metsma <[email protected]>
@metsma
Update indenting
a00c4f4 
Signed-off-by: Raul Metsma <[email protected]>
@metsma
Update indenting
e407157 
Signed-off-by: Raul Metsma <[email protected]>
@frankmorgner frankmorgner merged commit 514f898 into OpenSC:master Nov 17, 2017
@frankmorgner
Copy link
Member

thanks

@metsma metsma deleted the esteid_t0 branch November 17, 2017 09:51
@dengert dengert mentioned this pull request Nov 19, 2017
Merged
6 tasks
metsma added a commit to metsma/OpenSC that referenced this pull request Nov 23, 2017
@metsma
Fix reading EstEID certificates with T=0 (OpenSC#1193)
1c28c1b
@frankmorgner frankmorgner mentioned this pull request Feb 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankmorgner frankmorgner frankmorgner requested changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@metsma @frankmorgner @dengert