From: hshoexer <hshoexer@yerbouti.franken.de>
Subject: Re: isakmpd(8): Validate proposal and transform sizes
To: tech@openbsd.org
Date: Mon, 8 Jun 2026 17:08:39 +0200

Hi,

anyone?

On Wed, Apr 22, 2026 at 03:01:20PM +0200, hshoexer wrote:
> Check
> - that a proposal payload fits within the outer SA payload,
> - that the provided SPI and the following transform header fit
>   within the proposal, and
> - transforms fit within the outer proposal payload.
> 
> It's already ensured that we never read outside the message.  However,
> within the message the mentioned payloads might be misinterpreted
> due to malformed size values.
> 
> ok?
> 
> Take care,
> HJ.
> 
> ------------------------------------------------------------------------------
> diff --git a/sbin/isakmpd/message.c b/sbin/isakmpd/message.c
> index ee0df3a4282..c49a14d67b7 100644
> --- a/sbin/isakmpd/message.c
> +++ b/sbin/isakmpd/message.c
> @@ -335,7 +335,7 @@ next_payload:
>   * Parse a proposal payload found in message MSG.  PAYLOAD is always
>   * ISAKMP_PAYLOAD_PROPOSAL and ignored in here.  It's needed as the API for
>   * message_parse_payloads requires it.  BUF points to the proposal's
> - * generic payload header.
> + * generic payload header.  P is the outer SA payload.
>   */
>  static int
>  message_parse_proposal(struct message *msg, struct payload *p,
> @@ -343,6 +343,19 @@ message_parse_proposal(struct message *msg, struct payload *p,
>  {
>  	set	payload_set;
>  
> +	/* Proposal must lie within outer SA payload. */
> +	if (buf + GET_ISAKMP_GEN_LENGTH(buf) >
> +	    p->p + GET_ISAKMP_GEN_LENGTH(p->p)) {
> +		message_drop(msg, ISAKMP_NOTIFY_PAYLOAD_MALFORMED, 0, 1, 1);
> +		return -1;
> +	}
> +
> +	if ((ISAKMP_PROP_SPI_OFF + GET_ISAKMP_PROP_SPI_SZ(buf) +
> +	    ISAKMP_GEN_SZ) > GET_ISAKMP_GEN_LENGTH(buf)) {
> +		message_drop(msg, ISAKMP_NOTIFY_PAYLOAD_MALFORMED, 0, 1, 1);
> +		return -1;
> +	}
> +
>  	/* Put the proposal into the proposal bucket.  */
>  	if (message_index_payload(msg, p, payload, buf) == -1)
>  		return -1;
> @@ -363,6 +376,13 @@ static int
>  message_parse_transform(struct message *msg, struct payload *p,
>      u_int8_t payload, u_int8_t *buf)
>  {
> +	/* Transform must lie within outer proposal payload. */
> +	if (buf + GET_ISAKMP_GEN_LENGTH(buf) >
> +	    p->p + GET_ISAKMP_GEN_LENGTH(p->p)) {
> +		message_drop(msg, ISAKMP_NOTIFY_PAYLOAD_MALFORMED, 0, 1, 1);
> +		return -1;
> +	}
> +
>  	/* Put the transform into the transform bucket.  */
>  	if (message_index_payload(msg, p, payload, buf) == -1)
>  		return -1;
>