server/peer.c, server/keyset.c: Fix key renegotiation behaviour.

The existing behaviour is just wrong.  Whenever an encryption fails, the
key exchange gets kicked politely.  If the challenge is still valid,
this does nothing very useful, so fix it so that it forces a new
challenge.  If there are no keys left at all, this results in a
pointless flood of key-exchange packets.

This change introduces error codes for the ks_* and ksl_* functions, so
that callers can work out what's wrong.  (This isn't strictly necessary:
there was enough information before, but it wasn't a good idea.)  I took
the opportunity to improve the header comments on the ks_* and ksl_*
functions.

It also changes peer.c to distinguish between the various cases.  This
change provides new peer-level convenience functions for doing the
symmetric crypto, which reduces the amount of duplicated code lying
around.

diff --git a/server/keyset.c b/server/keyset.c
index fe491280cbcfd4f3792ce0dd9450cfa392d9f3c5..9dd17facc3af65fa364905f595a71f26cd6cb944 100644 (file)
--- a/server/keyset.c
+++ b/server/keyset.c
@@ -82,7 +82,9 @@
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *

- * Returns:    Zero if OK, nonzero if a new key is required.
+ * Returns:    Zero if OK; @KSERR_REGEN@ if it's time to generate new keys.
+ *             Also returns zero if there was insufficient buffer space, but
+ *             the buffer is broken in this case.

  *
  * Use:                Encrypts a message with the given key.  We assume that the
  *             keyset is OK to use.
  *
  * Use:                Encrypts a message with the given key.  We assume that the
  *             keyset is OK to use.


@@ -154,7 +156,7 @@ static int doencrypt(keyset *ks, unsigned ty, buf *b, buf *bb)
   if (osz >= SZ_REGEN && nsz < SZ_REGEN) {
     T( trace(T_KEYSET, "keyset: keyset %u data regen limit exceeded -- "
             "forcing exchange", ks->seq); )
   if (osz >= SZ_REGEN && nsz < SZ_REGEN) {
     T( trace(T_KEYSET, "keyset: keyset %u data regen limit exceeded -- "
             "forcing exchange", ks->seq); )

-    rc = -1;
+    rc = KSERR_REGEN;

   }
   ks->sz_exp = nsz;
   return (rc);
   }
   ks->sz_exp = nsz;
   return (rc);


@@ -168,7 +170,7 @@ static int doencrypt(keyset *ks, unsigned ty, buf *b, buf *bb)
  *             @buf *bb@ = pointer to an output buffer
  *             @uint32 *seq@ = where to store the sequence number
  *
  *             @buf *bb@ = pointer to an output buffer
  *             @uint32 *seq@ = where to store the sequence number
  *

- * Returns:    Zero if OK, nonzero if it failed.
+ * Returns:    Zero on success; @KSERR_DECRYPT@ on failure.

  *
  * Use:                Attempts to decrypt a message with the given key.  No other
  *             checking (e.g., sequence number checks) is performed.  We
  *
  * Use:                Attempts to decrypt a message with the given key.  No other
  *             checking (e.g., sequence number checks) is performed.  We


@@ -196,7 +198,7 @@ static int dodecrypt(keyset *ks, unsigned ty, buf *b, buf *bb, uint32 *seq)
 
   if (psz < ivsz + SEQSZ + tagsz) {
     T( trace(T_KEYSET, "keyset: block too small for keyset %u", ks->seq); )
 
   if (psz < ivsz + SEQSZ + tagsz) {
     T( trace(T_KEYSET, "keyset: block too small for keyset %u", ks->seq); )

-    return (-1);
+    return (KSERR_DECRYPT);

   }
   sz = psz - ivsz - SEQSZ - tagsz;
   pmac = BCUR(b); pseq = pmac + tagsz; piv = pseq + SEQSZ; ppk = piv + ivsz;
   }
   sz = psz - ivsz - SEQSZ - tagsz;
   pmac = BCUR(b); pseq = pmac + tagsz; piv = pseq + SEQSZ; ppk = piv + ivsz;


@@ -224,7 +226,7 @@ static int dodecrypt(keyset *ks, unsigned ty, buf *b, buf *bb, uint32 *seq)
        trace(T_KEYSET, "keyset: incorrect MAC: decryption failed");
        trace_block(T_CRYPTO, "crypto: expected MAC", pmac, tagsz);
       })
        trace(T_KEYSET, "keyset: incorrect MAC: decryption failed");
        trace_block(T_CRYPTO, "crypto: expected MAC", pmac, tagsz);
       })

-      return (-1);
+      return (KSERR_DECRYPT);

     }
   }
 
     }
   }
 


@@ -399,9 +401,10 @@ void ks_activate(keyset *ks)
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Zero if OK, nonzero if the key needs replacing.  If the
- *             encryption failed, the output buffer is broken and zero is
- *             returned.
+ * Returns:    Zero if successful; @KSERR_REGEN@ if we should negotiate a
+ *             new key; @KSERR_NOKEYS@ if the key is not usable.  Also
+ *             returns zero if there was insufficient buffer (but the output
+ *             buffer is broken in this case).

  *
  * Use:                Encrypts a block of data using the key.  Note that the `key
  *             ought to be replaced' notification is only ever given once
  *
  * Use:                Encrypts a block of data using the key.  Note that the `key
  *             ought to be replaced' notification is only ever given once


@@ -415,7 +418,7 @@ int ks_encrypt(keyset *ks, unsigned ty, buf *b, buf *bb)
 
   if (!KEYOK(ks, now)) {
     buf_break(bb);
 
   if (!KEYOK(ks, now)) {
     buf_break(bb);

-    return (0);
+    return (KSERR_NOKEYS);

   }
   return (doencrypt(ks, ty, b, bb));
 }
   }
   return (doencrypt(ks, ty, b, bb));
 }


@@ -427,7 +430,9 @@ int ks_encrypt(keyset *ks, unsigned ty, buf *b, buf *bb)
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *

- * Returns:    Zero on success, or nonzero if there was some problem.
+ * Returns:    Zero on success; @KSERR_DECRYPT@ on failure.  Also returns
+ *             zero if there was insufficient buffer (but the output buffer
+ *             is broken in this case).

  *
  * Use:                Attempts to decrypt a message using a given key.  Note that
  *             requesting decryption with a key directly won't clear a
  *
  * Use:                Attempts to decrypt a message using a given key.  Note that
  *             requesting decryption with a key directly won't clear a


@@ -443,7 +448,7 @@ int ks_decrypt(keyset *ks, unsigned ty, buf *b, buf *bb)
       buf_ensure(bb, BLEN(b)) ||
       dodecrypt(ks, ty, b, bb, &seq) ||
       seq_check(&ks->iseq, seq, "SYMM"))
       buf_ensure(bb, BLEN(b)) ||
       dodecrypt(ks, ty, b, bb, &seq) ||
       seq_check(&ks->iseq, seq, "SYMM"))

-    return (-1);
+    return (KSERR_DECRYPT);

   return (0);
 }
 
   return (0);
 }
 


@@ -532,7 +537,10 @@ void ksl_prune(keyset **ksroot)
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Nonzero if a new key is needed.
+ * Returns:    Zero if successful; @KSERR_REGEN@ if it's time to negotiate a
+ *             new key; @KSERR_NOKEYS@ if there are no suitable keys
+ *             available.  Also returns zero if there was insufficient
+ *             buffer space (but the output buffer is broken in this case).

  *
  * Use:                Encrypts a packet.
  */
  *
  * Use:                Encrypts a packet.
  */


@@ -546,7 +554,7 @@ int ksl_encrypt(keyset **ksroot, unsigned ty, buf *b, buf *bb)
     if (!ks) {
       T( trace(T_KEYSET, "keyset: no suitable keysets found"); )
       buf_break(bb);
     if (!ks) {
       T( trace(T_KEYSET, "keyset: no suitable keysets found"); )
       buf_break(bb);

-      return (-1);
+      return (KSERR_NOKEYS);

     }
     if (KEYOK(ks, now) && !(ks->f & KSF_LISTEN))
       break;
     }
     if (KEYOK(ks, now) && !(ks->f & KSF_LISTEN))
       break;


@@ -563,7 +571,9 @@ int ksl_encrypt(keyset **ksroot, unsigned ty, buf *b, buf *bb)
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Nonzero if the packet couldn't be decrypted.
+ * Returns:    Zero on success; @KSERR_DECRYPT@ on failure.  Also returns
+ *             zero if there was insufficient buffer (but the output buffer
+ *             is broken in this case).

  *
  * Use:                Decrypts a packet.
  */
  *
  * Use:                Decrypts a packet.
  */


@@ -575,7 +585,7 @@ int ksl_decrypt(keyset **ksroot, unsigned ty, buf *b, buf *bb)
   uint32 seq;
 
   if (buf_ensure(bb, BLEN(b)))
   uint32 seq;
 
   if (buf_ensure(bb, BLEN(b)))

-    return (-1);
+    return (KSERR_DECRYPT);

 
   for (ks = *ksroot; ks; ks = ks->next) {
     if (!KEYOK(ks, now))
 
   for (ks = *ksroot; ks; ks = ks->next) {
     if (!KEYOK(ks, now))


@@ -586,11 +596,14 @@ int ksl_decrypt(keyset **ksroot, unsigned ty, buf *b, buf *bb)
                 ks->seq); )
        ks->f &= ~KSF_LISTEN;
       }
                 ks->seq); )
        ks->f &= ~KSF_LISTEN;
       }

-      return (seq_check(&ks->iseq, seq, "SYMM"));
+      if (seq_check(&ks->iseq, seq, "SYMM"))
+       return (KSERR_DECRYPT);
+      else
+       return (0);

     }
   }
   T( trace(T_KEYSET, "keyset: no matching keys, or incorrect MAC"); )
     }
   }
   T( trace(T_KEYSET, "keyset: no matching keys, or incorrect MAC"); )

-  return (-1);
+  return (KSERR_DECRYPT);

 }
 
 /*----- That's all, folks -------------------------------------------------*/
 }
 
 /*----- That's all, folks -------------------------------------------------*/

diff --git a/server/peer.c b/server/peer.c
index b23ef7bfb676b6645dea0673d72fb423b1ff4fa4..7f4d09405a44db7713cef5aa80b9e615f9bdb595 100644 (file)
--- a/server/peer.c
+++ b/server/peer.c
@@ -122,6 +122,57 @@ found:
   p_pingdone(pg, PING_OK);
 }
 
   p_pingdone(pg, PING_OK);
 }
 

+/* --- @p_encrypt@ --- *
+ *
+ * Arguments:  @peer *p@ = peer to encrypt message to
+ *             @int ty@ message type to send
+ *             @buf *bin, *bout@ = input and output buffers
+ *
+ * Returns:    ---
+ *
+ * Use:                Convenience function for packet encryption.  Forces
+ *             renegotiation when necessary.  Check for the output buffer
+ *             being broken to find out whether the encryption was
+ *             successful.
+ */
+
+static int p_encrypt(peer *p, int ty, buf *bin, buf *bout)
+{
+  int err = ksl_encrypt(&p->ks, ty, bin, bout);
+
+  if (err == KSERR_REGEN) {
+    kx_start(&p->kx, 1);
+    err = 0;
+  }
+  if (!BOK(bout))
+    err = -1;
+  return (err);
+}
+
+/* --- @p_decrypt@ --- *
+ *
+ * Arguments:  @peer *p@ = peer to decrypt message from
+ *             @int ty@ = message type to expect
+ *             @buf *bin, *bout@ = input and output buffers
+ *
+ * Returns:    Zero on success; nonzero on error.
+ *
+ * Use:                Convenience function for packet decryption.  Reports errors
+ *             and updates statistics appropriately.
+ */
+
+static int p_decrypt(peer *p, int ty, buf *bin, buf *bout)
+{
+  if (ksl_decrypt(&p->ks, ty, bin, bout)) {
+    p->st.n_reject++;
+    a_warn("PEER", "?PEER", p, "decrypt-failed", A_END);
+    return (-1);
+  }
+  if (!BOK(bout))
+    return (-1);
+  return (0);
+}
+

 /* --- @p_read@ --- *
  *
  * Arguments:  @int fd@ = file descriptor to read from
 /* --- @p_read@ --- *
  *
  * Arguments:  @int fd@ = file descriptor to read from


@@ -208,11 +259,8 @@ static void p_read(int fd, unsigned mode, void *v)
        return;
       }
       buf_init(&bb, buf_o, sizeof(buf_o));
        return;
       }
       buf_init(&bb, buf_o, sizeof(buf_o));

-      if (ksl_decrypt(&p->ks, MSG_PACKET, &b, &bb)) {
-       p->st.n_reject++;
-       a_warn("PEER", "?PEER", p, "decrypt-failed", A_END);
+      if (p_decrypt(p, MSG_PACKET, &b, &bb))

        return;
        return;

-      }

       if (BOK(&bb)) {
        p->st.n_ipin++;
        p->st.sz_ipin += BSZ(&b);
       if (BOK(&bb)) {
        p->st.n_ipin++;
        p->st.sz_ipin += BSZ(&b);


@@ -239,26 +287,19 @@ static void p_read(int fd, unsigned mode, void *v)
          break;
        case MISC_EPING:
          buf_init(&bb, buf_t, sizeof(buf_t));
          break;
        case MISC_EPING:
          buf_init(&bb, buf_t, sizeof(buf_t));

-         if (ksl_decrypt(&p->ks, ch, &b, &bb)) {
-           p->st.n_reject++;
-           a_warn("PEER", "?PEER", p, "decrypt-failed", A_END);
+         if (p_decrypt(p, ch, &b, &bb))

            return;
            return;

-         }

          if (BOK(&bb)) {
            buf_flip(&bb);
          if (BOK(&bb)) {
            buf_flip(&bb);

-           if (ksl_encrypt(&p->ks, MSG_MISC | MISC_EPONG, &bb,
-                           p_txstart(p, MSG_MISC | MISC_EPONG)))
-             kx_start(&p->kx, 0);
+           p_encrypt(p, MSG_MISC | MISC_EPONG, &bb,
+                     p_txstart(p, MSG_MISC | MISC_EPONG));

            p_txend(p);
          }
          break;
        case MISC_EPONG:
          buf_init(&bb, buf_t, sizeof(buf_t));
            p_txend(p);
          }
          break;
        case MISC_EPONG:
          buf_init(&bb, buf_t, sizeof(buf_t));

-         if (ksl_decrypt(&p->ks, ch, &b, &bb)) {
-           p->st.n_reject++;
-           a_warn("PEER", "?PEER", p, "decrypt-failed", A_END);
+         if (p_decrypt(p, ch, &b, &bb))

            return;
            return;

-         }

          if (BOK(&bb)) {
            buf_flip(&bb);
            p_ponged(p, MISC_EPONG, &bb);
          if (BOK(&bb)) {
            buf_flip(&bb);
            p_ponged(p, MISC_EPONG, &bb);


@@ -427,8 +468,7 @@ int p_pingsend(peer *p, ping *pg, unsigned type,
       buf_init(&bb, buf_t, sizeof(buf_t));
       p_pingwrite(pg, &bb);
       buf_flip(&bb);
       buf_init(&bb, buf_t, sizeof(buf_t));
       p_pingwrite(pg, &bb);
       buf_flip(&bb);

-      if (ksl_encrypt(&p->ks, MSG_MISC | MISC_EPING, &bb, b))
-       kx_start(&p->kx, 0);
+      p_encrypt(p, MSG_MISC | MISC_EPING, &bb, b);

       if (!BOK(b))
        return (-1);
       p_txend(p);
       if (!BOK(b))
        return (-1);
       p_txend(p);


@@ -486,8 +526,7 @@ void p_tun(peer *p, buf *b)
   buf *bb = p_txstart(p, MSG_PACKET);
 
   TIMER;
   buf *bb = p_txstart(p, MSG_PACKET);
 
   TIMER;

-  if (ksl_encrypt(&p->ks, MSG_PACKET, b, bb))
-    kx_start(&p->kx, 0);
+  p_encrypt(p, MSG_PACKET, b, bb);

   if (BOK(bb) && BLEN(bb)) {
     p->st.n_ipout++;
     p->st.sz_ipout += BLEN(bb);
   if (BOK(bb) && BLEN(bb)) {
     p->st.n_ipout++;
     p->st.sz_ipout += BLEN(bb);

diff --git a/server/tripe.h b/server/tripe.h
index 448f9991ce6d5fee8fac35f137a2f6a9842960df..47c8cf0f4bf1ff13d5fe5f21c6d9a1b2e8bdb0f3 100644 (file)
--- a/server/tripe.h
+++ b/server/tripe.h
@@ -217,6 +217,10 @@ typedef struct keyset {
 #define KSF_LISTEN 1u                  /* Don't encrypt packets yet */
 #define KSF_LINK 2u                    /* Key is in a linked list */
 
 #define KSF_LISTEN 1u                  /* Don't encrypt packets yet */
 #define KSF_LINK 2u                    /* Key is in a linked list */
 

+#define KSERR_REGEN -1                 /* Regenerate keys */
+#define KSERR_NOKEYS -2                        /* No keys left */
+#define KSERR_DECRYPT -3               /* Unable to decrypt message */
+

 /* --- Key exchange --- *
  *
  * TrIPE uses the Wrestlers Protocol for its key exchange.  The Wrestlers
 /* --- Key exchange --- *
  *
  * TrIPE uses the Wrestlers Protocol for its key exchange.  The Wrestlers


@@ -687,9 +691,10 @@ extern void ks_activate(keyset */*ks*/);
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Zero if OK, nonzero if the key needs replacing.  If the
- *             encryption failed, the output buffer is broken and zero is
- *             returned.
+ * Returns:    Zero if successful; @KSERR_REGEN@ if we should negotiate a
+ *             new key; @KSERR_NOKEYS@ if the key is not usable.  Also
+ *             returns zero if there was insufficient buffer (but the output
+ *             buffer is broken in this case).

  *
  * Use:                Encrypts a block of data using the key.  Note that the `key
  *             ought to be replaced' notification is only ever given once
  *
  * Use:                Encrypts a block of data using the key.  Note that the `key
  *             ought to be replaced' notification is only ever given once


@@ -707,7 +712,9 @@ extern int ks_encrypt(keyset */*ks*/, unsigned /*ty*/,
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *
  *             @buf *b@ = pointer to an input buffer
  *             @buf *bb@ = pointer to an output buffer
  *

- * Returns:    Zero on success, or nonzero if there was some problem.
+ * Returns:    Zero on success; @KSERR_DECRYPT@ on failure.  Also returns
+ *             zero if there was insufficient buffer (but the output buffer
+ *             is broken in this case).

  *
  * Use:                Attempts to decrypt a message using a given key.  Note that
  *             requesting decryption with a key directly won't clear a
  *
  * Use:                Attempts to decrypt a message using a given key.  Note that
  *             requesting decryption with a key directly won't clear a


@@ -760,7 +767,10 @@ extern void ksl_prune(keyset **/*ksroot*/);
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Nonzero if a new key is needed.
+ * Returns:    Zero if successful; @KSERR_REGEN@ if it's time to negotiate a
+ *             new key; @KSERR_NOKEYS@ if there are no suitable keys
+ *             available.  Also returns zero if there was insufficient
+ *             buffer space (but the output buffer is broken in this case).

  *
  * Use:                Encrypts a packet.
  */
  *
  * Use:                Encrypts a packet.
  */


@@ -775,7 +785,9 @@ extern int ksl_encrypt(keyset **/*ksroot*/, unsigned /*ty*/,
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *
  *             @buf *b@ = pointer to input buffer
  *             @buf *bb@ = pointer to output buffer
  *

- * Returns:    Nonzero if the packet couldn't be decrypted.
+ * Returns:    Zero on success; @KSERR_DECRYPT@ on failure.  Also returns
+ *             zero if there was insufficient buffer (but the output buffer
+ *             is broken in this case).

  *
  * Use:                Decrypts a packet.
  */
  *
  * Use:                Decrypts a packet.
  */