Sym crypt hw #2473

popovec · 2021-12-23T15:02:10Z

symmetric encryption

New code:
pkcs#11: C_EncryptInit, C_Encrypt, C_EncryptUpdate, C_EncryptFinal
pkcs#15: pkcs15_skey_encrypt, sc_pkcs15_encrypt_sym
also implements: sc_encrypt_sym

MyEID driver: support for symmetric crypt

Tested: MyEID 4.0.1, 4.5.5 card, pkcs11-tool --encrypt, 128 bit AES key (small file encryption by C_encrypt, and big file by C_EncryptUpdate and C_EncrytpFinal), all supported mechanisms: AES-ECB, AEC-CBC, AES-CBC-PAD.

[x ] PKCS#11 module is tested

src/libopensc/card-myeid.c

Jakuje · 2022-02-25T13:11:04Z

src/pkcs11/mechanism.c

+sc_pkcs11_encrypt_init(sc_pkcs11_operation_t *operation,
+			struct sc_pkcs11_object *key)
+{
+	struct signature_data *data;


The usage of signature_data is confusing here. If I see right, only the key part is used. Would it make sense to assign the key directly into the operation->priv_data? Or create generic operation_data if we envision there will be some more data needed to share between the functions?

We already have same confusing name in sc_pkcs11_decrypt_init(). It doesn't make sense to fix this in this PR just because of the new operation sc_pkcs11_encrypt_init (), we have to fix it globally (so also for the function sc_pkcs11_decrypt_init ())

static CK_RV sc_pkcs11_decrypt_init(sc_pkcs11_operation_t *operation, struct sc_pkcs11_object *key) { struct signature_data *data; CK_RV rv; if (!(data = new_signature_data())) return CKR_HOST_MEMORY;

Would it make sense to assign the key directly into the operation->priv_data?

This solution complicates the freeing of memory (in sc_pkcs11_new_fw_mechanism () we use mt-> release = sc_pkcs11_signature_release;)

It might be a good idea to rename struct signature_data to operation_data and similarly rename functions that operate with it: new_signature_data(), signature_data_release(), signature_data_buffer_append() and of course sc_pkcs11_signature_release().

The question is whether this renaming will not impair the readability of the code in sign and verify operations. I believe that this renaming should be done outside this PR.

could you create a PR with the suggested fix to the naming?

Jakuje · 2022-02-25T18:31:15Z

Then it looks ok to me, even though I did not go into detail of the operations.

One more thing, can you rebase on top of current master to get also CI results with OpenSSL 3.0?

In any case, I would like also somebody else to go through this before merging.

popovec · 2022-02-25T20:00:01Z

Rebased, CI tests with openssl 3.0 and libressl are OK.

This PR is designed so as not to interfere with current asymmetric operations in OpenSC. Code for symmetric decryption may affect asymmetric decipher operation and therefore decipher code is planned to be submitted as separate PR . Preliminary code for symmetric decipher is available here: https://github.com/popovec/OpenSC/tree/sym_crypt_hw_test

Some details about implementation:

Symmetric encrypt allow us to encrypt large amount of data, there is no way to accumulate all data in buffer and then encrypt them. The sc * functions must allow us to initialize encryption and then allow us to repetitive call (to update data) from C_EncryptUpdate and get last part of result - C_EncryptFinal.

However, only one version of sc_encrypt_sym () is enough, because each of the calls C_EncryptInit, C_EncryptUpdate and C_EncryptFinal sets the parameters in sc_encrypt_sym () differently (similarly in sc_pkcs15_encrypt_sym ()).

C_EncryptInit is mapped to sc_pkcs15_encrypt_sym (...., NULL, 0, NULL,NULL)
C_EncryptUpdate is mapped to sc_pkcs15_encrypt_sym (...., data, len, encdata, &enclen)
C_EncryptFinal is mapped to sc_pkcs15_encrypt_sym (...., NULL, 0, encdata, &enclen)
C_Encrypt is constructed from C_EncryptUpdate and C_EncryptFinal.

If the above conversion is unsatisfactory for some reason, I would be happy if someone suggests another way to map the pkcs11 function to the pkcs15 interface.

frankmorgner · 2022-04-19T22:33:57Z

src/pkcs11/mechanism.c

+sc_pkcs11_encrypt_init(sc_pkcs11_operation_t *operation,
+			struct sc_pkcs11_object *key)
+{
+	struct signature_data *data;


could you create a PR with the suggested fix to the naming?

Jakuje

thank you! Look good

Jakuje

I just went through the code and noticed only couple of formatting issues, which I would like to tackle before merging. You can use as a help the clang-format from #2017, but do not take every suggestion as a rule as we did not finalize the formatting rules yet.

src/pkcs11/pkcs11-object.c

src/pkcs11/mechanism.c

src/libopensc/card-myeid.c

initial implementation: pkcs#11: C_EncryptInit, C_Encrypt, C_EncryptUpdate, C_EncryptFinal pkcs#15: pkcs15_skey_encrypt, sc_pkcs15_encrypt_sym also implements: sc_encrypt_sym This code is inspired by: https://github.com/carblue/OpenSC-1/tree/sym_hw_encrypt

AES-ECB, AEC-CBC, AES-CBC-PAD Tested: MyEID 4.0.1, 4.5.5 card, pkcs11-tool --encrypt, 128 bit AES key (small file encryption by C_encrypt, and big file by C_EncryptUpdate and C_EncrytpFinal), all supported mechanisms: AES-ECB, AEC-CBC, AES-CBC-PAD.

modified: .github/test-oseid.sh

Old name (signature_data) was confusing, because this structure is already used in encrypt and decrypt operation. Related functions are also renamed.

popovec · 2022-08-11T11:08:24Z

Rebased, formating fixed to match definitions in .clang-format from #2017 except reformatting of large structures in src/pkcs11/openssl.c, src/pkcs11/framework-pkcs15.c,src/libopensc/iso7816.c and src/pkcs11/pkcs11-object.c. (Is it appropriate to add another patch that reformats these structures as well?)

this condition is repeated at least twice throughout this function. Would it make sense to make some helper condition or macro such as is_wrap_or_encrypt(env->operation) to make the code more readable?

This is fixed by macro IS_SYMETRIC_CRYPT(x) (I don't want to mix wrap/unwrap operations and symmetric encryption/decryption operations in one function/macro).

Jakuje · 2022-08-11T11:16:53Z

@frankmorgner do you have some more comments regarding this or can I merge this for the 0.23.0 release?

popovec · 2022-08-26T11:49:26Z

This patch introduced two defects reported by "coverity scan". I'm not sure if these are false negative reports or if they are real bugs. I will look into both reported issues.

*** CID 380538:  Memory - illegal accesses  (OVERRUN)
/src/libopensc/card-myeid.c: 1971 in myeid_enc_dec_sym()
1965     
1966                                    sc_log(ctx, "Found padding byte %02x", pad_byte);
1967                                    if (pad_byte == 0 || pad_byte > block_size)
1968                                            LOG_FUNC_RETURN(ctx, SC_ERROR_WRONG_PADDING);
1969                                    sdata = priv->sym_plain_buffer + block_size - pad_byte;
1970                                    for (i = 0; i < pad_byte; i++)
>>>     CID 380538:  Memory - illegal accesses  (OVERRUN)
>>>     Overrunning array of 16 bytes at byte offset 30 by dereferencing pointer "sdata + i".
1971                                            if (sdata[i] != pad_byte)
1972                                                    LOG_FUNC_RETURN(ctx, SC_ERROR_WRONG_PADDING);
1973                                    return_len = block_size - pad_byte;
1974                            }
1975                            *outlen = return_len;
1976                            if (return_len > *outlen)

/src/pkcs11/framework-pkcs15.c: 5586 in pkcs15_skey_encrypt()
5580            if (rv < 0)
5581                    return sc_to_cryptoki_error(rv, "C_Encrypt...");
5582     
5583            /* pointer CK_ULONG_PTR to size_t conversion */
5584            lpEncryptedDataLen = pulEncryptedDataLen ? &lEncryptedDataLen : NULL;
5585     
>>>     CID 380537:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "skey".
5586            rv = sc_pkcs15_encrypt_sym(fw_data->p15_card, skey->prv_p15obj, flags,
5587                            pData, ulDataLen, pEncryptedData, lpEncryptedDataLen,
5588                            pMechanism->pParameter, pMechanism->ulParameterLen);
5589     
5590            if (pulEncryptedDataLen)
5591                    *pulEncryptedDataLen = *lpEncryptedDataLen;

Jakuje reviewed Jan 16, 2022

View reviewed changes

src/libopensc/card-myeid.c Outdated Show resolved Hide resolved

src/libopensc/card-myeid.c Outdated Show resolved Hide resolved

popovec force-pushed the sym_crypt_hw branch from 57cf4d1 to 265e4ab Compare January 17, 2022 11:43

Jakuje reviewed Feb 25, 2022

View reviewed changes

popovec force-pushed the sym_crypt_hw branch from 265e4ab to 3827515 Compare February 25, 2022 18:53

Jakuje mentioned this pull request Apr 1, 2022

Prepare for release 0.23.0 #2426

Closed

frankmorgner approved these changes Apr 19, 2022

View reviewed changes

Jakuje approved these changes May 2, 2022

View reviewed changes

Jakuje reviewed Aug 10, 2022

View reviewed changes

popovec added 4 commits August 11, 2022 10:01

Github actions: AES encryption test (OsEID/MyEID)

52ca4fc

modified: .github/test-oseid.sh

Rename "signature_data" to "operation_data"

e480afb

Old name (signature_data) was confusing, because this structure is already used in encrypt and decrypt operation. Related functions are also renamed.

popovec force-pushed the sym_crypt_hw branch from 1753bf3 to e480afb Compare August 11, 2022 10:19

Jakuje merged commit 549432d into OpenSC:master Aug 26, 2022

Sym crypt hw #2473

Sym crypt hw #2473

Uh oh!

Conversation

popovec commented Dec 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jakuje Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

popovec Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

frankmorgner Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Jakuje commented Feb 25, 2022

Uh oh!

popovec commented Feb 25, 2022

Uh oh!

frankmorgner Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

popovec commented Aug 11, 2022

Uh oh!

Jakuje commented Aug 11, 2022

Uh oh!

popovec commented Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants