Use struct sm_cwa_session from sm.h in dnie

[rickyepoderi]

This is just a question not an issue. Making dnie dependent of the sm I realized that the structures that the dnie uses for establishing the secure channel are (more or less) already defined in sm.h file.

In cwa14890.h the dnie uses the following structures to handle the sm:

typedef struct cwa_sm_session_st {
        int state;      /** one of NONE, INPROGRESS, or ACTIVE */
        u8 kenc[16];    /** key used for data encoding */
        u8 kmac[16];    /** key for mac checksum calculation */
        u8 ssc[8];      /** send sequence counter */
} cwa_sm_session_t;

typedef struct cwa_sm_status_st {
        u8 kicc[32];
        u8 kifd[32];
        u8 rndicc[8];   /** 8 bytes random number generated by card */
        u8 rndifd[8];   /** 8 bytes random number generated by application */
        u8 sig[128];    /** buffer to store & compute signatures (1024 bits) */
        cwa_sm_session_t session; /** current session data */
} cwa_sm_status_t;

In sm.h there is another structure to establish the sm:

struct sm_cwa_token_data  {
        unsigned char sn[8];
        unsigned char rnd[8];
        unsigned char k[32];
};

struct sm_cwa_session {
        ...
        struct sm_cwa_token_data icc;
        struct sm_cwa_token_data ifd;

        unsigned char session_enc[16];
        unsigned char session_mac[16];

        unsigned char ssc[8];
        ...
};

If you see, the sm structures defined in sm.h are the same than in cwa14890.h except for the sig buffer. This field is just te signature buffer used when creating the secure channel and it is transient (only used when the channel is created). I think we can get rid of this duplicate structures and I have implemented a quick PoC which is working with dnie, the commit is this one:

01ec599

And anyone with a dnie can test it with this branch:

https://github.com/rickyepoderi/OpenSC/tree/new-sm

Besides I decided to finally delete the sc_reset at channel creation, in dnie 3.0 there are a lot of secure channels being created and I don't want to reset the card all the time. No problems so far with that branch in my testing.

That's all. Tell me if you think this is useful or I have to just forget it.

Thanks!

[rickyepoderi]

In general DNIe 3.0 bug #810 (comment) @davernos commented that the sc_reset gave problems to him.

In my testing I have experienced the same issue but it is very, very rare (maybe a fail in one hundred, and usually only in the first tests). I don't know if it depends on the reader or what. Nevertheless I think it is important to remove that sc_reset, the idea has been around for a long time but I was reluctant because I supposed there was a reason for it to be there. But now I am more comfortable with the code and I don't know any reason to perform that reset (official implementation does not use it either).

So, my only question is if I continue with this idea of using the common sm structures and types inside the DNIe or I just remove the sc_reset and keep custom DNIe structs... Do you think using common sm structures is useful or just a crazy idea?

[frankmorgner]

as said before, the dnie didn't use much of the SM framework created by @viktorTarasov. Instead everything was implemented at the driver level (again). So yes, unifying is a good way to go.

A step further down this road, the dnie driver should always use sc_transmit_apdu() instead of dnie_transmit_apdu() which manually checks the SM state and calls cwa_decode_response. Instead, handling SM should be done transparently within sc_sm_single_transmit while the driver's card->sm_ctx.ops.get_sm_apdu should be used to encrypt ("encode") the APDU and card->sm_ctx.ops.free_sm_apdu() should be used to decrypt it ("decode"). See discussion in #655.

[rickyepoderi]

Ok, perfect! For the moment I'll continue deleting the repeated structs in DNIe side.

My suspicion is that, in order to make DNIe work with sc_sm_single_transmit, we'll need changes outside the DNIe driver.