Le 10/06/2012 22:39, Frank Morgner a écrit :
> On Sunday, June 10 at 03:39PM, Viktor Tarasov wrote:
>> Le 10/06/2012 14:21, Frank Morgner a écrit :
>>> Uh, I think I am a bit late on this discussion...
>>>
>>> But I wanted to add a general concern that there are some conceptual
>>> problems with the SM branch (and the recently included patches in
>>> staging).
>>>
>>> 1. Global SM configuration is mixed with a configuration at the driver
>>> level. For example, look at [3]. It includes CWA, IAS/ECC data types
>>> which should be realized at the card driver level (or maybe some SM
>>> driver).
>> In [3] there is no IAS/ECC types.
>> There are data types related to the two more-or-less different SM
>> implementations: GlobalPlatform and CWA. It's not card specific.
> Well, well... I copied "CWA, IAS/ECC data types" from
> https://github.com/OpenSC/OpenSC/blob/staging/src/libopensc/sm.h:133.
> If there are no IAS/ECC types, it shows how wrong the documentation is.
> (By the way, there *is* some function called
> `iasecc_sm_external_authentication`, line 342)
You are perfectly right, the documentation is not perfect -- will be working on
it.
Do you see something card-specific in the data types?
The name of function can be easily changed, and will be changed.
Thanks to point it out.
> However, sm.h still requires you to know cwa structures and gp
> structures. This is typically what you would want to abstract with a SM
> layer. Look at opensc.h and cards.h, which offer a similar abstraction
> to the smart card driver. If you add a smart card driver to opensc, you
> only have to add sc_get_yourcard_driver to card.h and add it to
> internal_card_drivers in ctx.c. Effectively, you have to add only two
> lines to OpenSC's core files and your card driver is integrated. Now think
> about what you would have to change when adding a new SM implementation with
> your architecture.
The difference between libopensc and SM is that there is only one ISO
specification for the cards,
and there are at least two SM specifications -- GlobalPlatform and CWA .
In sm.h the SM data types include the sub-types specific for the SM
specification,
in the same manner as pkcs15-key data type includes the data types specific to
the different key types -- RSA, DSA, GOST, ...
>>> 2. There are at least two methods to hook into SM, using a SM module and
>>> implementing SM at the driver level. This conceptually introduces
>>> duplication. It is sure to be followed with code duplication. Both
>>> should be unified: Each card driver has a SM driver, which provides
>>> wrapping and unwrapping SM. A SM driver can then also be a SM module
>>> with external key sets.
>> Was it a question?
>> Yes, there are two methods to trigger SM wrapping: 'APDU transmit' and
>> 'ACL' mode.
> I don't understand ACL mode, because it isn't used anywhere. (This, by
> the way, begs the question why it is defined...) The two hooks I see is
> via module or via implementation by the card driver. I am advocating
> for merging sm_module_operations and sm_card_operations.
ACL mode is used when decision to-use or not-to-use SM for the next operation
is derived by the ACL for this operation.
Example is the iasecc card driver.
>> Duplication can happen due to fact that each card driver implement SM
>> as it wants, and can include into card specific part entire SM crypto
>> library.
> Code duplication is bad. It leads to error duplication and other
> problems.
Once more,
the card drivers are relatively free inside their card-specific part.
My humble opinion is that during the period of implementing of the common SM
framework,
we could tolerate some code duplication.
>> I do not contest that duplication can happen, but:
>> - it should not be obstacle to the implementation;
>> - please point out exactly, where in code you see duplication.
> - CWA implements SM according to ISO 7816 ([4], p67) and so does
> epass3000.
Once more,
card driver can do what it want if this do not hurts the common part,
or until the moment when the common SM framework will be installed.
> - About two years ago I have pointed at some places where bit padding is
> implemented multiple times (with different implementations) -- this still
> holds today.
Sorry, I will not be looking for where two years ago you pointed these places,
please, point them here and now .
If this bit padding takes more then few lines in codes, we will certainly
create some dedicated procedure.
>>> 3. As stated earlier, I am advocating for an additional abstraction
>>> between encoding and encryption. Thus, a SM driver would only
>>> implement encoding, offering an other abstraction to the crypto.
>>> Not implementing this abstraction has led to code duplication [1].
>>> And reinventing the wheel will continue, when all ISO-7816-like
>>> cards [2, for example] (or modules) offer their own implementation
>>> of:
>>> - TLV-encoding/decoding of cryptogram (with padding content
>>> indicator), mac and le (depending on the APDU case)
>>> - ISO padding of APDU header and data
>> As stated ealier, every card driver (at least at the beginning) can do
>> roughly what it wants inside it's card specific part. We have only
>> started to introduce the common SM framework, and for a while, we will
>> not blame the card driver if it includes it's own SM related crypto
>> procedures (... and thus duplicates the code. Is that the code
>> duplication that you are talking about?
> Yes, a good architecture can reduce code duplication. Again, look at
> OpenSC's card drivers. All can do what they want -- that's fine.
> However, most drivers are reusing the iso7816 driver adding some tweaks.
> For SM implementations, we should offer the same possibility.
Already answered: one specification for card, two specifications for SM.
>>> 4. General problems with code quality:
>>> - A lot of dead code pieces ("#if 0")
>> You mean the SM part or entire code base of OpenSC.
> Uh, I `grep`ped for '#if 0' and saw that this seems to be a common
> practice in OpenSC. However, new patches should not introduce new pieces
> of dead code. But the recent integration of SM into staging has has done
> just that.
Agree, 'however, should not'.
Currently not all functionalities are tested/ported from the other branches.
Will clean it out.
>>> - Usage of global switches instead of switches in the card context
>> What do you mean?
> g_sm in epass3000
Already answered,
'g_sm' is the epass2003 internal issue.
It has no relation to the common SM framework.
> By the way, is there a possibility to comment on code? I think, github
> only lets you comment on commits. I looked through the code yesterday
> and found some other problems, which would have been easy to report...
All these codes was pending long time in the dedicated branches and in the
github pull requests -- you could comment them there.
Now you can do it here.
>>> - No or wrong documentation of the SM stuff
>>> - directory "sm" should be renamed to "sm-modules"
>> ...
>>
>>> These issues, I think, disqualify the SM branch to be included in
>>> OpenSC's trunk.
>>>
>>> The biggest problem, I think is the coexistance of two independant SM
>>> hooks.
>> You simply cannot cover all SM use cases with one low level SM wrapping
>> method.
>> But understand you quite well -- you know/use only this one.
> I disagree. I think it is possible. Simply look at the interface to
> OpenSC's card drivers.
It's about ACL SM mode.
Two years ago I asked you:
how do you imagine to deal with more then one keysets for one card/application ?
to deal with the mixed card operations -- when not all card transactions has to
be SM wrapped?
Until now I have no answer.
>>> In general I dislike the concept of a SM module, because all
>>> OpenSC initialization magic is lost, when only the APDU buffer is pasfsed
>>> to the module. If a module is only used for external keysets, then it
>>> should do only encrypting/decrypting/authenticating instead of handling
>>> the whole smart card stuff.
>> External module do not handle all the smart card staff, but it has to
>> do part of it. Example: when importing, the RSA key has to be
>> presented by a sequence of 5-7 APDUs, which could be different for the
>> different cards. So, external module has to be able to compose the
>> plain APDUs .
> Partly wrong: Yes, SM has to be smart card aware (as you have pointed
> out with your RSA example). But the crypto module should not need to be
> smart card aware. The whole point of your sm modules (correct me if I am
> wrong) are cryptographic keys, which are located somewhere else. When it
> comes to your RSA example: Simply let OpenSC collect all RSA data and
> then send it to you crypto module.
What 'OpenSC' are you talking about? The one that is installed on the client
side?
The hole idea is that this part have never access to the plain RSA key and
cannot send data to the crypto module.
>>> Then, what you get is OpenSC that handles
>>> smart card stuff, including SM encoding and a crypto layer with loadable
>>> modules.
>> Sorry, do not understood.
> Do something like:
>
> struct sc_card_driver {
> ...
> struct sc_sm_driver *sm;
> ...
> }
> struct sc_sm_driver {
> ...
> struct sm_ctx *sm_ctx;
> int (*get_sm_apdu)(struct sc_card *card, struct sc_apdu *apdu, struct
> sc_apdu **sm_apdu);
> int (*free_sm_apdu)(struct sc_card *card, struct sc_apdu *apdu, struct
> sc_apdu **sm_apdu);
> ...
> }
> /** Secure messaging context */
> struct sm_ctx {
> /** 1 if secure messaging is activated, 0 otherwise */
> unsigned char active;
>
> /** data of the specific crypto implementation */
> void *priv_data;
>
> /** Padding-content indicator byte (ISO 7816-4 Table 30) */
> u8 padding_indicator;
> /** Pad to this block length */
> size_t block_length;
>
> /** Call back function for authentication of data */
> int (*authenticate)(sc_card_t *card, const struct sm_ctx *ctx,
> const u8 *data, size_t datalen, u8 **outdata);
> /** Call back function for verifying authentication data */
> int (*verify_authentication)(sc_card_t *card, const struct sm_ctx *ctx,
> const u8 *mac, size_t maclen,
> const u8 *macdata, size_t macdatalen);
>
> /** Call back function for encryption of data */
> int (*encrypt)(sc_card_t *card, const struct sm_ctx *ctx,
> const u8 *data, size_t datalen, u8 **enc);
> /** Call back function for decryption of data */
> int (*decrypt)(sc_card_t *card, const struct sm_ctx *ctx,
> const u8 *enc, size_t enclen, u8 **data);
>
> /** Call back function for actions before encoding and encryption of \a
> apdu */
> int (*pre_transmit)(sc_card_t *card, const struct sm_ctx *ctx,
> sc_apdu_t *apdu);
> /** Call back function for actions before decryption and decoding of \a
> sm_apdu */
> int (*post_transmit)(sc_card_t *card, const struct sm_ctx *ctx,
> sc_apdu_t *sm_apdu);
> /** Call back function for actions after decrypting SM protected APDU */
> int (*finish)(sc_card_t *card, const struct sm_ctx *ctx,
> sc_apdu_t *apdu);
>
> /** Clears and frees private data */
> void (*clear_free)(const struct sm_ctx *ctx);
> };
>
> (struct sm_ctx is actually copied from my approach to SM [5])
>
> Now, you have two points to customize SM: First you can specify get_apdu and
> free_apdu (which should really be renamed in something like sm_wrap_apdu and
> sm_unwrap_apdu). Second, you can change the call back functions that actually
> calculate a cryptogram or a mac using external keys. The second hook does not
> need to be aware to smart card stuff.
I do not see radical difference between what is currently committed to
'staging' and your proposals.
We can longly argue about importance of one or other feature (post-transmit,
pre-transmit, priv-data, ...) .
The only question that I would like to ask is:
I'm sure that the current SM framework can answer your needs that concerns the
low level SM wrapping of APDUs.
Can you do the same in a regards to the needs of external SM module, support of
the two SM specifications, more then one keysets per card/application ?
>>> [1]
>>> http://www.opensc-project.org/pipermail/opensc-devel/2010-October/015121.html
>>> [2]
>>> https://github.com/OpenSC/OpenSC/blob/staging/src/libopensc/card-epass2003.c
>>> [3] https://github.com/OpenSC/OpenSC/blob/staging/src/libopensc/sm.h
> [4] ftp://ftp.cenorm.be/Public/Cwas/e-europe/esign/cwa14890-01-2004-Mar.pdf
> [5] http://vsmartcard.sourceforge.net
>
>
>
> _______________________________________________
> opensc-devel mailing list
> opensc-devel@lists.opensc-project.org
> http://www.opensc-project.org/mailman/listinfo/opensc-devel
_______________________________________________
opensc-devel mailing list
opensc-devel@lists.opensc-project.org
http://www.opensc-project.org/mailman/listinfo/opensc-devel
