[PATCH API-NEXT v1] api: crypto: deprecate per-session IV#32
[PATCH API-NEXT v1] api: crypto: deprecate per-session IV#32lumag wants to merge 1 commit intoOpenDataPlane:api-nextfrom
Conversation
Per-session IV (in it's current form) are static IVs used (possibly) for several packets, and so they degradate security of genrated packets. Instead of modifying semantics of IV wrt processed packets, etc. let's deprecate (and in future remove) this IV altogether and enforce applications to always specify correct IV in the per-packet operation params. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
muvarov
changed the title
Bill-Fischofer-Linaro
left a comment
Bill-Fischofer-Linaro
left a comment
There was a problem hiding this comment.
This looks like an important bug correction. Main suggestion is that a security bug be opened and this patch marked as a correction for it.
| /* Generate an IV */ | ||
| if (entry->esp.iv_len) { | ||
| int32_t size = entry->esp.iv_len; | ||
| int32_t ret = odp_random_data(esp->iv, size, 1); |
There was a problem hiding this comment.
Should use enum here: ODP_RANDOM_CRYPTO
| /** Cipher Initialization Vector (IV) */ | ||
| odp_crypto_iv_t iv; | ||
| /** @deprecated use iv_length and per-packet IV instead */ | ||
| ODP_DEPRECATE(odp_crypto_iv_t) ODP_DEPRECATE(iv); |
There was a problem hiding this comment.
Not clear why checkpatch gets confused here. It seems to think this is a cast of some sort. Another reason perhaps to simply correct this issue without deprecation.
| uint32_t length; | ||
|
|
||
| } odp_crypto_iv_t; | ||
| } ODP_DEPRECATE(odp_crypto_iv_t); |
There was a problem hiding this comment.
Since this is really a bug correction, perhaps this should just be removed without deprecation? Why would anyone want to retain the incorrect behavior "for compatibility"?
There was a problem hiding this comment.
@Bill-Fischofer-Linaro I would like to hear a word from other parties. This bug might be local to linux-generic. And there might be use cases for the IV specified at session creation time (e.g. if packets should be encrypted in one continuous stream).
There was a problem hiding this comment.
During today's ARCH call this was discussed briefly and Nikhil pointed out that per-session IVs are useful for many cases, so deprecation may be premature, especially as we already permit per-packet IV overrides. We can continue this discussion on Monday's ARCH call if the mailing list can't reach resolution before then.
| #if ODP_DEPRECATED_API | ||
| /* Copy of session IV data */ | ||
| uint8_t iv_data[MAX_IV_LEN]; | ||
| #endif |
There was a problem hiding this comment.
Needing an #ifdef looks ugly here and elsewhere in this series. Better to correct the code than retain "compatibility" here.
| if (!((16 == session->p.iv_length) || (16 == session->p.iv.length))) | ||
| return -1; | ||
| #else | ||
| if (16 != session->p.iv_length) |
There was a problem hiding this comment.
The comment above got deleted. Magic numbers should either be enums or at least retain a comment explaining their use/significance.
3 participants
Per-session IV (in it's current form) are static IVs used (possibly) for
several packets, and so they degradate security of genrated packets.
Instead of modifying semantics of IV wrt processed packets, etc. let's
deprecate (and in future remove) this IV altogether and enforce
applications to always specify correct IV in the per-packet operation
params.
Signed-off-by: Dmitry Eremin-Solenikov dmitry.ereminsolenikov@linaro.org