Hi,
this diff changes our SA payload parser to parse more than the first
proposal. This allows us to select one of the peer's proposals (and not
only the first). The xforms parser calls itself recursively without
limits, which I'm not sure is a good idea. This diff uses a do {} while
for the proposal parser instead. I'm not sure this is great, but I
think it's better than the recursive way. I'd like to change the xforms
parser to do the same later on.
ok?
Patrick
diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c
index f3e21c3a41c..fb599d1c080 100644
--- a/sbin/iked/ikev2_pld.c
+++ b/sbin/iked/ikev2_pld.c
@@ -326,10 +326,6 @@ ikev2_validate_sa(struct iked_message *msg, size_t offset,
size_t left,
return (0);
}
-/*
- * NB: This function parses both the SA header and the first proposal.
- * Additional proposals are ignored.
- */
int
ikev2_pld_sa(struct iked *env, struct ikev2_payload *pld,
struct iked_message *msg, size_t offset, size_t left)
@@ -342,100 +338,102 @@ ikev2_pld_sa(struct iked *env, struct ikev2_payload
*pld,
struct iked_proposals *props;
size_t total;
- if (ikev2_validate_sa(msg, offset, left, &sap))
- return (-1);
+ do {
+ if (ikev2_validate_sa(msg, offset, left, &sap))
+ return (-1);
- if (sap.sap_more)
- log_debug("%s: more than one proposal specified", __func__);
+ /* Assumed size of the first proposals, including SPI if
present. */
+ total = (betoh16(sap.sap_length) - sizeof(sap));
- /* Assumed size of the first proposals, including SPI if present. */
- total = (betoh16(sap.sap_length) - sizeof(sap));
+ props = &msg->msg_parent->msg_proposals;
- props = &msg->msg_parent->msg_proposals;
+ offset += sizeof(sap);
+ left -= sizeof(sap);
+
+ if (sap.sap_spisize) {
+ if (left < sap.sap_spisize) {
+ log_debug("%s: malformed payload: SPI larger
than "
+ "actual payload (%zu < %d)", __func__, left,
+ sap.sap_spisize);
+ return (-1);
+ }
+ if (total < sap.sap_spisize) {
+ log_debug("%s: malformed payload: SPI larger
than "
+ "proposal (%zu < %d)", __func__, total,
+ sap.sap_spisize);
+ return (-1);
+ }
+ if (total < sap.sap_spisize) {
+ log_debug("%s: malformed payload: SPI too large
"
+ "(%zu < %d)", __func__, total,
sap.sap_spisize);
+ return (-1);
+ }
+ switch (sap.sap_spisize) {
+ case 4:
+ memcpy(&spi32, msgbuf + offset, 4);
+ spi = betoh32(spi32);
+ break;
+ case 8:
+ memcpy(&spi64, msgbuf + offset, 8);
+ spi = betoh64(spi64);
+ break;
+ default:
+ log_debug("%s: unsupported SPI size %d",
+ __func__, sap.sap_spisize);
+ return (-1);
+ }
- offset += sizeof(sap);
- left -= sizeof(sap);
+ offset += sap.sap_spisize;
+ left -= sap.sap_spisize;
- if (sap.sap_spisize) {
- if (left < sap.sap_spisize) {
- log_debug("%s: malformed payload: SPI larger than "
- "actual payload (%zu < %d)", __func__, left,
- sap.sap_spisize);
- return (-1);
- }
- if (total < sap.sap_spisize) {
- log_debug("%s: malformed payload: SPI larger than "
- "proposal (%zu < %d)", __func__, total,
- sap.sap_spisize);
- return (-1);
+ /* Assumed size of the proposal, now without SPI. */
+ total -= sap.sap_spisize;
}
- if (total < sap.sap_spisize) {
- log_debug("%s: malformed payload: SPI too large "
- "(%zu < %d)", __func__, total, sap.sap_spisize);
- return (-1);
- }
- switch (sap.sap_spisize) {
- case 4:
- memcpy(&spi32, msgbuf + offset, 4);
- spi = betoh32(spi32);
- break;
- case 8:
- memcpy(&spi64, msgbuf + offset, 8);
- spi = betoh64(spi64);
- break;
- default:
- log_debug("%s: unsupported SPI size %d",
- __func__, sap.sap_spisize);
+
+ /*
+ * As we verified sanity of packet headers, this check will
+ * be always false, but just to be sure we keep it.
+ */
+ if (left < total) {
+ log_debug("%s: malformed payload: too long for payload "
+ "(%zu < %zu)", __func__, left, total);
return (-1);
}
- offset += sap.sap_spisize;
- left -= sap.sap_spisize;
+ log_debug("%s: more %d reserved %d length %d"
+ " proposal #%d protoid %s spisize %d xforms %d spi %s",
+ __func__, sap.sap_more, sap.sap_reserved,
+ betoh16(sap.sap_length), sap.sap_proposalnr,
+ print_map(sap.sap_protoid, ikev2_saproto_map),
sap.sap_spisize,
+ sap.sap_transforms, print_spi(spi, sap.sap_spisize));
- /* Assumed size of the proposal, now without SPI. */
- total -= sap.sap_spisize;
- }
-
- /*
- * As we verified sanity of packet headers, this check will
- * be always false, but just to be sure we keep it.
- */
- if (left < total) {
- log_debug("%s: malformed payload: too long for payload "
- "(%zu < %zu)", __func__, left, total);
- return (-1);
- }
+ if (ikev2_msg_frompeer(msg)) {
+ if ((msg->msg_parent->msg_prop =
config_add_proposal(props,
+ sap.sap_proposalnr, sap.sap_protoid)) == NULL) {
+ log_debug("%s: invalid proposal", __func__);
+ return (-1);
+ }
+ prop = msg->msg_parent->msg_prop;
+ prop->prop_peerspi.spi = spi;
+ prop->prop_peerspi.spi_protoid = sap.sap_protoid;
+ prop->prop_peerspi.spi_size = sap.sap_spisize;
- log_debug("%s: more %d reserved %d length %d"
- " proposal #%d protoid %s spisize %d xforms %d spi %s",
- __func__, sap.sap_more, sap.sap_reserved,
- betoh16(sap.sap_length), sap.sap_proposalnr,
- print_map(sap.sap_protoid, ikev2_saproto_map), sap.sap_spisize,
- sap.sap_transforms, print_spi(spi, sap.sap_spisize));
+ prop->prop_localspi.spi_protoid = sap.sap_protoid;
+ prop->prop_localspi.spi_size = sap.sap_spisize;
+ }
- if (ikev2_msg_frompeer(msg)) {
- if ((msg->msg_parent->msg_prop = config_add_proposal(props,
- sap.sap_proposalnr, sap.sap_protoid)) == NULL) {
- log_debug("%s: invalid proposal", __func__);
+ /*
+ * Parse the attached transforms
+ */
+ if (sap.sap_transforms &&
+ ikev2_pld_xform(env, &sap, msg, offset, total) != 0) {
+ log_debug("%s: invalid proposal transforms", __func__);
return (-1);
}
- prop = msg->msg_parent->msg_prop;
- prop->prop_peerspi.spi = spi;
- prop->prop_peerspi.spi_protoid = sap.sap_protoid;
- prop->prop_peerspi.spi_size = sap.sap_spisize;
- prop->prop_localspi.spi_protoid = sap.sap_protoid;
- prop->prop_localspi.spi_size = sap.sap_spisize;
- }
-
- /*
- * Parse the attached transforms
- */
- if (sap.sap_transforms &&
- ikev2_pld_xform(env, &sap, msg, offset, total) != 0) {
- log_debug("%s: invalid proposal transforms", __func__);
- return (-1);
- }
+ offset += total;
+ left -= total;
+ } while (sap.sap_more);
return (0);
}
