|
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.
|
