Major changes for all cards that needs developer review

[maciejsszmigiero]

This pull request contains a few fixes mostly centered around OpenPGP card:

1. Remove raw RSA algo flag from OpenPGP card since it doesn't support it.

2. Initialize PIN logged_in field for OpenPGP card and other cards that do not support PIN info.
   This fixes Firefox PIN login problem on these cards introduced by commit 2f10de4 .

3. Make OpenPGP card user/signature PIN order match expected by _get_auth_object_by_name() function in OpenSC PKCS#11 framework.

4. Strip execute permission bits from two .c files that have them set.

Please see individual commit messages for details.

[mouse07410]

Your 2fd7aaf may fix the problem I mentioned in #842 (which, as I suspected, was introduced by 2f10de4). In any case, I think it should be merged.

[maciejsszmigiero]

Rebased pull request againt current master and updated it with three more commits:

1. Set PIN-PUK association for cards that don't have it set (like OpenPGP card) so sc_pkcs15_unblock_pin() will correctly select policy which the PUK will be checked against.

2. Improve handling of OpenPGP card PIN change and unblock commands,
   provide more debug messages for user in unsupported cases and use correct
   reference to PW1-mode 2 (82) in these commands.

3. Use correct size_t printf length modifier on Windows, since "%z" doesn't work there.

@mouse07410: While I don't know how Apple software handles C_GetTokenInfo() return values,
if it continues to prompt for PIN after you already had given it it would match Firefox behavior
that commit 2fd7aaf fixes.

PS. Looks like commit 6cd28cf broke one of the test builds on AppVeyor.

[mouse07410]

... if it continues to prompt for PIN after you already had given it it would match Firefox behavior
that commit 2fd7aaf fixes

Yes I've had this problem. My workaround was not to use Firefox for now. :-(

[maciejsszmigiero]

Rebased pull request against current master and updated with latest commits.
Some of them aren't strictly for OpenPGP card only, but are general improvements of OpenSC, just
were done while working with this card (doing a separate pull request for them would require a lot of
rebasing work, however).

Most important of them is adding GCC format checking attributes to log functions and going through
all warnings that were generated. There were a few issues there (size_t printed as int, too few / too many arguments, pointers printed as integers, long printed as int, etc.) but repeated many times
across the code.

Other warnings that were shown when building by GCC on 64-bit Linux, 32-bit and 64-bit mingw builds
were also (mostly) taken care of.

Also, minidriver is now buildable again on mingw (it looks like it bit rotted a little there).
It has now support for multiple PINs and allows PIN unblocking on Windows Ctrl+Alt+Del PIN unblock
screen.

Another important change is provide notification about and handle card resets by other contexts,
as most of the cards supported by OpenSC do not support the "logout" method required to
deauthenticate a card.
This means that that the only way to deauthenticate the card is to reset it.

Now, Windows components seems to deauthenticate card very often, this made at least some cards (for example OpenPGP card and cards having multiple applets) non-working due to lost card state.
Also, other users (like PKCS#11 library) don't know about the lost card state.

This all means that we need to catch card resets by other contexts in reader backends and provide higher layers with this information so they can decide what to do when it happens (reinitialize the card
as of now).

More elegant solutions might be possible for particular cards (like I see was done for PIV card
in bb2d863), but for now (at least until other cards are
checked and fixed) reinitializing a card seems simplest solution.

Minidriver is a bit special, since transaction begin (where usually the reader backend learns about
card reset) is handled by Base CSP (Windows component), so it has to keep track of other context
using this card on its own.

As usual, please see individual commit messages for details.

[frankmorgner]

Thanks for the work you put into removing compiler warnings and using additional compiler features! But I think they should go into a separate PR. Also the minidriver changes are interesting, but should be looked at in a separate PR since they concern all cards.

I think you should not check for winscard.h in configure.ac, because we are including a local file, instead of the global one (even in mingw32). Also, we are loading winscard.dll at runtime rather than at link time, which is why we don't need mingw32 to actually have winscard.lib. Also, you added a lot of boilerplate simply to build the minidriver in mingw32, why not simply say that it is not supported?

You should not force the developer to put cardmod.h into a specific folder (your check in configure.ac). Instead, use ./configure CFLAGS="-I/path/to/cardmod_h"

Adding SC_FORMAT_LEN_SIZE_T everywhere is not very readable. The C99 addition of printing %zu is available since Visual Studio 2015. I think we should simply live with the warning on VS2012 and before.

It seems that a7e180d is a hack that will very likely to be used by anyone...

I think that e04bf03 can be removed with #842 being accepted.

@CardContact could you look at 6cbf2bc?

[maciejsszmigiero]

Hi Frank, thanks for looking into it (and naturally for all your work with OpenSC!).

Thanks for the work you put into removing compiler warnings and using additional compiler
features! But I think they should go into a separate PR.
Also the minidriver changes are interesting, but should be looked at in a separate PR since they
concern all cards.

The problem was that compiler warning and card reset handling changes are pretty intrusive (and by that I mean they change a lot of places), so they collide with these in minidriver.
If the mindriver changes absolutely cannot be reviewed here then I will split them out and submit
later to not do rebasing twice.

I think you should not check for winscard.h in configure.ac, because we are including a local file,
instead of the global one (even in mingw32).

This check was added since "cardmod.h" from CNG requires this header, so it won't compile on
mingw32 versions that don't have it.
But this check can be removed if we don't care about old mingw32 versions.

Also, you added a lot of boilerplate simply to build the minidriver in mingw32,
why not simply say that it is not supported?

Well, it was working some time ago (that's why there still is a "--enable-minidriver" parameter for
configure), just needed a refreshment with regard to changes to minidriver code in recent times.
Besides that this only requires a 28-line header which mostly consists of directives like
"#define __deref" and a few little changes, so it isn't a big price to pay for ability to cross-compile full
OpenSC for Win32 and Win64 on Linux.

BTW. The changes are about mingw-w64 targeting both Win32 and Win64 and not mingw32 - sorry
if I didn't make it clear in commit messages and here.

You should not force the developer to put cardmod.h into a specific folder (your check in
configure.ac). Instead, use ./configure CFLAGS="-I/path/to/cardmod_h"

I will remove this check in next patch version.

Adding SC_FORMAT_LEN_SIZE_T everywhere is not very readable.
The C99 addition of printing %zu is available since Visual Studio 2015.
I think we should simply live with the warning on VS2012 and before.

Are you sure about this that "%zu" is available since Visual Studio 2015?
MSDN doesn't list it: https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

However, on older compiler (or CRT library) releases it is not only about warning, but about correctness: if a we use an unknown (at that version) conversion specifier it is unspecified what will happen - whether the argument is going to be printed as int (which on Win64 has different length) or skipped altogether.
It might work by coincidence but that's not something that should be relied upon.

I agree this it is ugly but it is better to use an ugly correct solution than nice looking one that is not right.
Also it can be changed later to "%z" via simple grep when all minimum supported compiler versions
support it.

Update:
There is an alternative option where SC_FORMAT_LEN_SIZE_T don't need to be added everywhere:
on affected compilers just scan the format string in sc_log() / sc_debug() and replace "z" and "t"
in conversion specification with "I".
This would limit SC_FORMAT_LEN_SIZE_T macro to cases where direct (f)printf-style call
was made.

It seems that a7e180d is a hack that will very likely to be used by anyone...
There is no other way to unblock a PIN using built-in Windows GUI that I am aware of.

I think that e04bf03 can be removed with #842 being accepted.

#842 is only for PIV card and cannot be used with minidriver (which is the main source of card resets),
since transaction begin (where usually the reader backend learns about card reset and from where changes introduced in #842 learn about it) is handled by Base CSP (Windows component).
You can see that "lock" method (for beginning a transaction in Windows smart card terminology) is set to NULL in cardmod reader driver, so it can't provide reset information to sc_lock().

[dengert]

#842 was designed to be used with OpenSC calling PCSC. And was designed that other card drivers could write there own routine.

The minidriver passes in handles to use. The check_reader_status look to see if the handle changes,
and uses the disassociate_card and associate_card to clean up. I have not looked at minidriver in years.
the CARD_DATA (PCARD_DATA) has the SCARDCONTEXT and SCARDHANDLE. Maybe there is some way to query if some other code did a reset? Does windows even tell you if a reset was done?

Look at sc_ctx_use_reader that passes in the Windows provided handles. to reader-pcsc.c sc_ctx_use_reader.

The look at what cardmod_init is doing. Note that
cardmod_ops.lock = NULL;
cardmod_ops.unlock = NULL;
So no locks are ever set by OpenSC. Windows might be doing some locking, its not clear.

[maciejsszmigiero]

The check_reader_status look to see if the handle changes,
and uses the disassociate_card and associate_card to clean up

Handles change when the card is reinserted / replaced.
They don't change when the card is reset.

Maybe there is some way to query if some other code did a reset?
Does windows even tell you if a reset was done?

Normally this happens during transaction begin ("sc_lock()" in OpenSC terminology) and
the reader is then reconnected to clear this state.
But this happens inside Base CSP not in minidriver and it looks like there is no documented
mechanism to notify minidriver about it.

The look at what cardmod_init is doing.
Note that cardmod_ops.lock = NULL; cardmod_ops.unlock = NULL;
So no locks are ever set by OpenSC. Windows might be doing some locking, its not clear.

As I said to Frank:

transaction begin (where usually the reader backend learns about card reset and from where
changes introduced in #842 learn about it) is handled by Base CSP (Windows component).
"lock" method (for beginning a transaction in Windows smart card terminology) is set
to NULL in cardmod reader driver, so it can't provide reset information to sc_lock().

In other words, "sc_lock" and "sc_unlock" equivalent operations are done by Base CSP.
Quoting Windows Smart Card Minidriver Specifications version 7.07:

A card minidriver should assume that transactions are handled by the
caller, if it uses SCRM to access the card.
The card minidriver can assume that all entry points except
CardDeleteContext are called by holding the card transaction.

"holding the card transaction" = sc_lock'ed

Multiple contexts can exist in a single process. Calling
CardDeleteContext on one process should not prevent the other
context from functioning.

We deinit card in CardDeleteContext, this means we have to be careful not to break other contexts
during this.

[dengert]

I should have taken more time to read the thread this morning. As you point out in 7.07: "A card minidriver should assume that transactions are handled by the caller" It also says: "Handling the authentication state of the card is also the responsibility of the caller, not the card minidriver."

As you point out card reset is the biggest problem and not having a "logout" function causes
the baseCSP to reset the card. NIST 800-73-4 part 2 "3.2.1 VERIFY Card Command" says: " If P1='FF', and Lc and the command data field are absent, the command shall reset the security status of the key reference in P2. The security status of the key reference specified in P2 shall be set to FALSE and the retry counter associated with the key reference shall remain unchanged." I would speculate Microsoft asked NIST to add this to solve the card reset problems.

The above is a logout APDU command: 00 20 FF XX 00. It is the missing command needed in a multi application environment. I have not found this in any other standards or know if any other cards also support this. I don't believe there are any PIV cards in the field yet that support this either.

If the OpenPGP applet or other applet support this it would cut out the need for card_reset.

Another problem, which I don't see an obvious solution, are card with multiple applets, where the user may want to use different applets from different applications. PIV and CAC on same card. PIV and OpenPGP on same card, (Yubico supports this.) Selecting the AID of one applet in one processes resets the security status, and also the AID. Another needed APDU command is to query which AID is active, to see if it has changed by some other application.

I have not spent much time on the OpenSC PIV driver when used with the OpenSC minidriver because there is not much use for it because Microsoft provides its own PIV minidriver. But a logout to try the 00 20 FF XX 00 command would be easy to add, and if it fails, let the baseCSP do what it does today.

Another issue which causes card_reset is the opensc.conf:

77 # What to do when disconnecting from a card (SCardDisconnect )
78 # Valid values: leave, reset, unpower.
79 # Default: reset
80 # disconnect_action = unpower;

The default is to always causes a card_reset. You may want to test with:

80 disconnect_action = leave;

Another issue is the way OpenSC matches cards. Doing a select AID to see if the applet is on the card may cause the card to reset it state, even if the AID is not on the card. So limiting the card drivers to cards used by the user can cut down on this problem.

[frankmorgner]

OK, I understand why #842 does not solve the problem in minidriver and why you need a dedicated detection mechanism for resets. But instead of doing a complete associate_card you could call vs->card->card_reader_lock_obtained(vs->card, 1). In fact, to solve the PIV issue with resets, I suggested to do a complete re-initialization as done in associate_card instead of creating a new driver callback (#816 (comment)), which obviously was rejected.

[frankmorgner]

I suggested to create separate PRs to accelerate adoption of the changes to the OpenPGP driver, which seemed to be your primary target. You may leave this PR as is and wait a little longer for others to do testing and review.

[frankmorgner]

@dengert the default disconnect/reconnect/transactionend actions for the cardmod driver are left at 0. This always leaves these actions at SCARD_LEAVE_CARD.

Update: for reference have a look at cardmod_init

[frankmorgner]

Yes, I am sure that %zu is implemented in VS2015.

[frankmorgner]

Some more people need to check all the changes that will influence all other cards!

[dengert]

The card_init may leave the action at SCARD_LEAVE_CARD for the minidriver. I was suggesting that if one also uses FireFox/NSS/PKCS#11/opensc, the action would be reset, that would cause problems with minidriver connections to the card.

[dengert]

In response to "OK, I understand why #842 does not solve the problem in minidriver and why you need a dedicated detection mechanism for resets."

I don't think on Windows you can have a dedicated detection mechanism for resets. This is based on 7.07 saying: "Handling the authentication state of the card is also the responsibility of the caller, not the card minidriver." I read this as Microsoft will not tell the minidriver the card was reset. And reset is not the only interference that other applications can cause. Other vendor's smart card middleware can also cause interference.

But if you can avoid other vendor's middleware and use only OpenSC minidriver and OpenSC libs, maybe there is a way on Windows to have the different applications share information about the state of the card. I have not done any windows programming in years, but there should be some way to share and set this information from multiple processes. The state should include what AID is active, what card driver is being used, the login state, and if objects or files have been changed on the card. Things that PCSCD does not keep track of. (Might be able to do this on Linux and MacOS too.)

[maciejsszmigiero]

@dengert:

It also says: "Handling the authentication state of the card is also the responsibility of the caller, not
the card minidriver."

Just a side note here: this spec is written with assumption that all smart card accesses are done via
CryptoAPI (that is, minidriver). It doesn't consider concurrent CryptoAPI / PKCS#11 accesses.
I have't yet read PKCS#11 specs closely enough to know what should PKCS#11 library do when
application logins to the card, then some other process resets the card and so the security state
(from quick glance it looked like it does not envision such situation).
Currently, from application perspective it looks like somebody has removed then reinserted the card
which is a safe option but maybe there is a better one.

I am currently looking if there is a better option overall regarding card reset issue, since what is
implemented with these commits is safe but annoying to people who have a card that doesn't support
deauthentication (logout) operation AND use it on Windows AND use it from CryptoAPI and PKCS#11
applications at the same time.
However, this requires at least reviewing what general options are possible for all card types to make
sure they all could be made working properly (I hope this will be possible).

If the OpenPGP applet or other applet support this it would cut out the need for card_reset.

I've tried various commands on OpenPGP card but none seemed to work.
It would still be kind of grey-area as specs don't define any logout command, however.

Another problem, which I don't see an obvious solution, are card with multiple applets, where the
user may want to use different applets from different applications. PIV and CAC on same card. PIV
and OpenPGP on same card, (Yubico supports this.) Selecting the AID of one applet in one
processes resets the security status, and also the AID. Another needed APDU command is to query
which AID is active, to see if it has changed by some other application.

Here, problem with knowing currently selected applet looks similar to me as situation with security
status (if it turns out that PKCS#11 does not allow the card to lose it), with problem of synchronizing
them across different contexts in different processes (or reloading at every transaction or "lock").
If changing applet resets the security status then these problems even intersect.

Another issue which causes card_reset is the opensc.conf:
77 # What to do when disconnecting from a card (SCardDisconnect )
78 # Valid values: leave, reset, unpower.

Yes, but if an user forces card reset via this setting then he shouldn't be surprised that something
breaks. This is different from minidriver deauthentication situation where handling a card reset is
required just to use it without needing to restart the application so card can be reinitialized.

But a logout to try the 00 20 FF XX 00 command would be easy to add, and if it fails,
let the baseCSP do what it does today.

You mean send it blindly to the card with XX replaced with every key reference possible on that card?
(since we need to deauthenticate them all in this minidriver method).
Do you know any non-PIV card where this could possibly work?

Another issue is the way OpenSC matches cards. Doing a select AID to see if the applet is on the
card may cause the card to reset it state, even if the AID is not on the card.
So limiting the card drivers to cards used by the user can cut down on this problem.

Good to know that, that basically means that multiple contexts in this case can break even without
explicit card reset, so this could happen even in cards with logout method defined (but at least there
is a workaround here).

@frankmorgner:

OK, I understand why #842 does not solve the problem in minidriver and why you need a dedicated
detection mechanism for resets. But instead of doing a complete associate_card you could call
vs->card->card_reader_lock_obtained(vs->card, 1).

Yes, but then I would need to implement this method for: acos5, asepcos, authentic, belpic,
cardos (some cards), entersafe, gemsafeV1, gpk, ias, iasecc, incrypto34, isoApplet, jcop, jpki,
masktech, mcrd, miocos, muscle, myeid, openpgp, setcos, tcos, westcos.

As we need to have reinitialize support in minidriver anyway for "handles changed" situation it was
easier to do full reinit for now until we have a global solution to "card loses state" problem.

I suggested to create separate PRs to accelerate adoption of the changes to the OpenPGP driver,
which seemed to be your primary target.

I will create a separate pull request for these small few-line commits which can be merged separately ( f984c96 cb9d8aa fa4174f 4e5ff7c ).
There is also #849 - which breaks certificate login on at least Firefox at least on OpenPGP card.
Should it be added as revert commit to this pull request or will discussion continue in that thread?

Some more people need to check all the changes that will influence all other cards!

It would also greatly help if people who maintain or work with cards that do not implement the logout
method would check if implementing this method is possible for their card.
Any card driver with implemented logout method is one card driver less that have to be made
reset-aware (as maybe we'll find some solution for AID changed issue) .

[dengert]

https://github.com/maciejsszmigiero,
In regards to: "You mean send it blindly to the card with XX replaced with every key reference possible on that card?"

No, XX is key reference the software used in the verify command sent earlier to login to the card.
I was referring to sending it to pre-NIST 800-73-4 PIV cards, were it would fail, and the piv driver could fall back to using card_reset. (or don't do anything if opensc.conf had disconnect = leave)

You said: "Do you know any non-PIV card where this could possibly work?"

No, i don't. I was asking if anyone else had seen a "logout" command like this in any other card or standard.

[frankmorgner]

#864 is merged, could you rebase this PR?

[maciejsszmigiero]

Rebased against current master, configure no longer forces developer to put cardmod.h into a specific
folder as Frank had suggested.

Also amended log messages format and parameter fixes by minidriver ones which were missing from
previous commits as minidriver logged most of its output via a dedicated log function.
This function needed separate format checking attributes before warnings about calls to it were shown.

[maciejsszmigiero]

Rebased against current master, added missing "-no-undefined" to src/smm LDFLAGS (this had
previously prevented building SMM as DLL on mingw) and added commit that moves SM test in
configure.ac after LIB_PRE and DYN_LIB_EXT variables assignment (since this test uses these
variables).

[viktorTarasov]

Sorry to say it so late,
but this PR includes too many of different, weakly linked, heavy changes to the common framework,
(in contradiction with it's title "few fixes mostly about OpenPGP card", which promised to be only few little changes in the card specific part),
that makes it too difficult to review and accept.

If it's not too late,
you can start from pure OpenPGP part and then gradually submit PRs for the common part.

[dengert]

@viktorTarasov
I totally agree with you. These are not "a few fixes mostly for OpenPGP".
There are some major changes, especially with the card reset/logout and the assumption that the user knows the PUK and the PUK can be associated with the PIN.

@maciejsszmigiero
I would suggest that all the log and printf issues be one PR.

The reset/logout should be its own PR, so others are aware that their favorite card needs testing, and your PR should point out any issues you see with selected cards, and how they might be addressed.

Minidriver only changes should be another PR.

Make sure the subject of the PR is not under stated: "A few fixes mostly about OpenPGP card" should have been: "Major changes for all cards that needs developer review and minor changes for OpenPGP."

[maciejsszmigiero]

@viktorTarasov

Sorry to say it so late,
but this PR includes too many of different, weakly linked, heavy changes to the common framework,
that makes it too difficult to review and accept.

That's why this PR is divided into 22 commits.
Every subject has its own commit with specific description what is changed and why.

(in contradiction with it's title "few fixes mostly about OpenPGP card", which promised to be only few
little changes in the card specific part),

I will change the PR description.

@dengert

There are some major changes, especially with the card reset/logout

The old behavior meant that cards than didn't support the logout method (that is, the majority of cards
that OpenSC supports) and kept some internal state (like selected AID or authentication status) were simply stuck in non-working state until application restart (or until the card was reinserted) on Windows with minidriver (or minidriver + pkcs11).
This meant such cards were effectively unusable on that platform.
This change isn't as risky as it looks at first glance, but naturally, as any, will benefit from wider testing
(do users test code from pull requests)?
If still there are questions then I will gladly answer them.

the assumption that the user knows the PUK and the PUK can be associated with the PIN.

Do you mean commit 5c82c8f here?
This commit only sets the PIN/PUK association for cards where it was unset and where this
association was either obvious or described in the code or specs.
Note that the old code blindly tested PUK against PIN (a different authentication) policy for these cards.

If you mean commit 1acde82 then this is an additional
functionality (it wasn't present at all before).
As with any new functionality, it can be described in Wiki when it actually can be used by users.

I would suggest that all the log and printf issues be one PR.
The reset/logout should be its own PR, so others are aware that their favorite card needs testing,
and your PR should point out any issues you see with selected cards, and how they might be
addressed.
Minidriver only changes should be another PR.

As I said previously the problem is these changes (mostly) depend on each other, for example all significant minidriver changes (that, is excluding minor things like commit b34a34a) depend on each other sequentially and also on card reset handling commit 7eff745.
Additionally, if I to split this PR to, let say, 5 concurrent PRs then I will need to test 5 separate versions
of code. And not just once, since there are usually some changes needed in the review process which
then require retesting.

If this process is done sequentially instead (that is, preparing next PR after closing the previous one) then at the current pace (for which I also am to blame since I usually cannot respond / rebase the changes immediately) I am afraid the whole process will take years.

Unfortunately, the log arguments fixes are the most intrusive changes (by the number of places changed).
On the other hand, if the log format checking commit is applied without actual fixes (this can be done
easily) then every build results in a flood of compiler warnings.

There are commits that look like they could be split out relatively easily, though - minidriver build fixes
on mingw.

Make sure the subject of the PR is not under stated: "A few fixes mostly about OpenPGP card"
should have been: "Major changes for all cards that needs developer review and minor changes for
OpenPGP."

Will do (without the OpenPGP part as small OpenPGP changes have actually already been merged in
#864).

[maciejsszmigiero]

Rebased against current master,
Added OpenSC license text to "cardmod-mingw-compat.h" file,
Removed dead code from minidriver,
Reverted a change that added an extra parameter to sc_lock() call that wasn't currently used.

[frankmorgner]

If you want to complete a78bc32: sc-hsm's sopin unblocks the user pin

[frankmorgner]

Regarding f2df771, I'm not sure if solving this in the minidriver is the ideal solution. If I'm not mistaken, we have the same problem with resets and with threads also in PKCS#11 or Tokend. @dengert tried to solve this partially by introducing card_reader_lock_obtained in the lowest possible layer. Isn't this what we should do here, too?

In some commit message you explained why SCardBeginTransmit (i.e. sc_lock) is not the right place to check for resets. Instead, you're calling sc_detect_card_presenceand check the result for SC_READER_CARD_RESET. Isn't this what we should change in the lower layers?

[frankmorgner]

Could you define this outside of md_set_cmapfile?

[maciejsszmigiero]

Will do.

[frankmorgner]

still contains the old locking parameter

[dengert]

Why did you change this?

[maciejsszmigiero]

@frankmorgner:

still contains the old locking parameter

Here I would prefer to leave this as-is, because this is a very small
change (only 5 changed places, mostly one-liners) and
I am afraid that this functionality (in contrast to a change that added
a new parameter to sc_lock()) might not be easy to add back if the
PC/SC reader driver changes too much.

In fact, I see that a significant rewrite of PC/SC reader driver is waiting to be
merged (issue #892).
Are you planning to merge that change soon?

It looks like this rewrite is going to conflict with changes from this PR so maybe
we can wait until #892 is merged, then I will rebase and we'll see how this
change fits in the rewritten driver?

[maciejsszmigiero]

@dengert:

Why did you change this?

Since the card is going to be reinitialized anyway on reset there is (currently)
no point in calling card_reader_lock_obtained or reopening sm channel.

[frankmorgner]

You removed the call to card_reader_lock_obtained. Was this intended?

[dengert]

Don't remove that the PIV driver, and any other driver that implements card_reader_lock_obtained
depends on it. For any card that does not define its on card_reader_lock_obtained the code is a noop.

[maciejsszmigiero]

In previous iteration of this changeset sc_lock() function was always
called with resetok = 0 while card_reader_lock_obtained
method would only get called and sm reopen would only happen
with resetok = 1.

This parameter was removed since Frank said in comment to
line introducing it that we shouldn't add unused code
(OK, this removed code wasn't strictly "added", it was already
there but it would be a dead code after resetok parameter removal).

[frankmorgner]

sm channel is not reopened on reset anymore

[maciejsszmigiero]

@frankmorgner:

sm channel is not reopened on reset anymore

It is, since a reset triggers card reinitialization and
this results in opening of a sm channel.

BTW. Old code had a difference in behavior there:
in init code the sm channel was opened only in
"Transmit" sm mode, but on reset it was opened
unconditionally.

[maciejsszmigiero]

@frankmorgner:

If you want to complete a78bc32: sc-hsm's sopin unblocks the user pin
Thanks for clarification, will amend that commit.

Regarding f2df771, I'm not sure if solving this in the minidriver is the ideal solution.
If I'm not mistaken, we have the same problem with resets and with threads also in PKCS#11 or
Tokend.
@dengert tried to solve this partially by introducing card_reader_lock_obtained in the lowest possible
layer. Isn't this what we should do here, too?

As I have written previously:

Unfortunately, the currently selected AID isn't the only thing that is lost on a reset.
Also the authentication state is lost - that is the main source of resets but it could be done by another
application - so to "recover the state" for other applications every card_reader_lock_obtained would
need to verify again all PINs that were previously verified.
And this alone gives a bit of corner cases (what if a PIN was typed on external PIN-pad?).
If there is a card that supports session objects then these would be lost, too.

If we allow authentication state to be lost then the PKCS#11 upper layer needs to know about it
since it needs to change PKCS#11 session(s) state and destroy private session objects.
So "fixing" it in a card_reader_lock_obtained method of card driver won't be enough.
Since this is an equivalent of pulling out a card and inserting it again this is handled exactly like it
in commit c210866.

(now this is a commit db5e03c).

In some commit message you explained why SCardBeginTransmit (i.e. sc_lock) is not the right place
to check for resets. Instead, you're calling sc_detect_card_presenceand check the result for
SC_READER_CARD_RESET. Isn't this what we should change in the lower layers?

As I said in the previous comment, higher layers (like PKCS#11) need to know about
card reset, too, so we can't "fix" the situation caused by reset transparently to them.

[dengert]

I STRONGLY object to the removal of the loop. Multiple applications may have been waiting on the lock, and as each one gets control, they may reset the card again. you really want to leave the while loop in place. I disagree with your logic: "Since the card is going to be reinitialized anyway on reset there is..." r = card->ops->card_reader_lock_obtained(card, was_reset); may do something different. You can change: r = card->reader->ops->lock(card->reader); to r = card->reader->ops->lock(card->reader, 0); But don't remove the while loop, and don't touch the

…

On 2/10/2017 8:52 AM, Maciej S. Szmigiero wrote: ***@***.**** commented on this pull request. -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- In src/libopensc/card.c <#850>: > @@ -397,41 +397,25 @@ int sc_lock(sc_card_t *card) return r; if (card->lock_count == 0) { if (card->reader->ops->lock != NULL) { - r = card->reader->ops->lock(card->reader); - while (r == SC_ERROR_CARD_RESET || r == SC_ERROR_READER_REATTACHED) { + r = card->reader->ops->lock(card->reader, 0); @dengert <https://github.com/dengert>: Why did you change this? Since the card is going to be reinitialized anyway on reset there is (currently) no point in calling card_reader_lock_obtained or reopening sm channel. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#850>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA00MbaojsmEDyjQbSWCrDWkghwF42LUks5rbHm-gaJpZM4JiX_s>.

-- Douglas E. Engert <[email protected]>

[maciejsszmigiero]

I STRONGLY object to the removal of the loop.
Multiple applications may have been waiting on the lock, and as each one gets control, they may
reset the card again. you really want to leave the while loop in place.

I can revert this to version before last respin of this changeset where the loop was present.

[dengert]

Thanks.

…

On 2/10/2017 10:08 AM, Maciej S. Szmigiero wrote: I STRONGLY object to the removal of the loop. Multiple applications may have been waiting on the lock, and as each one gets control, they may reset the card again. you really want to leave the while loop in place. I can revert this to version before last respin of this changeset where the loop was present. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#850 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA00Mf8qNBMNqaofXzMUI9Ga80CeaNKVks5rbItqgaJpZM4JiX_s>.

-- Douglas E. Engert <[email protected]>

[frankmorgner]

I created #979 to at least merge the accepted changes from this PR.

[frankmorgner]

@maciejsszmigiero, the rewrite of the pcsc driver is up for review in #983.

I'd like to merge most of your commits. However, I failed to cherry-pick them due to merge conflicts and dependencies to non-picked commits (see #979). If you could make a new PR excluding the following commits, I'll review and merge the others seperately:

• b83a806
• db5e03c
• f2df771

[maciejsszmigiero]

Created PR #996 with uncontroversial changes cherry-picked.
Will rebase these 3 remaining commits once that PR and #983 are merged.

[frankmorgner]

@maciejsszmigiero it's finally done, both PRs are merged.

[maciejsszmigiero]

Created PR #1070 which contains b83a806 (multiple PINs support) and non-reset part of f2df771, skipping the (controversial) card reset handling part for now.

[maciejsszmigiero]

All the changes from this pull request are merged now, besides a card reset handling.

It is clear, however, that there isn't a consensus yet how exactly to address this issue
(and handling of cards with multiple applets, which is a bit related matter) and that more research
is needed in this area.
That's why I will close this pull request for now, so it does not pollute the open PRs list.

For users of the majority of cards that do not support the "logout" method there is a simple workaround of disabling card resets completely, at a price of a bit less security.