Bug: Not issue enough GET_RESPONSE

[hongquan]

Hello,

I'm reading long data from a card which only support short APDU (Gnuk card).
To read this, we need multiple GET_RESPONSE command but OpenSC only issued 1 then stop.
Here is the debug log: http:https://hastebin.com/pametuleki.md
At line 7, card returned 61 00.
At line 10, OpenSC checked SW and flags. They were all OK and GET_RESPONSE was issued.
The problem then came: The iso7816_get_response() function changed the flag: line 12 & 13.
As a result, at line 46, the check was not OK and OpenSC did not issue GET_RESPONSE any more. We could not get enough data (line 53).

[martinpaljak]

The log is gone from the URL, but IIRC this was already discussed and the result was that the behavior of Gnuk device is not really standard in this context and thus the ISO7816 function is not supposed to work for it. Has something changed?

[hongquan]

Yes. The last discussion is about Gnuk did not return correct SW byte for GET RESPONSE. The author fixed it.
This new problem is that iso7816_get_response() changed the flag needed for GET_RESPONSE:

    /* don't call GET RESPONSE recursively */
    apdu.flags  |= SC_APDU_FLAGS_NO_GET_RESP;

and makes sc_transmit() issue only 1 GET_RESPONSE then stop.

    if (apdu->sw1 == 0x61 && (apdu->flags & SC_APDU_FLAGS_NO_GET_RESP) == 0)
        r = sc_get_response(card, apdu, olen);

[martinpaljak]

OK, I see it now. Indeed, current code in iso7816.c (the only implementation of get_response driver hook, which makes me question the usefulness of it) assumes that GET RESPONSE is only called once yet the code in apdu.c would call it until indicated as end of data by the card.

Short term solution: fix iso7816.c. In the long run it might be meaningful to keep the ISO7816 conforming behavior in apdu.c and avoid a few table lookups going through the "generic" and "driver" ops...

Do you have a patch?

[hongquan]

I don't have patch because I don't know how this change could affect other card drivers.

[martinpaljak]

I have two proposals for a patch:

1. fix iso7816.7 to "work properly" (smallest change) and I expect it works for all other cards that need it (mostly it gets triggered once with "normal" cards, as commands such as reading certificates etc use a offset into the file with READ BINARY), thus every command gets handled separately.

2. implement apdu.c in a way that get_response hook does not exist (shifting the iso7816.c method into apdu.c). Given that the hook is not implemented by not a single driver and that apdu.c is "also 7816-4" compatible component like iso7816.c, it makes perfect sense to keep that functionality there.

[hongquan]

Hi,
I choose the 1st option.
The 2nd one should be left to someone with more experience than me.

[hongquan]

Hi, I tried fixing iso7816.c (delete the line which changes SC_APDU_FLAGS_NO_GET_RESP flag). But now I found a new bug: The total response length is not accumulated via multiple GET_RESPONSE.

Here is the log with additional logging:
http:https://pastebin.com/gV1F8TQC

At the start of pgp_get_pubkey(), we prepared 320 bytes for response (line 2). This function called sc_transmit_apdu().
There were then two GET_RESPONSE, which acquired 256 and 14 bytes. So, the total is 270 (line 26 & 62).
But at the end, pgp_get_pubkey() only received 256 bytes from sc_transmit_apdu() function (line 86).

[martinpaljak]

Happy debugging ;)

[hongquan]

I found the logic error.
The GET_RESPONSE are called via recursive calls of sc_get_response()
pgp_get_pubkey() > sc_transmit_apdu() > sc_transmit() > sc_get_response() > iso7816_get_response()
This iso7816_get_response() in turn calls {sc_transmit_apdu() > sc_transmit() > sc_get_response() > iso7816_get_response()}.

We will use apdu.resplen, defined in 1st sc_transmit_apdu, to accumulate all response lengths from multiple subsequent GET_RESPONSE.
At very first, we set apdu.resplen to maximum, as the reserved room for all sub-response, and expect that after multiple calls, this variable will be set to the actual received length.
When apdu.resplen is accumulated step by step, we need something to remember the ceiling, i.e. the maximum reserved.
sc_transmit() uses olen variable to remember this ceiling value, and here the logic error comes.
We need the variable to be persistent from the first sc_transmit_apdu() to the last iso7816_get_response(), but unfortunately, it is redefined to be equal to apdu.resplen at the beginning of sc_transmit(), whereas apdu.resplen was meant to keep the accumulated received. So, at the second loop (inside sc_get_reponse()), the remained room is calculated buflen = olen - apdu->resplen = 0, which stops new response from being copied.

Debug log: http:https://pastebin.com/UQvyEuKK
Line 10 is apdu.resplen and olen at the starting. olen=320.
Line 13: We make sure that the remaining room is 320.
Line 20: sc_transmit() define new olen in its context, set to to 256. At this line, APDU has not been transmitted yet, apdu->resplen = 256 is expected response length for the first GET_RESPONSE only.
Line 50: New value of olen is transfered to sc_get_response()
Line 53: Because olen was reset, the remaining room is now ceiling - resplen = 0. No data is copied.

What's difficult to fix is, there is no way to transfer olen from line 10 to line 20, or there is no way to transfer the overall ceiling value from the first sc_transmit() to the second sc_transmit().

---

Update 1:
There is one thing I need to reconsider: the apdu variables in the two sc_transmit_apdu are not the same.

---

Update 2:
Because the GET_REPONSE is called recursively, the write has to be done backward. If the total response is 270 bytes, got in 2 parts /----256----/----14----/,
we write 14-byte part first, then write 256-byte part.
In current implementation, the starting point to write is determined based on apdu->resplen. To write backward, this value apdu->resplen has to decrease step by step, but then it is contradict to its initial purpose: to store accumulated response!

[martinpaljak]

That's why I said it might be a good idea to reap the call stack a bit and unite the functionality into a single location, where it is easier to follow, to bring some sense and understanding into it. The level of (useless) indirection in this specific location is way too high for no obvious benefit.

[hongquan]

Yes. You are right.
Who will take this job?

[martinpaljak]

Looking at your analysis I see you have it all figured out already ? :)

[hongquan]

Hi, Martin.
I rewrote the sc_transmit() to iterate over GET_RESPONSE on its own, instead of calling sc_get_response() > iso7816_get_response().

Do I need to remove those unused get_response function?

[martinpaljak]

Without looking at any code you've written, I can suggest that keeping functions with sensible size is desired, so having a (static) sc_get_response separated from (public) sc_transmit() for readability is good. Given that sc_get_reponse already now would give an error (and thus render the card useless) if the card would respond with 61XX (and thus need get_reponse), I think it is safe to remove the hook. I'll review the drivers one more time.

[hongquan]

Hi, here is my code:
hongquan@85bbb64#L0R521

The change is small and after change, the function is 56 lines long. So I think we don't need to split it out.

[viktorTarasov]

Sorry, probably I do not understood your problem.

Yes. The last discussion is about Gnuk did not return correct SW byte for GET RESPONSE. The author fixed it.
This new problem is that iso7816_get_response() changed the flag needed for GET_RESPONSE:

/* don't call GET RESPONSE recursively */
apdu.flags |= SC_APDU_FLAGS_NO_GET_RESP;

and makes sc_transmit() issue only 1 GET_RESPONSE then stop.

if (apdu->sw1 == 0x61 && (apdu->flags & SC_APDU_FLAGS_NO_GET_RESP) == 0)
r = sc_get_response(card, apdu, olen);

It's not true.
The sc_get_response() contains the do {} while loop to iterate on the data to be read,
sc_get_response() calls the card's or iso7816 get-response() (that's the reason why the last should not be recursive), controls it's SW, available space in output buffer, moves the pointer of the output buffer, etc.

I also have the read-public-key procedure that needs the get-response() procedure to be called two times (and other such cases), and ''it-works-for-me''.

I would propose you to post here high level logs of your problem after the invalid SW has been fixed and with the current master sources (or with a few debug messages more in the code).

[hongquan]

Hello @martinpaljak and @viktorTarasov
I re-tested and found out the old implementation of apdu.c is OK. The change of SC_APDU_FLAGS_NO_GET_RESP does not stop sc_get_response() from issuing enough GET_RESPONSE.

Which causes me to misunderstand this issue is one bug in OpenPGP driver, which did not provide enough buffer for "Short APDU" case.

So, no need to change OpenSC's core implementation. I'm closing this issue.

[martinpaljak]

Nevertheless, removing the unused get_response hook and moving the actual get_response code closer to where it is used (apdu.c) is a justified move.