radius: authenticate replies against the request on the wire

Verify replies before callbacks and server-health updates. Keep the secret
with the request and centralize accounting signing after server selection,
including retransmits and Accounting-On/Off. Reset the outbound
Message-Authenticator field before recalculating it on retries.

Adapted from Ritika Chopra's accel-ppp-ng PR #40, T8526/T8545, with
upstream's OpenSSL API and secret-reload ownership. Existing accounting
callback lifetime fixes are retained.

Co-authored-by: Ritika Chopra <r.chopra@vyos.io>

5 files changed, 80 insertions, 79 deletions

diff --git a/accel-pppd/radius/acct.c b/accel-pppd/radius/acct.c
index d30aae7a..e9b2147f 100644
--- a/accel-pppd/radius/acct.c
+++ b/accel-pppd/radius/acct.c
@@ -6,7 +6,6 @@
 #include <sys/ioctl.h>
 #include <netinet/in.h>
 
-#include <openssl/md5.h>
 
 #include "linux_ppp.h"
 
@@ -25,30 +24,6 @@
 
 #define INTERIM_SAFE_TIME 10
 
-static int req_set_RA(struct rad_req_t *req)
-{
-	char *secret;
-	MD5_CTX ctx;
-
-	secret = rad_server_secret_dup(req->serv);
-	if (!secret)
-		return -1;
-
-	if (rad_packet_build(req->pack, req->RA)) {
-		_free(secret);
-		return -1;
-	}
-
-	MD5_Init(&ctx);
-	MD5_Update(&ctx, req->pack->buf, req->pack->len);
-	MD5_Update(&ctx, secret, strlen(secret));
-	MD5_Final(req->pack->buf + 4, &ctx);
-
-	_free(secret);
-
-	return 0;
-}
-
 static int req_set_stat(struct rad_req_t *req, struct ap_session *ses)
 {
 	struct timespec ts;
@@ -188,9 +163,6 @@ static void rad_acct_interim_update(struct triton_timer_t *t)
 	rpd->acct_req->ts = ts.tv_sec;
 	rpd->acct_req->pack->id++;
 
-	if (!rpd->acct_req->before_send)
-		req_set_RA(rpd->acct_req);
-
 	rpd->acct_req->timeout.expire_tv.tv_sec = conf_timeout;
 	rpd->acct_req->try = 0;
 
@@ -221,8 +193,6 @@ static int rad_acct_before_send(struct rad_req_t *req)
 	clock_gettime(CLOCK_MONOTONIC, &ts);
 
 	rad_packet_change_int(req->pack, NULL, "Acct-Delay-Time", ts.tv_sec - req->ts + conf_acct_delay_start);
-	req_set_RA(req);
-
 	return 0;
 }
 
@@ -317,8 +287,6 @@ static int __rad_acct_start(struct radius_pd_t *rpd)
 
 	if (conf_acct_delay_time)
 		req->before_send = rad_acct_before_send;
-	else if (req_set_RA(req))
-		goto out_err;
 
 	req->recv = rad_acct_start_recv;
 	req->timeout.expire = rad_acct_start_timeout;
@@ -550,7 +518,6 @@ int rad_acct_stop(struct radius_pd_t *rpd)
 
 	rad_packet_change_val(req->pack, NULL, "Acct-Status-Type", "Stop");
 	req_set_stat(req, rpd->ses);
-	req_set_RA(req);
 
 	req->recv = rad_acct_stop_recv;
 	req->timeout.expire = rad_acct_stop_timeout;
diff --git a/accel-pppd/radius/auth.c b/accel-pppd/radius/auth.c
index d2cb9803..63b1345e 100644
--- a/accel-pppd/radius/auth.c
+++ b/accel-pppd/radius/auth.c
@@ -22,7 +22,7 @@ static int decrypt_chap_mppe_keys(struct rad_req_t *req, struct rad_attr_t *attr
 	uint8_t md5[MD5_DIGEST_LENGTH];
 	uint8_t sha1[SHA_DIGEST_LENGTH];
 	uint8_t plain[32];
-	char *secret;
+	const char *secret;
 	int i;
 
 	if (attr->len != 32) {
@@ -30,7 +30,7 @@ static int decrypt_chap_mppe_keys(struct rad_req_t *req, struct rad_attr_t *attr
 		return -1;
 	}
 
-	secret = rad_server_secret_dup(req->serv);
+	secret = (const char *)req->pack->secret;
 	if (!secret)
 		return -1;
 
@@ -59,7 +59,6 @@ static int decrypt_chap_mppe_keys(struct rad_req_t *req, struct rad_attr_t *attr
 	SHA1_Final(sha1, &sha1_ctx);
 
 	memcpy(key, sha1, 16);
-	_free(secret);
 
 	return 0;
 }
@@ -69,7 +68,7 @@ static int decrypt_mppe_key(struct rad_req_t *req, struct rad_attr_t *attr, uint
 	MD5_CTX md5_ctx;
 	uint8_t md5[16];
 	uint8_t plain[32];
-	char *secret;
+	const char *secret;
 	int i;
 
 	if (attr->len != 34) {
@@ -82,7 +81,7 @@ static int decrypt_mppe_key(struct rad_req_t *req, struct rad_attr_t *attr, uint
 		return -1;
 	}
 
-	secret = rad_server_secret_dup(req->serv);
+	secret = (const char *)req->pack->secret;
 	if (!secret)
 		return -1;
 
@@ -99,7 +98,6 @@ static int decrypt_mppe_key(struct rad_req_t *req, struct rad_attr_t *attr, uint
 
 	if (plain[0] != 16) {
 		log_ppp_warn("radius: %s: incorrect key length (%i)\n", attr->attr->name, plain[0]);
-		_free(secret);
 		return -1;
 	}
 
@@ -111,7 +109,6 @@ static int decrypt_mppe_key(struct rad_req_t *req, struct rad_attr_t *attr, uint
 	plain[16] ^= md5[0];
 
 	memcpy(key, plain + 1, 16);
-	_free(secret);
 
 	return 0;
 }
@@ -288,17 +285,16 @@ int rad_auth_pap(struct radius_pd_t *rpd, const char *username, va_list args)
 	const char *passwd = va_arg(args, const char *);
 	uint8_t *epasswd;
 	int epasswd_len;
-	char *secret;
+	const char *secret;
 
 	if (!req)
 		return PWDB_DENIED;
 
-	secret = rad_server_secret_dup(req->serv);
+	secret = (const char *)req->pack->secret;
 	if (!secret)
 		return PWDB_DENIED;
 
 	epasswd = encrypt_password(passwd, secret, req->RA, &epasswd_len);
-	_free(secret);
 	if (!epasswd)
 		return PWDB_DENIED;
 
diff --git a/accel-pppd/radius/packet.c b/accel-pppd/radius/packet.c
index 4a0ab244..9f135507 100644
--- a/accel-pppd/radius/packet.c
+++ b/accel-pppd/radius/packet.c
@@ -881,6 +881,7 @@ int rad_packet_send(struct rad_packet_t *pack, int fd, struct sockaddr_in *addr)
 		uint8_t hmac[HMAC_MD5_LEN];
 		uint8_t *ptr = pack->buf;
 		uint8_t *hmac_ptr = ptr + PACKET_SIGNED_OFFSET;
+		memset(hmac_ptr, 0, HMAC_MD5_LEN);
 		if (hmac_md5((const uint8_t *)pack->secret, strlen((const char *)pack->secret), pack->buf, pack->len, hmac) < 0) {
 			log_emerg("radius:packet: failed to calculate HMAC\n");
 			return -1;
diff --git a/accel-pppd/radius/req.c b/accel-pppd/radius/req.c
index 72c46b16..9a957493 100644
--- a/accel-pppd/radius/req.c
+++ b/accel-pppd/radius/req.c
@@ -5,6 +5,8 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <assert.h>
+#include <openssl/md5.h>
+#include <openssl/crypto.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -17,6 +19,49 @@
 
 #define HMAC_MD5_LEN 16
 
+/* Keep the signing secret and authenticator with the packet across reloads. */
+static int rad_req_set_RA(struct rad_req_t *req)
+{
+	char *secret = rad_server_secret_dup(req->serv);
+	MD5_CTX ctx;
+
+	if (!secret)
+		return -1;
+
+	memset(req->RA, 0, sizeof(req->RA));
+	if (rad_packet_build(req->pack, req->RA)) {
+		_free(secret);
+		return -1;
+	}
+
+	MD5_Init(&ctx);
+	MD5_Update(&ctx, req->pack->buf, req->pack->len);
+	MD5_Update(&ctx, secret, strlen(secret));
+	MD5_Final(req->pack->buf + 4, &ctx);
+	memcpy(req->RA, req->pack->buf + 4, sizeof(req->RA));
+	_free(req->pack->secret);
+	req->pack->secret = (uint8_t *)secret;
+	return 0;
+}
+
+static int verify_response_authenticator(struct rad_req_t *req, struct rad_packet_t *pack)
+{
+	uint8_t expected[MD5_DIGEST_LENGTH];
+	MD5_CTX ctx;
+
+	if (!pack || !pack->buf || pack->len < 20 ||
+	    !req->pack->buf || !req->pack->secret)
+		return -1;
+
+	MD5_Init(&ctx);
+	MD5_Update(&ctx, pack->buf, 4);
+	MD5_Update(&ctx, req->pack->buf + 4, 16);
+	MD5_Update(&ctx, pack->buf + 20, pack->len - 20);
+	MD5_Update(&ctx, req->pack->secret, strlen((char *)req->pack->secret));
+	MD5_Final(expected, &ctx);
+	return CRYPTO_memcmp(expected, pack->buf + 4, sizeof(expected));
+}
+
 static int make_socket(struct rad_req_t *req);
 static mempool_t req_pool;
 
@@ -75,12 +120,15 @@ static struct rad_req_t *__rad_req_alloc(struct radius_pd_t *rpd, int code, cons
 	if (!req->pack)
 		goto out_err;
 
-	if (code == CODE_ACCESS_REQUEST && conf_blast_protection) {
-		uint8_t buf[HMAC_MD5_LEN] = {0};
-		req->pack->message_authenticator = 1;
+	if (code == CODE_ACCESS_REQUEST) {
 		req->pack->secret = (uint8_t *)rad_server_secret_dup(req->serv);
 		if (!req->pack->secret)
 			goto out_err;
+	}
+
+	if (code == CODE_ACCESS_REQUEST && conf_blast_protection) {
+		uint8_t buf[HMAC_MD5_LEN] = {0};
+		req->pack->message_authenticator = 1;
 		if (rad_packet_add_octets(req->pack, NULL, "Message-Authenticator", buf, HMAC_MD5_LEN)) {
 			_free(req->pack->secret);
 			req->pack->secret = NULL;
@@ -380,6 +428,10 @@ int __rad_req_send(struct rad_req_t *req, int async)
 	if (req->before_send && req->before_send(req))
 		goto out_err;
 
+	/* Re-sign accounting after server selection, including retries without delay-time. */
+	if (req->pack->code == CODE_ACCOUNTING_REQUEST && rad_req_set_RA(req))
+		goto out_err;
+
 	if (!req->pack->buf && rad_packet_build(req->pack, req->RA))
 		goto out_err;
 
@@ -461,12 +513,15 @@ int rad_req_read(struct triton_md_handler_t *h)
 		if (rad_packet_recv(h->fd, &pack, NULL))
 			return 0;
 
-		rad_server_reply(req->serv);
-
-		if (pack->id == req->pack->id)
-			break;
+		if (!pack)
+			return 0;
+		if (pack->id != req->pack->id || verify_response_authenticator(req, pack)) {
+			rad_packet_free(pack);
+			continue;
+		}
 
-		rad_packet_free(pack);
+		rad_server_reply(req->serv);
+		break;
 	}
 
 	req->reply = pack;
diff --git a/accel-pppd/radius/serv.c b/accel-pppd/radius/serv.c
index 71398c82..34367731 100644
--- a/accel-pppd/radius/serv.c
+++ b/accel-pppd/radius/serv.c
@@ -11,7 +11,6 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 
-#include <openssl/md5.h>
 
 #include "log.h"
 #include "triton.h"
@@ -315,10 +314,19 @@ void rad_server_req_exit(struct rad_req_t *req)
 int rad_server_realloc(struct rad_req_t *req)
 {
 	struct rad_server_t *s = __rad_server_get(req->type, req->serv, 0, 0);
+	char *secret;
 
 	if (!s)
 		return -1;
 
+	secret = rad_server_secret_dup(s);
+	if (!secret) {
+		rad_server_put(s, req->type);
+		return -1;
+	}
+	_free(req->pack->secret);
+	req->pack->secret = (uint8_t *)secret;
+
 	if (req->serv)
 		rad_server_put(req->serv, req->type);
 
@@ -449,30 +457,6 @@ void rad_server_stat_interim_query(struct rad_server_t *s, unsigned int dt)
 	stat_accm_add(s->stat.interim_query_5m, dt);
 }
 
-static int req_set_RA(struct rad_req_t *req)
-{
-	char *secret;
-	MD5_CTX ctx;
-
-	secret = rad_server_secret_dup(req->serv);
-	if (!secret)
-		return -1;
-
-	if (rad_packet_build(req->pack, req->RA)) {
-		_free(secret);
-		return -1;
-	}
-
-	MD5_Init(&ctx);
-	MD5_Update(&ctx, req->pack->buf, req->pack->len);
-	MD5_Update(&ctx, secret, strlen(secret));
-	MD5_Final(req->pack->buf + 4, &ctx);
-
-	_free(secret);
-
-	return 0;
-}
-
 static void acct_on_sent(struct rad_req_t *req, int res)
 {
 	if (!res && !req->hnd.tpd) {
@@ -556,11 +540,9 @@ static void send_acct_on(struct rad_server_t *s)
 		if (rad_packet_add_ipaddr(req->pack, NULL, "NAS-IP-Address", conf_nas_ip_address))
 			goto out_err;
 
-	if (req_set_RA(req))
+	if (__rad_req_send(req, 0))
 		goto out_err;
 
-	__rad_req_send(req, 0);
-
 	triton_timer_add(&s->ctx, &req->timeout, 0);
 
 	return;