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

Avoid dnie_transmit_apdu in the dnie driver (#970) #975

Closed
wants to merge 2 commits into from

Conversation

rickyepoderi
Copy link
Contributor

This pull request is motivated by the issue #970. During that conversation some doubts arisen about how get responses should be processed inside SM communication. In the particular case of the DNIe all SM apdus work the following way:

  1. If in SM the apdu is encoded and converted in a CASE4 apdu.
  2. The apdu always returns 61XX (get response is needed).
  3. One or more plain (no encoded) get response apdus are needed to recover the data.
  4. The data is the real response apdu but encoded, it should be decoded and returned back to the caller.

So in order to avoid the use of a specific wrapper dnie_transmit_apdu it is needed to call to free_sm_apdu after the get responses loop. That means changes in apdu.c and sm.c. The main idea is calling recursively to sc_transmit_apdu inside sc_sm_single_transmit but marking the apdu as NO_SM. It's the same idea provided by @dengert but minimally modified to be more similar to the existing flags.

Please test this pull with other SM cards. The changes are at the OpenSC core and could affect other SM implementations. I personally have only checked it with DNIe.

For more information see #970.

Copy link
Member

@frankmorgner frankmorgner left a comment

Choose a reason for hiding this comment

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

Looks good and reasonable to me.

But there is still this other piece of code that assumes GET RESPONSE commands are SM protected themselves. I suppose this change will break @viktorTarasov's use of SM, so I hope he finds the time to comment on this.

@@ -279,6 +279,8 @@ typedef struct sc_file {
* returns 0x6Cxx (wrong length)
*/
#define SC_APDU_FLAGS_NO_RETRY_WL 0x00000004UL
/* APDU is from Secure Messaging */
Copy link
Member

Choose a reason for hiding this comment

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

explanation could be better.

@dengert
Copy link
Member

dengert commented Feb 25, 2017

Looks good. As @frankmorgner pointed out other piece of code needs to be looked at. Only called if card->ops->get_response returns error and resp_len != 0.

@viktorTarasov should have a look.

@rickyepoderi
Copy link
Contributor Author

I have realized that the apdu.c was modified by @germanblanco not very long ago in the previous try to integrate dnie into sm (e9f94d7).

IMHO I think that not a lot of cards use the sc_get_response function and, even less, in conjunction with SM. Nevertheless that sm related code in the function should be managed (removed or protected by another if that check SM condition). But I really think that part is never executed.

Two more things:

  • If there are cards that need SM with the get_response apdu we will need to change a bit the present pull. (For example add the flag inside get_sm_apdu, each card can do SM or not inside get response, and execute sc_transmit_apdu or card->reader->ops->transmit depending the same flag. The name of the flag should be changed to something more meaningful.) But I prefer to wait until current SM cards will be checked.
  • @frankmorgner if the change goes on change the flag comment to whatever you like or ask me to do it.

Thanks!

@dengert
Copy link
Member

dengert commented Feb 28, 2017

See discussion in #632 as to why e9f94d7 has the code as;
} while (rv != 0 && minlen != 0);

I don't think that needs to be changed.

It is not a question of: "IMHO I think that not a lot of cards use the sc_get_response function",
if we have any cards that use it, we have to keep it working. And my favorite card, PIV, uses it a lot.

I don't see how you could have: "that assumes GET RESPONSE commands are SM protected"
because the get_response can not be issued on its own, it is always a type 2 APDU and is returning data that could not be transmitted for the previous apdu, i.e. either the original apdu or a previous get_response.
It even has its own section in ISO 7816-4 "7. Transmission-oriented Interindustry Commands"

The only thing about a get response that might be different when using SM, is the CLA B4 and B3 i.e. the SM bits in the class byte. The card may be expecting them because it is trying to return the results of the previous SM protected command. That may be card dependent.

Still need @viktorTarasov to comment.

@rickyepoderi
Copy link
Contributor Author

I agree with you. My point was that the existence of that issue (it complicated the loop of get_responses) is an indication of not many cards using that code or not very intensively. I still believe the same.

The first point of my previous comment is just an idea in case this pull don't get through. A way of making the changes needed by dnie without affecting current paths of execution.

We really need testing the pull with current SM cards (authentic and epass). I suppose that Viktor is our guy here, so better wait and see.

@viktorTarasov
Copy link
Member

Sorry for the delay,
will try to test it during the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants