From 69d65f583c18782b3b2fd302cbd310e6b9e7d7b5 Mon Sep 17 00:00:00 2001 From: Matt Dunwoodie Date: Fri, 23 Apr 2021 11:31:35 +1000 Subject: 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 --- TODO.md | 3 --- src/wg_cookie.c | 34 +++++++++++++++++++--------------- src/wg_cookie.h | 5 +++-- 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); -- cgit v1.2.3-59-g8ed1b