From a50f9a0eaed03dfe85ff3d7a4c24da20ac705dae Mon Sep 17 00:00:00 2001 Message-Id: From: Mark Wooding Date: Sat, 20 Dec 2008 11:39:33 +0000 Subject: [PATCH] server/peer.c, server/keyset.c: Fix key renegotiation behaviour. Organization: Straylight/Edgeware From: Mark Wooding 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. --- server/keyset.c | 47 +++++++++++++++++++----------- server/peer.c | 77 +++++++++++++++++++++++++++++++++++++------------ server/tripe.h | 24 +++++++++++---- 3 files changed, 106 insertions(+), 42 deletions(-) diff --git a/server/keyset.c b/server/keyset.c index fe491280..9dd17fac 100644 --- 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 * - * 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. @@ -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); ) - rc = -1; + rc = KSERR_REGEN; } 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 * - * 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 @@ -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); ) - return (-1); + return (KSERR_DECRYPT); } 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); }) - 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 * - * 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 @@ -415,7 +418,7 @@ int ks_encrypt(keyset *ks, unsigned ty, buf *b, buf *bb) if (!KEYOK(ks, now)) { buf_break(bb); - return (0); + return (KSERR_NOKEYS); } 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 * - * 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 @@ -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")) - return (-1); + return (KSERR_DECRYPT); return (0); } @@ -532,7 +537,10 @@ void ksl_prune(keyset **ksroot) * @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. */ @@ -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); - return (-1); + return (KSERR_NOKEYS); } 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 * - * 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. */ @@ -575,7 +585,7 @@ int ksl_decrypt(keyset **ksroot, unsigned ty, buf *b, buf *bb) uint32 seq; if (buf_ensure(bb, BLEN(b))) - return (-1); + return (KSERR_DECRYPT); 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; } - 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"); ) - return (-1); + return (KSERR_DECRYPT); } /*----- That's all, folks -------------------------------------------------*/ diff --git a/server/peer.c b/server/peer.c index b23ef7bf..7f4d0940 100644 --- a/server/peer.c +++ b/server/peer.c @@ -122,6 +122,57 @@ found: 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 @@ -208,11 +259,8 @@ static void p_read(int fd, unsigned mode, void *v) 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; - } 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)); - 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; - } 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)); - 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; - } 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); - 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); @@ -486,8 +526,7 @@ void p_tun(peer *p, buf *b) 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); diff --git a/server/tripe.h b/server/tripe.h index 448f9991..47c8cf0f 100644 --- 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 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 @@ -687,9 +691,10 @@ extern void ks_activate(keyset */*ks*/); * @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 @@ -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 * - * 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 @@ -760,7 +767,10 @@ extern void ksl_prune(keyset **/*ksroot*/); * @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. */ @@ -775,7 +785,9 @@ extern int ksl_encrypt(keyset **/*ksroot*/, unsigned /*ty*/, * @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. */ -- [mdw]