Minidriver code improvement proposition

[vletoux]

Hi,

I've checked the code of the minidrivier with the Microsoft quality tool cmck.exe
There are warnings / errors returned by cmck.exe which can be fixed easily

For example:

• not all boggus input parameters are checked (in CardAcquireContext, if the memory allocation function is null, there is a crash)
• the v7 minidriver functions does not point to an empty implementation nor a v7 connection is rejected
• there is no check to reject a "v8" connection
• the function CardChangeAuthenticatorEx is empty
  ...

I'm voluntary to fix them, but given the fact that there will be a lot of changes in the file minidriver.c, I'll like to have you approval before

(if you are doing changes at the same time, merging the modification will be difficult)

I'm waiting to here from you

regards,
Vincent

[vletoux]

Another screenshot ...

[viktorTarasov]

Dedicated GIT branch can be created and MSIs built with CI .
No one will disturb you there.
I do not think that in the nearest time someone will be actively contributing in MD.

[frankmorgner]

I agree that there most likely won't be any conflicts. Before your OpenSSL fix, there has no been activity for almost a year on the minidriver.

In the last couple of weeks we tried to improve the code quality, but there is still a long way to go. Reworking the minidriver fits perfectly. If the changes are really as complex as you say, then review might take some time so don't expect to see them included in the upcoming release.

[vletoux]

Let me rephrase what I wanted to they:
On my experience for fixing this issues , you do not change the core of the function but you add a bunch of "if" at the beginning of the function to test for bogus parameters (it helps debug bogus application by returning an error instead of running code). But there are a lot of functions and as a consequence a lot of "if".

My opinion is that a "simple change" - opposed to a change in functionalities - (because it is at the beginning of the functions, because cmck.exe stress it a lot when testing and because it is a copy past from my other minidriver implementation) but I admit than a lot of lines changed can you make feel that it is not a "simple change"

In short : a lot of additions - mainly "if" and at the beginning of functions - but no change in the core implementation. That's why I'm asking before making a patch.

[viktorTarasov]

Here is dedicated branch:
https://github.com/OpenSC/OpenSC/tree/minidriver-cmck-issues-i426
and linked Jenkins jobs
https://opensc.fr/jenkins/view/Minidriver-cmck-issues-i426/

[vletoux]

I've made a lot of changes.
What's remaining:

• pinpad firewall error handling
• compilation warning about deprecated functions

The most visible change I've made is the pinpad dialog. It's open to comment about its design.

@viktorTarasov : if possible, I'd like some help to understand why my branch cannot be automatically merged into the main branch (i suppose it'll be an issue)
To fix the compilation warnings, I'd like to have some insight about what function is supported (sprintf_s, ...).

[viktorTarasov]

@vletoux:

... why my branch cannot be automatically merged into the main branch

I do not sure that automatic merge of PR can be done in GitHub.

You do better if you push directly into the dedicated branch in OpenSC.
Or to install in your github repo the webhook directed to jenkins -- so that jenkins will build your own repo.

[viktorTarasov]

@vletoux
Do you consider that your minidriver contributions are ready to be integrated in the master?

For me it's the main essential issue to finalize new release.

[vletoux]

@viktorTarasov
Yes, I consider my minidriver contributions are ready to be integrated with the master.
Two thinks remains:

• the icon of the pinpad dialog (see minidriver: change the icon of the pinpad dialog #455)
• when the smart card is removed while the pinpad is asked on the pinpad dialog, the logon icons (tile) disappears. I think the issue was present before but invisible because i couldn't be tested (smart card logon with pinpad wasn't working - now it works, but with at least 5 pinprompts !)

I've also #454 which is optional.
#453 (inclusion of MTCOS cards) is independant of the change, but it is important to me. If #453 is accepted, the file win32/customactions.cpp will need to be modified to add the MTCOS ATR registration.

So ok for the minidriver change. If #453 is accepted, the custom action file will need to be modified to add the ATR registration. #454 is optional for this release.

[viktorTarasov]

You branch is merged (slightly rebased) into master.

Tested with "Gemalto IAS/ECC" the main certificate operations: generate, import, authentication, signature and decrypt.

Still to test are the smartcard logon, PIN change and unlock in logon.

[vletoux]

@viktorTarasov , I see at least 2 problems:
Output is set to verbose in the minidriver (certutil -scinfo output to console)
the masktech smart card is not recognized.

I'll back to you with a fix & a test procedure for the other functions
(I think my previous tests were wrongs because i discovered a previous entry in the calais database which make Windows use my old test version rather that the new build)

vincent

[vletoux]

@viktorTarasov everything is working for me with #459

A) Here is how to test Pin change

1. press control-alt-del
   
   change password
   
   other credential / signing options
   
   smart card
   

2. Download the Gemalto minidriver manager too (http://www.gemalto.com/products/dotnet_card/resources/development.html - best for debugging)
   

Press Change PIN

*B) Unblock PIN *
The Unblock PIN of the Gemalto MD tool doesn't work because it uses an admin challenge / response login.
It is the same process than the integrated login prompt which can be enabled next to the change pin prompt in the control-alt-del screen.
A custom program needs to be written or as an alternative, use cmck.exe

C) Smart card logon
Quick procedure
Do everything as a domain admin to avoid permission issues
Install a Windows 2008 stand alone
Create a domain
Install ADCS (active directory services)
run gpupdate /force to force the installation of the domain controller certificate

a) read/ write smart card
run certmgr.mmc => Personal, all task, request a new certificate => select the smart card logon template

b) read only smart card
Add the certificate using the smart policy configuration wizard stage 3 (http://www.mysmartlogon.com/download/#SmartPolicy)

Note: if the root certificate is not available on the card itself, install it as a root CA on the local account. Smart Policy will find it.

[viktorTarasov]

Fine, thanks.

[khanhnd1405]

I want to check opensc-minidriver of opensc on WinXp, I must do what to check this.
Please support to help me!

[frankmorgner]

@khanhnd1405 open a seperate issue for this