ipsec: make pre-shared keys harder to misuse

Using pre-shared keys is usually a bad idea, one should use eg. IKEv2
instead, but one does not always have the choice.

For AES-CBC, the IV must be unpredictable (see NIST SP800-38a Appendix
C) whereas for AES-CTR or AES-GCM, the IV should never be reused with
the same key material (see NIST SP800-38a Appendix B and NIST SP800-38d
section 8).
If one uses pre-shared keys and VPP is restarted, the IV counter
restarts at 0 and the same IVs are generated with the same pre-shared
keys materials.

To fix those issues we follow the recommendation from NIST SP800-38a
and NIST SP800-38d:
 - we use a PRNG (not cryptographically secured) to generate IVs to
avoid generating the same IV sequence between VPP restarts. The PRNG is
chosen so that there is a low chance of generating the same sequence
 - for AES-CBC, the generated IV is encrypted as part of the message.
This makes the (predictable) PRNG-generated IV unpredictable as it is
encrypted with the secret key
 - for AES-CTR and GCM, we use the IV as-is as predictable IVs are fine

Most of the changes in this patch are caused by the need to shoehorn an
additional state of 2 u64 for the PRNG in the 1st cacheline of the SA
object.

Type: improvement

Change-Id: I2af89c21ae4b2c4c33dd21aeffcfb79c13c9d84c
Signed-off-by: Benoît Ganne <bganne@cisco.com>

1 files changed, 9 insertions, 11 deletions

diff --git a/src/vnet/ipsec/esp_encrypt.c b/src/vnet/ipsec/esp_encrypt.c
index 88e93b9b2d4..861b3e98650 100644
--- a/src/vnet/ipsec/esp_encrypt.c
+++ b/src/vnet/ipsec/esp_encrypt.c
@@ -219,17 +219,18 @@ esp_get_ip6_hdr_len (ip6_header_t * ip6, ip6_ext_header_t ** ext_hdr)
  * encryption mode: IVs must be unpredictable for AES-CBC whereas it can
  * be predictable but should never be reused with the same key material
  * for CTR and GCM.
- * We use a packet counter as the IV for CTR and GCM, and to ensure the
- * IV is unpredictable for CBC, it is then encrypted using the same key
- * as the message. You can refer to NIST SP800-38a and NIST SP800-38d
- * for more details. */
+ * To avoid reusing the same IVs between multiple VPP instances and between
+ * restarts, we use a properly chosen PRNG to generate IVs. To ensure the IV is
+ * unpredictable for CBC, it is then encrypted using the same key as the
+ * message. You can refer to NIST SP800-38a and NIST SP800-38d for more
+ * details. */
 static_always_inline void *
 esp_generate_iv (ipsec_sa_t *sa, void *payload, int iv_sz)
 {
   ASSERT (iv_sz >= sizeof (u64));
   u64 *iv = (u64 *) (payload - iv_sz);
   clib_memset_u8 (iv, 0, iv_sz);
-  *iv = sa->iv_counter++;
+  *iv = clib_pcg64i_random_r (&sa->iv_prng);
   return iv;
 }
 
@@ -434,7 +435,7 @@ esp_prepare_sync_op (vlib_main_t *vm, ipsec_per_thread_data_t *ptd,
 	  crypto_len += iv_sz;
 	}
 
-      if (lb != b[0])
+      if (PREDICT_FALSE (lb != b[0]))
 	{
 	  /* is chained */
 	  op->flags |= VNET_CRYPTO_OP_FLAG_CHAINED_BUFFERS;
@@ -497,7 +498,7 @@ esp_prepare_async_frame (vlib_main_t *vm, ipsec_per_thread_data_t *ptd,
   esp_post_data_t *post = esp_post_data (b);
   u8 *tag, *iv, *aad = 0;
   u8 flag = 0;
-  u32 key_index;
+  const u32 key_index = sa->crypto_key_index;
   i16 crypto_start_offset, integ_start_offset;
   u16 crypto_total_len, integ_total_len;
 
@@ -508,8 +509,6 @@ esp_prepare_async_frame (vlib_main_t *vm, ipsec_per_thread_data_t *ptd,
   crypto_total_len = integ_total_len = payload_len - icv_sz;
   tag = payload + crypto_total_len;
 
-  key_index = sa->linked_key_index;
-
   /* generate the IV in front of the payload */
   void *pkt_iv = esp_generate_iv (sa, payload, iv_sz);
 
@@ -523,7 +522,6 @@ esp_prepare_async_frame (vlib_main_t *vm, ipsec_per_thread_data_t *ptd,
 	  /* constuct aad in a scratch space in front of the nonce */
 	  aad = (u8 *) nonce - sizeof (esp_aead_t);
 	  esp_aad_fill (aad, esp, sa, sa->seq_hi);
-	  key_index = sa->crypto_key_index;
 	}
       else
 	{
@@ -705,7 +703,7 @@ esp_encrypt_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
 	  is_async = im->async_mode | ipsec_sa_is_set_IS_ASYNC (sa0);
 	}
 
-      if (PREDICT_FALSE (~0 == sa0->thread_index))
+      if (PREDICT_FALSE ((u16) ~0 == sa0->thread_index))
 	{
 	  /* this is the first packet to use this SA, claim the SA
 	   * for this thread. this could happen simultaneously on