-
Notifications
You must be signed in to change notification settings - Fork 713
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
Conversation
There was a problem hiding this 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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explanation could be better.
Looks good. As @frankmorgner pointed out other piece of code needs to be looked at. Only called if @viktorTarasov should have a look. |
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:
Thanks! |
See discussion in #632 as to why e9f94d7 has the code as; 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", I don't see how you could have: "that assumes GET RESPONSE commands are SM protected" 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. |
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. |
Sorry for the delay, |
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:
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.