Stable - Githubissues

intel / intel-ipsec-mb

Intel(R) Multi-Buffer Crypto for IPSec

BSD 3-Clause "New" or "Revised" License

289 stars 87 forks source link

Stable #4

Closed deadcafe closed 7 years ago

deadcafe commented 7 years ago

Hi Tomasz, thank you for merging my request. This patch is intended to make NULL_CIPHER easier to use. NULL＿CIPHER is very useful in product development. why not recommended to use NULL_CIPHER and NULL_HASH?

In addition, I have some suggestions. (1) separate the expanded_keys from gcm_data. (2) integrate GCM API into MB.

I like this library because it is very fast.

tkanteck commented 7 years ago

Hi Deadcafe, Many thanks for your patch. I'll have a look into it soon.

In current shape of the code NULL_CIPHER and NULL_HASH can be used as follows: NULL_CIPHER usage: Order = HASH_CIPHER, Cipher = NULL_CIPHER, Direction = any

NULL_HASH usage: For ENCRYPT: Order = CIPHER_HASH, hash = NULL_HASH, Direction = ENCRYPT For DECRYPT: Order = HASH_CIPHER, hash = NULL_HASH, Direction = DECRYPT

I should have included the above in the documentation. I'll fix it soon. There are other software components using NULL CIPHER/HASH interface in the form above and if this is not something urgent/critical I wouldn't like to change it. Please let me know what you think.

Thanks, Tomasz

tkanteck commented 7 years ago

For the time being I have merged the commit regarding gcm_defines.h and precomp functions. Please provide more context on your NULL CIPHER usage and please check if the model described above would be enough. Thanks, Tomasz

deadcafe commented 7 years ago

Sorry for my late reply. It is understood that only the combination of NULL_CIPHER and NULL_HASH is acceptable. ORDER and DIRECTION are always equals relationship, right? So it is better to check them regardless of the algorithm. the tag_len is also so. Since NULL_CIPHER does not copy from SRC to DST, you should make sure that it is the same address.

＃I deleted my branch by mistake, sorry.

tkanteck commented 7 years ago

Sorry for delay. Many thanks for the details. I'll have a second look into your patch wrt the check.

tkanteck commented 7 years ago

I was testing the is_job_invalid() patch in terms of functionality and performance - some comments to the original version:

it is easier to read & performs well on average - thanks
it fixes no copy problem for NULL cipher - thanks
it wasn't checking for msg_len_to_hash against 0 (easy fix)
there is probably no point to do msg_len_to_hash or auth_tag_output_len for NULL_HASH case (easy fix)
NULL_CIPHER is guaranteed to be safe when chain_order = HASH_CIPHER and direction = any, the patch obviously changes it and I am looking for an elegant way to use your patch but keep current interface for NULL_CIPHER. I'll keep you posted.