intel / cryptography-primitives

Apache License 2.0
319 stars 86 forks source link

In-place operations #13

Closed jaakristioja closed 4 years ago

jaakristioja commented 4 years ago

Hello!

The Intel IPP Developer guide states that source and destination memory areas passed to IPP functions must not overlap, except when functions with the _I descriptor are used. Given this, I note that several functions (e.g. ippsAES_GCMEncrypt and ippsAES_GCMDecrypt) lack safety checks for source and destination area memory overlaps, i.e. these functions don't return an error status if the memory areas overlap. Is this intentional or do ippcp functions actually support such "in-place" operations?

Please add support for in-place operations and add safety checks to detect memory overlaps for functions which don't support such in-place operations.

Thank you!

skirillo commented 4 years ago

Hello! Thank you for you interest to IPP. In the case of IPP Crypto, we consider the suffix _I as redundant, since IPP Crypto implementations are designed for "in-place" operations too. We'll check IPP Crypto implementations and add suitable check to detect whether memory overlap for functions which don't (or could not) support "in-place" mode. Do you mentioned of AES-GCM functions in connection with the fact that these function don't provide correct "in-place" encryption/decryption?

Thank you!

skirillo commented 4 years ago

Hello, Think you are right. Indeed, AES-GCM will lead to incorrect result if source and destination are overlapped under certain conditions. We'll check IPP's implementations.

Thank you for you important remark.

skirillo commented 4 years ago

Hello, I've checked AES-GCM behaviour. If operation is really "in-place" (i.e. pointers to input and output are equal) then result is correct - the same as in case of usual "not in-place" operation. Your case (overlapped buffers) pSrc <pDst <(pSrc+len) is different from "in-place" operation. I think, that the check (of overlap case above) does not make sense, because it could be customer's intention.

skirillo commented 4 years ago

no comments нуе. the issues is closing.

jaakristioja commented 4 years ago

@skirillo I'm sorry I did not understand you were expecting me to comment. Please reopen.

Note that "In-place" is just one case of "overlap".

Please correct me if I am mistaken, but what I gathered from your responses is that ippsAES_GCMEncrypt nor ippsAES_GCMDecrypt (and other IPP functions without the I descriptor) should also work (and do work) properly for strict "in-place" operatations (i.e. pSrc==pDst), but might not work properly other overlap cases (i.e. (pSrc != pDst) && (pSrc < pDst + len) && (pDst < pSrc + len)).

If this is the case, then I believe both of these functions should still return an error on overlaps which are not in-place operations. They currently seem to lack such checks.

If the I descriptor in function names is redundant, and all IPP functions work reliably for the strictly "in-place" (i.e. pSrc == pDst) cases, then I believe that the IPP developer guide should reflect that (it currently does not), stating that all functions support strict in-place but not general overaps, and there should be no I descriptors in function names (currently at least the ippsMAC_BN_I function seems to have it).

skirillo commented 4 years ago

AES-GCM works fine in in-pace mode. Do you agree? Overlapping src and dst is customer decision. And I believe that AES-GCM works exactly as customer desired. Do you agree? That is I can't consider that (overlapping) as incorrect parameters and return "bad status". And so I don't see any reason to add check on overlapped src and dst.

Let me note that "IPP developer guide" has no (at least formal) relation to IPP Crypto, and I believe we are talking about it.

Please, express your clime clearly:

jaakristioja commented 4 years ago

From what I gather from our previous discussion, we can categorize the cases we're talking about as follows:

AES-GCM works fine in in-pace mode. Do you agree?

I agree that the ippsAES_GCMEncrypt and ippsAES_GCMDecrypt functions work in the no overlap case and in the full overlap i.e. in-place case.

Overlapping src and dst is customer decision.

Strictly speaking, not necessarily - the customer might accidentally pass an incorrect pointer to the function. But this is out of the scope for this discussion.

And I believe that AES-GCM works exactly as customer desired. Do you agree?

If the customer uses partial overlap does it work as the customer expects it to?

Note that the customer might not have knowledge about the inner mechanisms of these algorithms since the documentation does not explicitly mention how it works in overlap cases. Therefore it seems reasonable for the customer to assume that it works properly in all overlap cases, including any partial overlap cases.

But do these functions actually work properly in the partial overlap cases or does it silently (no error returned) overwrite part(s) of the input with output before actually consuming (reading) the input it overwrote while processing the previous chunk of input?

That is I can't consider that (overlapping) as incorrect parameters and return "bad status". And so I don't see any reason to add check on overlapped src and dst.

If it works properly for all kinds of overlap cases then you are certainly correct. But before I filed this issue, examining the ipp-crypto code led me to believe that this only holds for strict in-place operations but not for all partial overlap cases (especially for the where pDst == pSrc + n where 0 < n < len).

Let me note that "IPP developer guide" has no (at least formal) relation to IPP Crypto, and I believe we are talking about it.

If there is no relation and the _i suffix is redundant, then why are there still functions in IPP crypto using that suffix?

Please, express your clime clearly:

I'm sorry, I'm not a native English speaker and I don't understand what you're asking. I kindly ask you, could you please rephrase?