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

Solve few bugs #1110

Closed
wants to merge 1 commit into from
Closed

Solve few bugs #1110

wants to merge 1 commit into from

Conversation

FeitianSmartcardReader
Copy link
Contributor

@FeitianSmartcardReader FeitianSmartcardReader commented Jul 24, 2017

  1. Solve generate RSA Key error
  2. Fixes ePass2003 EC not supported #1073
Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • Tested with the following card:
    • tested PKCS#11
    • tested Windows Minidriver
    • tested macOS Tokend

1. Solve generate RSA Key error
2. Solve issue->#1073 (comment), add ECC support
3. Code passed test with
https://github.com/OpenSC/OpenSC/wiki/Smart-Card-Testing
@frankmorgner
Copy link
Member

In the last pull request you wrote:

The reason is our epass2003 series support plain APDU and cipher APDU, while in cipher APDU, after receive 6C command, then we will need get data back and re-package APDU with cipher mode, at this time, the set_le API seems only modify the LE, and nothing change of APDU, so we will modify apdu.c to solve it.

I don't fully understand what you mean by this, but I still wonder if it's possible to leave the layer above src/libopensc/card-epass2003.c untouched and realize this within your card driver. Can you show your specification with an example APDU trace or an example use case?

@FeitianSmartcardReader
Copy link
Contributor Author

@frankmorgner here is compare log, have a look, in 0.16 version, it works, not work in 0.17 version, thanks.
log_right.txt
log_wrong.txt

@frankmorgner
Copy link
Member

0xb725ea80 13:38:07.180 [pkcs15-init] apdu.c:339:sc_check_apdu: Invalid Case 4 short APDU:
cse=04 cla=0c ins=a4 p1=00 p2=00 lc=2 le=0
resp=0xbfe74b57 resplen=0 data=0xbfe74acc datalen=2
0xb725ea80 13:38:07.180 [pkcs15-init] card-epass2003.c:1318:epass2003_select_fid_: APDU transmit failed: -1300 (Invalid arguments)
0xb725ea80 13:38:07.180 [pkcs15-init] card-epass2003.c:1374:epass2003_select_fid: APDU transmit failed: -1300 (Invalid arguments)
0xb725ea80 13:38:07.180 [pkcs15-init] card-epass2003.c:1528:epass2003_select_path: SELECT FILE (DF-ID) failed: -1300 (Invalid arguments)
0xb725ea80 13:38:07.180 [pkcs15-init] card.c:770:sc_select_file: 'SELECT' error: -1300 (Invalid arguments)

You're sending an sc_apdu_t case 4 with resplen == 0 . The problem above is entirely in the scope of your card driver or your SM implementation. I cannot track down the problem, but setting some breakpoints should easily reveal the code that initialises resp, but not resplen!

@FeitianSmartcardReader
Copy link
Contributor Author

wrong2

@frankmorgner we had misunderstanding, the issue is not short APDU, it is about return data, check attachment screenshot, thanks

@frankmorgner
Copy link
Member

Please fix the problem I've pointed out above. At the very least, it clutters the log with irritating error messages.

Regarding the excerpt of the log you've pointed out, the only difference I can see is that log_right.txt uses a short length APDU and log_wrong.txt uses an extended length APDU. Please check if changing the APDU from extended to short fixes the problem. If so, there is no need to change anything except src/libopensc/card-epass2003.c or src/libopensc/pkcs15-epass2003.c.

Please note that I don't have time left looking through your logs. Please fix this problem within your card driver and try not to touch the layers above.

@dengert
Copy link
Member

dengert commented Jul 31, 2017

Is the problem related to d757db2 ?

The trace shows lines 375, 387 and 390 being executed which (IMHO) means line 382 is not executed.
That could mean SC_APDU_FLAGS_NO_SM = 1.

sm.c appears to be where SC_APDU_FLAGS_NO_SM = 1.

In looks like SW=6C1B causes the previous command to be reissued with the new length of 1B which then causes the SW=6988.

Does epass2003_sm_get_wrapped_apdu need changes to address d757db2 ?

@FeitianSmartcardReader
Copy link
Contributor Author

@dengert yes, you are right. that the issue, so we modify the few files to solve this issue, I will talk internal if possible do modify epass2003_sm_get_wrapped_apdu only, thanks

@dengert
Copy link
Member

dengert commented Aug 1, 2017

@FeitianSmartcardReader Also read #972 and related #970, #975. If you card is sending back a SM fragmented response, the new code will reassemble it before presenting it to you driver.

@FeitianSmartcardReader
Copy link
Contributor Author

@dengert Thanks for your reply, will check and do modify, keep update, thanks

@frankmorgner
Copy link
Member

@FeitianSmartcardReader Do NOT modify apdu.c! I don't know how to make this any clearer... Despite the last reviews, you are still modifying it (this time setting SC_APDU_FLAGS_NO_SM for your card).

If you absolutely need this modification, it's imperative to post a detailed explanation (i.e. don't say "problem solved"). In particular, you should rule out the three ideas I've given above which may solve the issue.

For further communication, please again do a detailed analysis with your technical staff (read: don't waste my time), thanks.

@dengert
Copy link
Member

dengert commented Aug 2, 2017

Here is something else to look at:
In log_write.txt line 738, 739: Outgoing APDU

0C B2 01 04 00 00 0E 97 02 00 D8 8E 08 86 CE 2B ...............+
84 77 8A 64 8A 00 D8

This is an extended APDU. Then after determining the length needs to be changed, the new APDU in 763, 764:

0C B2 01 04 0D 97 01 1B 8E 08 38 F2 0A 0D 2E 78 ..........8....x
92 7C 1B 

is a short APDU with the new length of 1B

But in #1110 (comment) above, and in log_wrong.txt the APDU sent with the corrected length is an extended APDU:

0C B2 01 04 00 00 0E 97 02 00 D8 8E 08 00 1E 69 ...............i
01 23 58 43 7C 00 1B 

with SW = 69 88

So why is it using a short APDU in 0.16.0 and extended APDU in the new code?
And why in both cases is the initial APDU extended? The length 1B is returned as a single byte.
Are extended APDU really needed at all?

@dengert
Copy link
Member

dengert commented Aug 4, 2017

This may be a generic problem with the handling of Wrong Length status codes and not just a epass2003 problem. The epass2003 may be the first SM card to run into the problem. See possible fix below.

Looking closer at #1110 (comment) and log_right.txt and log_wrong.txt:
The APDU:
'0C B2 01 04 00 00 0E 97 02 00 D8 8E 08 86 CE 2B 84 77 8A 64 8A 00 D8'
is sending an authenticated SM message with a '97' tag and a '8E' tag. The '97' appears to be the Le value requested: '00 DB', and the '8E' is the checksum.

The command fails with:
'99 02 6C 1B 8E 08 6D DC AE 50 67 3C C8 E8 6C 1B'
With tag '99' being the SW protected by a '8E' checksum and the actual SW of '6C 1B' of Wrong Length with the new length of 1B

In the failing case sc_set_le_and_transmit in apdu.c is called, and it retries the command by only changing the apdu->resplen and apdu->le then calls sc_single_transmit with:

0C B2 01 04 00 00 0E 97 02 00 D8 8E 08 00 1E 69 01 23 58 43 7C 00 1B

But does not work for a SM authenticated message because the message still contains the original Le of '00 DB' and original '8E' checksum, but the actual Le is now the '00 1B' as requested by the card.

This looks like a bug in apdu.c that sc_set_le_and_transmit should not be called for a SM message to just retry with the new length. With an SM message, the response should be verified, and a new APDU should be generated which contains the new length.

In previous versions, the SM code is called to handle all errors and would then generate a new APDU with the the new length and checksum as shown in log_right.txt: lines 743-764

99 02 6C 1B 8E 08 6D DC AE 50 67 3C C8 E8 6C 1B ..l...m..Pg<..l.
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:1004:epass2003_sm_free_wrapped_apdu: called
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:972:epass2003_sm_unwrap_apdu: called
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:185:epass2003_check_sw: Wrong length; correct length is 27
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:992:epass2003_sm_unwrap_apdu: unwrapped APDU: resplen 0, SW 6C1B
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:993:epass2003_sm_unwrap_apdu: returning with: 0 (Success)
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:1025:epass2003_sm_free_wrapped_apdu: returning with: 0 (Success)
0xb71f8a80 13:35:19.274 [pkcs15-init] sm.c:182:sc_sm_single_transmit: returning with: 0 (Success)
0xb71f8a80 13:35:19.274 [pkcs15-init] apdu.c:382:sc_single_transmit: returning with: 0 (Success)
0xb71f8a80 13:35:19.274 [pkcs15-init] apdu.c:401:sc_set_le_and_transmit: called
0xb71f8a80 13:35:19.274 [pkcs15-init] apdu.c:371:sc_single_transmit: called
0xb71f8a80 13:35:19.274 [pkcs15-init] apdu.c:378:sc_single_transmit: CLA:C, INS:B2, P1:1, P2:4, data(0) (nil)
0xb71f8a80 13:35:19.274 [pkcs15-init] sm.c:133:sc_sm_single_transmit: called
0xb71f8a80 13:35:19.274 [pkcs15-init] sm.c:134:sc_sm_single_transmit: SM_MODE:200
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:1037:epass2003_sm_get_wrapped_apdu: called
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:858:epass2003_sm_wrap_apdu: called
0xb71f8a80 13:35:19.274 [pkcs15-init] card-epass2003.c:1077:epass2003_sm_get_wrapped_apdu: returning with: 0 (Success)
0xb71f8a80 13:35:19.274 [pkcs15-init] reader-pcsc.c:283:pcsc_transmit: reader 'Feitian2003 00 00'
0xb71f8a80 13:35:19.275 [pkcs15-init] reader-pcsc.c:284:pcsc_transmit: 
Outgoing APDU (19 bytes):
0C B2 01 04 0D 97 01 1B 8E 08 38 F2 0A 0D 2E 78 ..........8....x
92 7C 1B         

Note that the new APDU is a short rather then an extended but that does not appear an issue. It has the new Le in the '97' tag and that matches the actual Le of '1B' and used to work.

POSSIBLE FIXES:
sm.c sc_sm_single_transmit sets needs:
apdu->flags |= SC_APDU_FLAGS_NO_RETRY_WL;
for every SM apdu.

or the card's card->sm_ctx.ops.get_sm_apdu could set SC_APDU_FLAGS_NO_RETRY_WL;

@frankmorgner
Copy link
Member

Nice find. Do you know at which point we made the transition from "SM code handles the error" to apdu.c handles the error? I don't remember that we changed something like that recently...

Also note that though setting SC_APDU_FLAGS_NO_RETRY_WL for SM APDUs does make sense, I wonder why the card responds with 6C 1B in the first place. Sounds like a problem with the previous command...

@dengert
Copy link
Member

dengert commented Aug 4, 2017

You asked: "Do you know at which point we made the transition from "SM code handles the error" to apdu.c handles he error?"
I believe this is a consequence of trying to handle 61XX and use GET RESPONSE to assemble a fragmented SM response in d757db2.

I don't see how any SM encoded APDU could be retried for Wrong Length, as the Le is included in the data sent and authenticated and/or encrypted.

So this might be a fix for any SM message:

diff --git a/src/libopensc/sm.c b/src/libopensc/sm.c
index a5f53de..39a897c 100644
--- a/src/libopensc/sm.c
+++ b/src/libopensc/sm.c
@@ -156,8 +156,8 @@ sc_sm_single_transmit(struct sc_card *card, struct sc_apdu *apdu)
                LOG_TEST_RET(ctx, rv, "cannot validate SM encoded APDU");
        }
 
-       /* send APDU flagged as NO_SM */
-       sm_apdu->flags |= SC_APDU_FLAGS_NO_SM;
+       /* send APDU flagged as NO_SM  and do not retry wrong length */
+       sm_apdu->flags |= SC_APDU_FLAGS_NO_SM | SC_APDU_FLAGS_NO_RETRY_WL;
        rv = sc_transmit_apdu(card, sm_apdu);
        if (rv < 0) {
                card->sm_ctx.ops.free_sm_apdu(card, apdu, &sm_apdu);

I have no way to test it, but @FeitianSmartcardReader could try it, and someone with DNiE could also test it.

@FeitianSmartcardReader
Copy link
Contributor Author

@dengert Already test, and it works.

@dengert
Copy link
Member

dengert commented Aug 7, 2017

If my change solved your problem, please submit a new pull request with the change. I am on vacation.

@FeitianSmartcardReader
Copy link
Contributor Author

@dengert Will do it tomorrow, thanks

@FeitianSmartcardReader
Copy link
Contributor Author

close the communication, to make issue clearly, will submit another pull request.

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.

ePass2003 EC not supported
3 participants