[Pcsclite-muscle] Client code review request

[Ludovic Rousseau]

2014-11-12 15:12 GMT+01:00 David Woodhouse <dwmw2 at infradead.org>:
> I would be grateful if someone could cast an eye over the code at
> http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/c24046b93 which I have added to the OpenConnect VPN client for supporting TOTP/HOTP keys from a Yubikey NEO.
>
> I wish client applications *didn't* have to get involved with
> libpcsclite and specific hardware details — imagine what the world would
> be like if we had to do the same for SSL keys, and PKCS#11/OpenSC didn't
> exist. But this is at least a functional start.
>
> Have I done the correct thing with the locking, using
> SCardBeginTransaction and reselecting the applet each time? It seems to
> make this code interoperate happily with OpenSC talking to the PIV
> applet on the same device.
>
> Is it a problem that I keep the card handle around for the entire
> lifetime of the VPN session? I assume I'm preventing another
> SCardConnect() from getting SCARD_SHARE_EXCLUSIVE access to the device
> but should I *care* about that, enough to hook up some kind of "all
> authentication is done now and you can call SCardReleaseContext()" event
> sooner in the lifetime of the program? Using pcsc-spy it looks like
> OpenSC is keeping *its* handle open, so I haven't bothered.
>
> Are there TLV handling functions/macros that I should be using instead
> of reinventing the wheel?
>
> And what else have I done wrong that I *haven't* even thought about or
> noticed? :)
>
> Any constructive feedback would be gratefully received. Thanks.

Some remarks;

Do not #include <pcsclite.h>. #include <winscard.h> should be enough.


+               ret = yubikey_cmd(vpninfo, pcsc_card, PRG_ERR, _("list
keys command"), list_keys, sizeof(list_keys), buf);
+               if (ret)
+                       continue;

If you "continue" then you will use the next reader without calling
SCardEndTransaction() and  SCardDisconnect().


+          SCardReleaseContext(pcsc_card);
This is bogus.
You need to use SCardDisconnect(pcsc_card, SCARD_LEAVE_CARD); to
release a card handle.

It is always a good idea to check the value return by a function to
catch these kind of bugs :-)


Maybe I missed other problems

Bye

-- 
 Dr. Ludovic Rousseau