l2tp: validate the deciphered length of hidden AVPs

decode_avp() only checked the 2 bytes length prefix of the Hidden AVP
Subformat when the ciphered attribute spanned more than one MD5 block.
For an attribute of at most 16 bytes it deciphered the single block and
returned straight away, leaving l2tp_recv() to take the prefix at face
value:

	orig_avp_len = ntohs(*(uint16_t *)avp->val) + sizeof(*avp);
	...
	attr->length = orig_avp_len - sizeof(*avp);

That prefix is an output of the cipher, so a peer which does not know
the secret (i.e. anyone able to reach the L2TP socket, no handshake
needed beyond a preceding Random-Vector AVP) turns it into 16 random
bits. Worse than an over-read: orig_avp_len is a uint16_t, so a prefix
of 0xffff wraps to 5, attr->length becomes -1, and the ATTR_TYPE_STRING
and ATTR_TYPE_OCTETS cases then run

	attr->val.string = _malloc(attr->length + 1);	/* _malloc(0) */
	memcpy(attr->val.string, orig_avp_val, attr->length);	/* SIZE_MAX */

that is an unbounded memcpy() into a zero sized allocation. Remotely
triggerable heap corruption on any tunnel with a secret configured. The
accel-ppp encoder always pads hidden AVPs by at least 16 bytes, so it
never produces an attribute short enough to reach this path; only a
crafted packet does.

Fix it at the source rather than at the call site: decode_avp() now
returns the deciphered length through an output parameter, and the
length is bounded against the room actually available in the received
AVP on every path out of the function, single block included. Callers
can no longer re-derive it from the AVP body and get it wrong, and the
existing multi-block check keeps guarding the deciphering loop itself.

Add packet_test.c, a standalone test which drives the real parser
through a real UDP socket: hand-crafted packets cover the length prefix
checks (including the boundaries of what fits and the single block path
the encoder cannot produce), and l2tp_packet_send()/l2tp_recv() round
trips cover the multi-block cipher and the unaligned AVP accessors. It
reproduces the corruption above under ASan on unpatched code. Wire it,
and the so far unused bitpool_test.c, into the ASAN/UBSAN workflow.

Inspired by the equivalent hardening in accel-ppp-ng (commit a8ca0f3f),
which bounds the length at the call site; the fix here is placed inside
decode_avp() and covered by a regression test.

1 files changed, 38 insertions, 9 deletions

diff --git a/accel-pppd/ctrl/l2tp/packet.c b/accel-pppd/ctrl/l2tp/packet.c
index 68f36ece..0a4113a0 100644
--- a/accel-pppd/ctrl/l2tp/packet.c
+++ b/accel-pppd/ctrl/l2tp/packet.c
@@ -174,9 +174,17 @@ static void memxor(uint8_t *dst, const uint8_t *src, size_t sz)
 /*
  * Decipher hidden AVPs, keeping the Hidden AVP Subformat (i.e. the attribute
  * value is prefixed by 2 bytes indicating its length in network byte order).
+ *
+ * On success the deciphered original attribute length is stored into
+ * *orig_attr_len, already validated against the size of the received AVP.
+ * Callers must never re-read that length from the AVP body themselves: it is
+ * the output of the cipher, so a peer using a mismatching secret (or an
+ * attacker blindly injecting hidden AVPs) makes it an essentially random
+ * 16 bits value.
  */
 static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
-		      const char *secret, size_t secret_len)
+		      const char *secret, size_t secret_len,
+		      uint16_t *orig_attr_len_out)
 {
 	MD5_CTX md5_ctx;
 	uint8_t md5[MD5_DIGEST_LENGTH];
@@ -190,7 +198,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
 	uint16_t last_block_len;
 
 	avp_len = avp->flags & L2TP_AVP_LEN_MASK;
-	if (avp_len < sizeof(struct l2tp_avp_t) + 2) {
+	if (avp_len < sizeof(struct l2tp_avp_t) + sizeof(uint16_t)) {
 		/* Hidden AVPs must contain at least two bytes
 		   for storing original attribute length */
 		log_warn("l2tp: incorrect hidden avp received (type %hu):"
@@ -208,20 +216,22 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
 	MD5_Final(p1, &md5_ctx);
 
 	if (attr_len <= MD5_DIGEST_LENGTH) {
+		/* The whole attribute fits in the first block: it is fully
+		   deciphered, nothing more to do but to check its length */
 		memxor(avp->val, p1, attr_len);
-		return 0;
+		goto out;
 	}
 
 	memxor(p1, avp->val, MD5_DIGEST_LENGTH);
 	orig_attr_len = unaligned_ntohs(p1);
 
-	if (orig_attr_len <= MD5_DIGEST_LENGTH - 2) {
+	if (orig_attr_len <= MD5_DIGEST_LENGTH - sizeof(uint16_t)) {
 		/* Enough bytes decoded already, no need to decode padding */
 		memcpy(avp->val, p1, MD5_DIGEST_LENGTH);
-		return 0;
+		goto out;
 	}
 
-	if (orig_attr_len > attr_len - 2) {
+	if (orig_attr_len > attr_len - sizeof(uint16_t)) {
 		log_warn("l2tp: incorrect hidden avp received (type %hu):"
 			 " original attribute length too big (ciphered"
 			 " attribute length: %hu bytes, advertised original"
@@ -232,7 +242,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
 
 	/* Decode remaining blocks. Start from the last block as
 	   preceding blocks must be kept hidden for computing MD5s */
-	bytes_left = orig_attr_len + 2 - MD5_DIGEST_LENGTH;
+	bytes_left = orig_attr_len + sizeof(uint16_t) - MD5_DIGEST_LENGTH;
 	last_block_len = bytes_left % MD5_DIGEST_LENGTH;
 	blocks_left = bytes_left / MD5_DIGEST_LENGTH;
 	if (last_block_len) {
@@ -256,6 +266,23 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
 	}
 	memcpy(avp->val, p1, MD5_DIGEST_LENGTH);
 
+out:
+	/* The length prefix comes out of the cipher, so it is only as
+	   trustworthy as the peer's knowledge of the shared secret. Bound it
+	   against the room actually available in the received AVP before
+	   letting it drive any read of the attribute value */
+	orig_attr_len = unaligned_ntohs(avp->val);
+	if (orig_attr_len > attr_len - sizeof(uint16_t)) {
+		log_warn("l2tp: incorrect hidden avp received (type %hu):"
+			 " deciphered attribute length too big (ciphered"
+			 " attribute length: %hu bytes, deciphered original"
+			 " attribute length: %hu bytes), wrong secret?\n",
+			 ntohs(avp->type), attr_len, orig_attr_len);
+		return -1;
+	}
+
+	*orig_attr_len_out = orig_attr_len;
+
 	return 0;
 }
 
@@ -269,6 +296,7 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info,
 	struct sockaddr_in addr;
 	socklen_t addr_len;
 	uint16_t orig_avp_len;
+	uint16_t orig_attr_len;
 	void *orig_avp_val;
 	uint8_t *buf, *ptr;
 	int n, length;
@@ -448,10 +476,11 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info,
 						  ntohs(avp->type));
 					goto out_err;
 				}
-				if (decode_avp(avp, RV, secret, secret_len) < 0)
+				if (decode_avp(avp, RV, secret, secret_len,
+					       &orig_attr_len) < 0)
 					goto out_err;
 
-				orig_avp_len = unaligned_ntohs(avp->val) + sizeof(*avp);
+				orig_avp_len = orig_attr_len + sizeof(*avp);
 				orig_avp_val = avp->val + sizeof(uint16_t);
 			} else {
 				orig_avp_len = avp_len;