wg_cookie: add cookie_valid bool

Primarily this commit adds a cookie_valid state, to prevent a recently
booted machine from sending a mac2. We also do a little bit of reworking
on locking and a fixup for int to bool.

There is one slight difference to cookie_valid (latest_cookie.is_valid)
on Linux and that is to set cookie_valid to false when the
cookie_birthdate has expired. The purpose of this is to prevent the
expensive timer check after it has expired.

For the locking, we want to hold a write lock in cookie_maker_mac
because we write to mac1_last, mac1_valid and cookie_valid. This
wouldn't cause too much contention as this is a per peer lock and we
only do so when sending handshake packets. This is different from Linux
as Linux writes all it's variables at the start, then downgrades to a
read lock.

We also match cookie_maker_consume_payload locking to Linux, that is to
read lock while checking mac1_valid and decrypting the cookie then take
a write lock to set the cookie.

Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>

3 files changed, 22 insertions, 20 deletions

diff --git a/TODO.md b/TODO.md
index 5f68380..3d13a8f 100644
--- a/TODO.md
+++ b/TODO.md
@@ -9,9 +9,6 @@
   permissions in another.)
 - Make code style consistent with one FreeBSD way, rather than a mix of styles.
 - Make sure noise state machine is correct.
-- The cookie logic appears to be broken in unusual ways, in particular right
-  after boot up. Audit and compare all `is_valid` checks, as well as
-  `have_sent_mac1` guards.
 - Investigate whether the allowed ips lookup structure needs reference
   counting.
 - Handle failures of `rn_inithead` and remember to call `rn_detachhead`
diff --git a/src/wg_cookie.c b/src/wg_cookie.c
index c9d3676..e131467 100644
--- a/src/wg_cookie.c
+++ b/src/wg_cookie.c
@@ -162,11 +162,10 @@ int
 cookie_maker_consume_payload(struct cookie_maker *cp,
     uint8_t nonce[COOKIE_NONCE_SIZE], uint8_t ecookie[COOKIE_ENCRYPTED_SIZE])
 {
-	int ret = 0;
 	uint8_t cookie[COOKIE_COOKIE_SIZE];
+	int ret;
 
-	rw_wlock(&cp->cp_lock);
-
+	rw_rlock(&cp->cp_lock);
 	if (!cp->cp_mac1_valid) {
 		ret = ETIMEDOUT;
 		goto error;
@@ -177,13 +176,18 @@ cookie_maker_consume_payload(struct cookie_maker *cp,
 		ret = EINVAL;
 		goto error;
 	}
+	rw_runlock(&cp->cp_lock);
 
+	rw_wlock(&cp->cp_lock);
 	memcpy(cp->cp_cookie, cookie, COOKIE_COOKIE_SIZE);
-	cp->cp_birthdate = getsbinuptime();
+	cp->cp_cookie_birthdate = getsbinuptime();
+	cp->cp_cookie_valid = true;
 	cp->cp_mac1_valid = false;
+	rw_wunlock(&cp->cp_lock);
 
+	return 0;
 error:
-	rw_wunlock(&cp->cp_lock);
+	rw_runlock(&cp->cp_lock);
 	return ret;
 }
 
@@ -191,25 +195,25 @@ void
 cookie_maker_mac(struct cookie_maker *cp, struct cookie_macs *cm, void *buf,
 		size_t len)
 {
-	rw_rlock(&cp->cp_lock);
-
+	rw_wlock(&cp->cp_lock);
 	cookie_macs_mac1(cm, buf, len, cp->cp_mac1_key);
-
 	memcpy(cp->cp_mac1_last, cm->mac1, COOKIE_MAC_SIZE);
 	cp->cp_mac1_valid = true;
 
-	if (!cookie_timer_expired(cp->cp_birthdate,
-	    COOKIE_SECRET_MAX_AGE - COOKIE_SECRET_LATENCY, 0))
+	if (cp->cp_cookie_valid &&
+	    !cookie_timer_expired(cp->cp_cookie_birthdate,
+	    COOKIE_SECRET_MAX_AGE - COOKIE_SECRET_LATENCY, 0)) {
 		cookie_macs_mac2(cm, buf, len, cp->cp_cookie);
-	else
+	} else {
 		bzero(cm->mac2, COOKIE_MAC_SIZE);
-
-	rw_runlock(&cp->cp_lock);
+		cp->cp_cookie_valid = false;
+	}
+	rw_wunlock(&cp->cp_lock);
 }
 
 int
 cookie_checker_validate_macs(struct cookie_checker *cc, struct cookie_macs *cm,
-		void *buf, size_t len, int busy, struct sockaddr *sa)
+		void *buf, size_t len, bool check_cookie, struct sockaddr *sa)
 {
 	struct cookie_macs our_cm;
 	uint8_t cookie[COOKIE_COOKIE_SIZE];
@@ -223,7 +227,7 @@ cookie_checker_validate_macs(struct cookie_checker *cc, struct cookie_macs *cm,
 	if (timingsafe_bcmp(our_cm.mac1, cm->mac1, COOKIE_MAC_SIZE) != 0)
 		return EINVAL;
 
-	if (busy != 0) {
+	if (check_cookie) {
 		cookie_checker_make_cookie(cc, cookie, sa);
 		cookie_macs_mac2(&our_cm, buf, len, cookie);
 
diff --git a/src/wg_cookie.h b/src/wg_cookie.h
index 3ffa7aa..3dc1977 100644
--- a/src/wg_cookie.h
+++ b/src/wg_cookie.h
@@ -33,8 +33,9 @@ struct cookie_maker {
 	uint8_t		cp_cookie_key[COOKIE_KEY_SIZE];
 
 	struct rwlock	cp_lock;
+	bool		cp_cookie_valid;
 	uint8_t		cp_cookie[COOKIE_COOKIE_SIZE];
-	sbintime_t	cp_birthdate;	/* sbinuptime */
+	sbintime_t	cp_cookie_birthdate;	/* sbinuptime */
 	bool		cp_mac1_valid;
 	uint8_t		cp_mac1_last[COOKIE_MAC_SIZE];
 };
@@ -63,7 +64,7 @@ int	cookie_maker_consume_payload(struct cookie_maker *,
 void	cookie_maker_mac(struct cookie_maker *, struct cookie_macs *,
 	    void *, size_t);
 int	cookie_checker_validate_macs(struct cookie_checker *,
-	    struct cookie_macs *, void *, size_t, int, struct sockaddr *);
+	    struct cookie_macs *, void *, size_t, bool, struct sockaddr *);
 
 #ifdef SELFTESTS
 void	cookie_selftest(void);