Merge branch 'ikev1-transform-nr'

With these changes we return the lifetimes of the actually selected
transform back to the client, which is an issue if the peer uses
different lifetimes for different proposals.  We now also return the
correct transform and proposal IDs.

Fixes #3329.

src/libcharon/encoding/payloads/proposal_substructure.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2014 Tobias Brunner
+ * Copyright (C) 2012-2020 Tobias Brunner
  * Copyright (C) 2005-2010 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -1004,18 +1004,25 @@ METHOD(proposal_substructure_t, get_proposals, void,
 	enumerator = this->transforms->create_enumerator(this->transforms);
 	while (enumerator->enumerate(enumerator, &transform))
 	{
-		if (!proposal)
-		{
-			proposal = proposal_create(this->protocol_id, this->proposal_number);
-			proposal->set_spi(proposal, spi);
-			proposals->insert_last(proposals, proposal);
-		}
 		if (this->type == PLV2_PROPOSAL_SUBSTRUCTURE)
 		{
+			if (!proposal)
+			{
+				proposal = proposal_create(this->protocol_id,
+										   this->proposal_number);
+				proposal->set_spi(proposal, spi);
+				proposals->insert_last(proposals, proposal);
+			}
 			add_to_proposal_v2(proposal, transform);
 		}
 		else
 		{
+			/* create a new proposal for each transform in IKEv1 */
+			proposal = proposal_create_v1(
+							this->protocol_id, this->proposal_number,
+							transform->get_transform_type_or_number(transform));
+			proposal->set_spi(proposal, spi);
+			proposals->insert_last(proposals, proposal);
 			switch (this->protocol_id)
 			{
 				case PROTO_IKE:
@@ -1028,8 +1035,6 @@ METHOD(proposal_substructure_t, get_proposals, void,
 				default:
 					break;
 			}
-			/* create a new proposal for each transform in IKEv1 */
-			proposal = NULL;
 		}
 	}
 	enumerator->destroy(enumerator);
@@ -1074,8 +1079,8 @@ static uint64_t get_attr(private_proposal_substructure_t *this,
  * Look up a lifetime duration of a given kind in all transforms
  */
 static uint64_t get_life_duration(private_proposal_substructure_t *this,
-				transform_attribute_type_t type_attr, ikev1_life_type_t type,
-				transform_attribute_type_t dur_attr)
+				uint8_t number, transform_attribute_type_t type_attr,
+				ikev1_life_type_t type, transform_attribute_type_t dur_attr)
 {
 	enumerator_t *transforms, *attributes;
 	transform_substructure_t *transform;
@@ -1084,6 +1089,10 @@ static uint64_t get_life_duration(private_proposal_substructure_t *this,
 	transforms = this->transforms->create_enumerator(this->transforms);
 	while (transforms->enumerate(transforms, &transform))
 	{
+		if (transform->get_transform_type_or_number(transform) != number)
+		{
+			continue;
+		}
 		attributes = transform->create_attribute_enumerator(transform);
 		while (attributes->enumerate(attributes, &attr))
 		{
@@ -1108,19 +1117,20 @@ static uint64_t get_life_duration(private_proposal_substructure_t *this,
 }
 
 METHOD(proposal_substructure_t, get_lifetime, uint32_t,
-	private_proposal_substructure_t *this)
+	private_proposal_substructure_t *this, uint8_t transform)
 {
 	uint32_t duration;
 
 	switch (this->protocol_id)
 	{
 		case PROTO_IKE:
-			return get_life_duration(this, TATTR_PH1_LIFE_TYPE,
-						IKEV1_LIFE_TYPE_SECONDS, TATTR_PH1_LIFE_DURATION);
+			return get_life_duration(this, transform, TATTR_PH1_LIFE_TYPE,
+							IKEV1_LIFE_TYPE_SECONDS, TATTR_PH1_LIFE_DURATION);
 		case PROTO_ESP:
 		case PROTO_AH:
-			duration = get_life_duration(this, TATTR_PH2_SA_LIFE_TYPE,
-						IKEV1_LIFE_TYPE_SECONDS, TATTR_PH2_SA_LIFE_DURATION);
+			duration = get_life_duration(this, transform,
+							TATTR_PH2_SA_LIFE_TYPE, IKEV1_LIFE_TYPE_SECONDS,
+							TATTR_PH2_SA_LIFE_DURATION);
 			if (!duration)
 			{	/* default to 8 hours, RFC 2407 */
 				return 28800;
@@ -1132,14 +1142,15 @@ METHOD(proposal_substructure_t, get_lifetime, uint32_t,
 }
 
 METHOD(proposal_substructure_t, get_lifebytes, uint64_t,
-	private_proposal_substructure_t *this)
+	private_proposal_substructure_t *this, uint8_t transform)
 {
 	switch (this->protocol_id)
 	{
 		case PROTO_ESP:
 		case PROTO_AH:
-			return 1000 * get_life_duration(this, TATTR_PH2_SA_LIFE_TYPE,
-					IKEV1_LIFE_TYPE_KILOBYTES, TATTR_PH2_SA_LIFE_DURATION);
+			return 1000 * get_life_duration(this, transform,
+							TATTR_PH2_SA_LIFE_TYPE, IKEV1_LIFE_TYPE_KILOBYTES,
+							TATTR_PH2_SA_LIFE_DURATION);
 		case PROTO_IKE:
 		default:
 			return 0;
@@ -1525,7 +1536,9 @@ static void set_data(private_proposal_substructure_t *this, proposal_t *proposal
 		default:
 			break;
 	}
-	this->proposal_number = proposal->get_number(proposal);
+	/* default to 1 if no number is set (mainly for IKEv1, for IKEv2 the numbers
+	 * are explicitly set when proposals are added to the SA payload) */
+	this->proposal_number = proposal->get_number(proposal) ?: 1;
 	this->protocol_id = proposal->get_protocol(proposal);
 	compute_length(this);
 }
@@ -1547,11 +1560,11 @@ proposal_substructure_t *proposal_substructure_create_from_proposal_v2(
 }
 
 /**
- * See header.
+ * Creates an IKEv1 proposal_substructure_t from a proposal_t.
  */
-proposal_substructure_t *proposal_substructure_create_from_proposal_v1(
+static proposal_substructure_t *proposal_substructure_create_from_proposal_v1(
 			proposal_t *proposal, uint32_t lifetime, uint64_t lifebytes,
-			auth_method_t auth, ipsec_mode_t mode, encap_t udp)
+			auth_method_t auth, ipsec_mode_t mode, encap_t udp, uint8_t number)
 {
 	private_proposal_substructure_t *this;
 
@@ -1560,12 +1573,12 @@ proposal_substructure_t *proposal_substructure_create_from_proposal_v1(
 	switch (proposal->get_protocol(proposal))
 	{
 		case PROTO_IKE:
-			set_from_proposal_v1_ike(this, proposal, lifetime, auth, 1);
+			set_from_proposal_v1_ike(this, proposal, lifetime, auth, number);
 			break;
 		case PROTO_ESP:
 		case PROTO_AH:
 			set_from_proposal_v1(this, proposal, lifetime,
-								 lifebytes, mode, udp, 1);
+								 lifebytes, mode, udp, number);
 			break;
 		default:
 			break;
@@ -1585,17 +1598,18 @@ proposal_substructure_t *proposal_substructure_create_from_proposals_v1(
 	private_proposal_substructure_t *this = NULL;
 	enumerator_t *enumerator;
 	proposal_t *proposal;
-	int number = 0;
+	int number = 1;
 
 	enumerator = proposals->create_enumerator(proposals);
 	while (enumerator->enumerate(enumerator, &proposal))
 	{
 		if (!this)
-		{
+		{	/* as responder the transform number is set and we only have a
+			 * single proposal, start with 1 otherwise */
 			this = (private_proposal_substructure_t*)
 						proposal_substructure_create_from_proposal_v1(
-								proposal, lifetime, lifebytes, auth, mode, udp);
-			++number;
+							proposal, lifetime, lifebytes, auth, mode, udp,
+							proposal->get_transform_number(proposal) ?: number);
 		}
 		else
 		{

src/libcharon/encoding/payloads/proposal_substructure.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 Tobias Brunner
+ * Copyright (C) 2012-2020 Tobias Brunner
  * Copyright (C) 2005-2006 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -131,18 +131,20 @@ struct proposal_substructure_t {
 	enumerator_t* (*create_substructure_enumerator)(proposal_substructure_t *this);
 
 	/**
-	 * Get the (shortest) lifetime of a proposal (IKEv1 only).
+	 * Get the lifetime of a transform (IKEv1 only).
 	 *
+	 * @param transform			transform number
 	 * @return					lifetime, in seconds
 	 */
-	uint32_t (*get_lifetime)(proposal_substructure_t *this);
+	uint32_t (*get_lifetime)(proposal_substructure_t *this, uint8_t transform);
 
 	/**
-	 * Get the (shortest) life duration of a proposal (IKEv1 only).
+	 * Get the life duration of a transform (IKEv1 only).
 	 *
+	 * @param transform			transform number
 	 * @return					life duration, in bytes
 	 */
-	uint64_t (*get_lifebytes)(proposal_substructure_t *this);
+	uint64_t (*get_lifebytes)(proposal_substructure_t *this, uint8_t transform);
 
 	/**
 	 * Get the first authentication method from the proposal (IKEv1 only).
@@ -181,20 +183,6 @@ proposal_substructure_t *proposal_substructure_create(payload_type_t type);
  */
 proposal_substructure_t *proposal_substructure_create_from_proposal_v2(
 														proposal_t *proposal);
-/**
- * Creates an IKEv1 proposal_substructure_t from a proposal_t.
- *
- * @param proposal	proposal to build a substruct out of it
- * @param lifetime	lifetime in seconds
- * @param lifebytes	lifebytes, in bytes
- * @param auth		authentication method to use, or AUTH_NONE
- * @param mode		IPsec encapsulation mode, TRANSPORT or TUNNEL
- * @param udp		ENCAP_UDP to use UDP encapsulation
- * @return			proposal_substructure_t object PLV1_PROPOSAL_SUBSTRUCTURE
- */
-proposal_substructure_t *proposal_substructure_create_from_proposal_v1(
-			proposal_t *proposal,  uint32_t lifetime, uint64_t lifebytes,
-			auth_method_t auth, ipsec_mode_t mode, encap_t udp);
 
 /**
  * Creates an IKEv1 proposal_substructure_t from a list of proposal_t.

src/libcharon/encoding/payloads/sa_payload.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2014 Tobias Brunner
+ * Copyright (C) 2012-2020 Tobias Brunner
  * Copyright (C) 2005-2010 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -404,16 +404,22 @@ METHOD(sa_payload_t, create_substructure_enumerator, enumerator_t*,
 }
 
 METHOD(sa_payload_t, get_lifetime, uint32_t,
-	private_sa_payload_t *this)
+	private_sa_payload_t *this, proposal_t *proposal)
 {
 	proposal_substructure_t *substruct;
 	enumerator_t *enumerator;
+	uint8_t number = proposal->get_number(proposal);
 	uint32_t lifetime = 0;
 
 	enumerator = this->proposals->create_enumerator(this->proposals);
-	if (enumerator->enumerate(enumerator, &substruct))
+	while (enumerator->enumerate(enumerator, &substruct))
 	{
-		lifetime = substruct->get_lifetime(substruct);
+		if (substruct->get_proposal_number(substruct) == number)
+		{
+			lifetime = substruct->get_lifetime(substruct,
+									proposal->get_transform_number(proposal));
+			break;
+		}
 	}
 	enumerator->destroy(enumerator);
 
@@ -421,16 +427,22 @@ METHOD(sa_payload_t, get_lifetime, uint32_t,
 }
 
 METHOD(sa_payload_t, get_lifebytes, uint64_t,
-	private_sa_payload_t *this)
+	private_sa_payload_t *this, proposal_t *proposal)
 {
 	proposal_substructure_t *substruct;
 	enumerator_t *enumerator;
+	uint8_t number = proposal->get_number(proposal);
 	uint64_t lifebytes = 0;
 
 	enumerator = this->proposals->create_enumerator(this->proposals);
-	if (enumerator->enumerate(enumerator, &substruct))
+	while (enumerator->enumerate(enumerator, &substruct))
 	{
-		lifebytes = substruct->get_lifebytes(substruct);
+		if (substruct->get_proposal_number(substruct) == number)
+		{
+			lifebytes = substruct->get_lifebytes(substruct,
+									proposal->get_transform_number(proposal));
+			break;
+		}
 	}
 	enumerator->destroy(enumerator);
 

src/libcharon/encoding/payloads/sa_payload.h

@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2012-2020 Tobias Brunner
  * Copyright (C) 2005-2006 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -60,18 +61,20 @@ struct sa_payload_t {
 	linked_list_t *(*get_ipcomp_proposals) (sa_payload_t *this, uint16_t *cpi);
 
 	/**
-	 * Get the (shortest) lifetime of a proposal (IKEv1 only).
+	 * Get the lifetime of a proposal/transform (IKEv1 only).
 	 *
+	 * @param proposal			proposal for which to get lifetime
 	 * @return					lifetime, in seconds
 	 */
-	uint32_t (*get_lifetime)(sa_payload_t *this);
+	uint32_t (*get_lifetime)(sa_payload_t *this, proposal_t *proposal);
 
 	/**
-	 * Get the (shortest) life duration of a proposal (IKEv1 only).
+	 * Get the life duration of a proposal/transform (IKEv1 only).
 	 *
+	 * @param proposal			proposal for which to get life duration
 	 * @return					life duration, in bytes
 	 */
-	uint64_t (*get_lifebytes)(sa_payload_t *this);
+	uint64_t (*get_lifebytes)(sa_payload_t *this, proposal_t *proposal);
 
 	/**
 	 * Get the first authentication method from the proposal (IKEv1 only).

src/libcharon/sa/ikev1/tasks/aggressive_mode.c

@@ -415,7 +415,8 @@ METHOD(task_t, process_r, status_t,
 			this->ike_sa->set_proposal(this->ike_sa, this->proposal);
 
 			this->method = sa_payload->get_auth_method(sa_payload);
-			this->lifetime = sa_payload->get_lifetime(sa_payload);
+			this->lifetime = sa_payload->get_lifetime(sa_payload,
+													  this->proposal);
 
 			switch (this->method)
 			{
@@ -653,7 +654,7 @@ METHOD(task_t, process_i, status_t,
 		}
 		this->ike_sa->set_proposal(this->ike_sa, this->proposal);
 
-		lifetime = sa_payload->get_lifetime(sa_payload);
+		lifetime = sa_payload->get_lifetime(sa_payload, this->proposal);
 		if (lifetime != this->lifetime)
 		{
 			DBG1(DBG_IKE, "received lifetime %us does not match configured "

src/libcharon/sa/ikev1/tasks/main_mode.c

@@ -406,7 +406,8 @@ METHOD(task_t, process_r, status_t,
 			this->ike_sa->set_proposal(this->ike_sa, this->proposal);
 
 			this->method = sa_payload->get_auth_method(sa_payload);
-			this->lifetime = sa_payload->get_lifetime(sa_payload);
+			this->lifetime = sa_payload->get_lifetime(sa_payload,
+													  this->proposal);
 
 			this->state = MM_SA;
 			return NEED_MORE;
@@ -654,7 +655,7 @@ METHOD(task_t, process_i, status_t,
 			}
 			this->ike_sa->set_proposal(this->ike_sa, this->proposal);
 
-			lifetime = sa_payload->get_lifetime(sa_payload);
+			lifetime = sa_payload->get_lifetime(sa_payload, this->proposal);
 			if (lifetime != this->lifetime)
 			{
 				DBG1(DBG_IKE, "received lifetime %us does not match configured "

src/libcharon/sa/ikev1/tasks/quick_mode.c

@@ -744,8 +744,8 @@ static void apply_lifetimes(private_quick_mode_t *this, sa_payload_t *sa_payload
 	uint32_t lifetime;
 	uint64_t lifebytes;
 
-	lifetime = sa_payload->get_lifetime(sa_payload);
-	lifebytes = sa_payload->get_lifebytes(sa_payload);
+	lifetime = sa_payload->get_lifetime(sa_payload, this->proposal);
+	lifebytes = sa_payload->get_lifebytes(sa_payload, this->proposal);
 	if (this->lifetime != lifetime)
 	{
 		DBG1(DBG_IKE, "received %us lifetime, configured %us",

src/libstrongswan/crypto/proposal/proposal.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2018 Tobias Brunner
+ * Copyright (C) 2008-2020 Tobias Brunner
  * Copyright (C) 2006-2010 Martin Willi
  * Copyright (C) 2013-2015 Andreas Steffen
  * HSR Hochschule fuer Technik Rapperswil
@@ -70,7 +70,12 @@ struct private_proposal_t {
 	/**
 	 * Proposal number
 	 */
-	u_int number;
+	uint8_t number;
+
+	/**
+	 * Transform number (IKEv1 only)
+	 */
+	uint8_t transform_number;
 };
 
 /**
@@ -455,12 +460,14 @@ METHOD(proposal_t, select_proposal, proposal_t*,
 
 	if (flags & PROPOSAL_PREFER_SUPPLIED)
 	{
-		selected = proposal_create(this->protocol, this->number);
+		selected = proposal_create_v1(this->protocol, this->number,
+									  this->transform_number);
 		selected->set_spi(selected, this->spi);
 	}
 	else
 	{
-		selected = proposal_create(this->protocol, other->get_number(other));
+		selected = proposal_create_v1(this->protocol, other->get_number(other),
+									  other->get_transform_number(other));
 		selected->set_spi(selected, other->get_spi(other));
 	}
 
@@ -539,12 +546,18 @@ static bool algo_list_equals(private_proposal_t *this, proposal_t *other,
 	return equals;
 }
 
-METHOD(proposal_t, get_number, u_int,
+METHOD(proposal_t, get_number, uint8_t,
 	private_proposal_t *this)
 {
 	return this->number;
 }
 
+METHOD(proposal_t, get_transform_number, uint8_t,
+	private_proposal_t *this)
+{
+	return this->transform_number;
+}
+
 METHOD(proposal_t, equals, bool,
 	private_proposal_t *this, proposal_t *other)
 {
@@ -598,6 +611,7 @@ METHOD(proposal_t, clone_, proposal_t*,
 
 	clone->spi = this->spi;
 	clone->number = this->number;
+	clone->transform_number = this->transform_number;
 
 	return &clone->public;
 }
@@ -918,7 +932,8 @@ METHOD(proposal_t, destroy, void,
 /*
  * Described in header
  */
-proposal_t *proposal_create(protocol_id_t protocol, u_int number)
+proposal_t *proposal_create_v1(protocol_id_t protocol, uint8_t number,
+							   uint8_t transform)
 {
 	private_proposal_t *this;
 
@@ -935,12 +950,14 @@ proposal_t *proposal_create(protocol_id_t protocol, u_int number)
 			.set_spi = _set_spi,
 			.get_spi = _get_spi,
 			.get_number = _get_number,
+			.get_transform_number = _get_transform_number,
 			.equals = _equals,
 			.clone = _clone_,
 			.destroy = _destroy,
 		},
 		.protocol = protocol,
 		.number = number,
+		.transform_number = transform,
 		.transforms = array_create(sizeof(entry_t), 0),
 		.types = array_create(sizeof(transform_type_t), 0),
 	);
@@ -948,6 +965,14 @@ proposal_t *proposal_create(protocol_id_t protocol, u_int number)
 	return &this->public;
 }
 
+/*
+ * Described in header
+ */
+proposal_t *proposal_create(protocol_id_t protocol, uint8_t number)
+{
+	return proposal_create_v1(protocol, number, 0);
+}
+
 /**
  * Add supported IKE algorithms to proposal
  */

src/libstrongswan/crypto/proposal/proposal.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009-2019 Tobias Brunner
+ * Copyright (C) 2009-2020 Tobias Brunner
  * Copyright (C) 2006 Martin Willi
  * HSR Hochschule fuer Technik Rapperswil
  *
@@ -181,7 +181,14 @@ struct proposal_t {
 	 *
 	 * @return				proposal number
 	 */
-	u_int (*get_number)(proposal_t *this);
+	uint8_t (*get_number)(proposal_t *this);
+
+	/**
+	 * Get number of the transform on which this proposal is based (IKEv1 only)
+	 *
+	 * @return				transform number (or 0)
+	 */
+	uint8_t (*get_transform_number)(proposal_t *this);
 
 	/**
 	 * Check for the equality of two proposals.
@@ -212,7 +219,18 @@ struct proposal_t {
  * @param number			proposal number, as encoded in SA payload
  * @return					proposal_t object
  */
-proposal_t *proposal_create(protocol_id_t protocol, u_int number);
+proposal_t *proposal_create(protocol_id_t protocol, uint8_t number);
+
+/**
+ * Create a proposal for IKE, ESP or AH that includes a transform number.
+ *
+ * @param protocol			protocol, such as PROTO_ESP
+ * @param number			proposal number, as encoded in SA payload
+ * @param transform			transform number, as encoded in payload
+ * @return					proposal_t object
+ */
+proposal_t *proposal_create_v1(protocol_id_t protocol, uint8_t number,
+							   uint8_t transform);
 
 /**
  * Create a default proposal.