On Wed, Jun 14, 2023 at 12:37:35PM +0200, Theo Buehler wrote:
> On Wed, Jun 14, 2023 at 11:38:15AM +0200, Claudio Jeker wrote:
> > On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> > > On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > > > There is no real need to have print_host() with the extra arguments.
> > > > So convert the last remaining print_host() calls to use print_addr().
> > > > I'm not entierly sure how to really test all these code paths but the
> > > > changes are failry simple.
> > >
> > > Thanks for beating me to it...
> > >
> > > In ikev2_pld_cp() you need to garbage collect buf and replace its
> > > remaining uses by the appropriate print_addr().
> >
> > Oh boy, I missed the fact that this double prints the same info.
> > I moved the first log_debug as default: of the switch statement.
>
> Yeah, that makes sense.
>
> > > In ikev2_print_id() it would seem more appropriate to use strlcat()
> > > instead of strlcpy() in the switch and and reserve the special dance
> > > of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> > > should be done in a separate diff.
> >
> > Oh boy, this is even worse than the above case. WTF!
>
> Indeed.
>
> > So many things are utterly bad in this function and the XXX test path is
> > very broken. I tried to fix this up a bit.
>
> Should s4 and s6 not be struct sockaddr_in{,6} on the stack and get a
> treatment similar to start4 and start6 in ikev2_pld_ts()? This buf thing
> seems completely nuts. The weird length check in the FQDN case could be
> done with a simple comparison to BUFSIZ and then buf could become a
> local 3-byte buffer in the for loop of the XXX test case.
>
> However, I completely understand if you don't want to touch more of this
> mess than you already did...
It never pays out to be lazy. Fixed in diff below.
Guess that saves around 990bytes of stack :)
> Just one thing, otherwise your diff is ok:
>
> > @@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
> > break;
> > default:
> > /* XXX test */
> > - for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
> > - snprintf(idstr + i, idstrlen - i,
> > - "%02x", ptr[i]);
> > + for (i = 0; i < len; i++) {
> > + snprintf(buf, sizeof(buf), "%02x", ptr[i]);
> > + if (strlcat(idstr, buf, idstrlen) >= idstrlen)
>
> Should this not return -1 like the other cases?
>
>From my understanding the idea is that we try to dump as much of the
unknown playload as possible. The code before did that with this
complicated logic:
i < ((ssize_t)idstrlen - 1) && i < len
I decided to just fill the buffer until strlcat() complains and exit the
loop then.
--
:wq Claudio
Index: ikev2.c
===================================================================
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.370
diff -u -p -r1.370 ikev2.c
--- ikev2.c 13 Jun 2023 12:34:12 -0000 1.370
+++ ikev2.c 14 Jun 2023 12:28:24 -0000
@@ -2285,7 +2285,7 @@ ikev2_nat_detection(struct iked *env, st
struct sockaddr_in *in4;
struct sockaddr_in6 *in6;
ssize_t ret = -1;
- struct sockaddr *src, *dst, *ss;
+ struct sockaddr_storage *src, *dst, *ss;
uint64_t rspi, ispi;
struct ibuf *buf;
uint32_t rnd;
@@ -2299,13 +2299,13 @@ ikev2_nat_detection(struct iked *env, st
return (-1);
ispi = hdr->ike_ispi;
rspi = hdr->ike_rspi;
- src = (struct sockaddr *)&msg->msg_peer;
- dst = (struct sockaddr *)&msg->msg_local;
+ src = &msg->msg_peer;
+ dst = &msg->msg_local;
} else {
ispi = htobe64(sa->sa_hdr.sh_ispi);
rspi = htobe64(sa->sa_hdr.sh_rspi);
- src = (struct sockaddr *)&msg->msg_local;
- dst = (struct sockaddr *)&msg->msg_peer;
+ src = &msg->msg_local;
+ dst = &msg->msg_peer;
}
ctx = EVP_MD_CTX_new();
@@ -2337,7 +2337,7 @@ ikev2_nat_detection(struct iked *env, st
EVP_DigestUpdate(ctx, &ispi, sizeof(ispi));
EVP_DigestUpdate(ctx, &rspi, sizeof(rspi));
- switch (ss->sa_family) {
+ switch (ss->ss_family) {
case AF_INET:
in4 = (struct sockaddr_in *)ss;
EVP_DigestUpdate(ctx, &in4->sin_addr.s_addr,
@@ -6902,15 +6902,14 @@ ikev2_print_static_id(struct iked_static
int
ikev2_print_id(struct iked_id *id, char *idstr, size_t idstrlen)
{
- uint8_t buf[BUFSIZ], *ptr;
- struct sockaddr_in *s4;
- struct sockaddr_in6 *s6;
+ uint8_t *ptr;
+ struct sockaddr_in s4 = { 0 };
+ struct sockaddr_in6 s6 = { 0 };
char *str;
ssize_t len;
int i;
const char *type;
- bzero(buf, sizeof(buf));
bzero(idstr, idstrlen);
if (id->id_buf == NULL)
@@ -6931,48 +6930,38 @@ ikev2_print_id(struct iked_id *id, char
strlcat(idstr, "/", idstrlen) >= idstrlen)
return (-1);
- idstrlen -= strlen(idstr);
- idstr += strlen(idstr);
-
switch (id->id_type) {
case IKEV2_ID_IPV4:
- s4 = (struct sockaddr_in *)buf;
- s4->sin_family = AF_INET;
- s4->sin_len = sizeof(*s4);
- memcpy(&s4->sin_addr.s_addr, ptr, len);
+ s4.sin_family = AF_INET;
+ s4.sin_len = sizeof(s4);
+ memcpy(&s4.sin_addr.s_addr, ptr, len);
- if (print_host((struct sockaddr *)s4,
- idstr, idstrlen) == NULL)
+ if (strlcat(idstr, print_addr(&s4), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_FQDN:
case IKEV2_ID_UFQDN:
- if (len >= (ssize_t)sizeof(buf))
- return (-1);
-
if ((str = get_string(ptr, len)) == NULL)
return (-1);
- if (strlcpy(idstr, str, idstrlen) >= idstrlen) {
+ if (strlcat(idstr, str, idstrlen) >= idstrlen) {
free(str);
return (-1);
}
free(str);
break;
case IKEV2_ID_IPV6:
- s6 = (struct sockaddr_in6 *)buf;
- s6->sin6_family = AF_INET6;
- s6->sin6_len = sizeof(*s6);
- memcpy(&s6->sin6_addr, ptr, len);
+ s6.sin6_family = AF_INET6;
+ s6.sin6_len = sizeof(s6);
+ memcpy(&s6.sin6_addr, ptr, len);
- if (print_host((struct sockaddr *)s6,
- idstr, idstrlen) == NULL)
+ if (strlcat(idstr, print_addr(&s6), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_ASN1_DN:
if ((str = ca_asn1_name(ptr, len)) == NULL)
return (-1);
- if (strlcpy(idstr, str, idstrlen) >= idstrlen) {
+ if (strlcat(idstr, str, idstrlen) >= idstrlen) {
OPENSSL_free(str);
return (-1);
}
@@ -6980,9 +6969,12 @@ ikev2_print_id(struct iked_id *id, char
break;
default:
/* XXX test */
- for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
- snprintf(idstr + i, idstrlen - i,
- "%02x", ptr[i]);
+ for (i = 0; i < len; i++) {
+ char buf[3];
+ snprintf(buf, sizeof(buf), "%02x", ptr[i]);
+ if (strlcat(idstr, buf, idstrlen) >= idstrlen)
+ break;
+ }
break;
}
Index: ikev2_pld.c
===================================================================
RCS file: /cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.129
diff -u -p -r1.129 ikev2_pld.c
--- ikev2_pld.c 6 Jun 2023 16:09:35 -0000 1.129
+++ ikev2_pld.c 14 Jun 2023 09:23:02 -0000
@@ -1522,9 +1522,8 @@ int
ikev2_pld_ts(struct iked *env, struct ikev2_payload *pld,
struct iked_message *msg, size_t offset, size_t left, unsigned int type)
{
- struct sockaddr_in s4;
- struct sockaddr_in6 s6;
- uint8_t buf[2][128];
+ struct sockaddr_in start4, end4;
+ struct sockaddr_in6 start6, end6;
uint8_t *msgbuf = ibuf_data(msg->msg_data);
uint8_t *ptr;
@@ -1539,22 +1538,21 @@ ikev2_pld_ts(struct iked *env, struct ik
return (-1);
}
- bzero(&s4, sizeof(s4));
- s4.sin_family = AF_INET;
- s4.sin_len = sizeof(s4);
- memcpy(&s4.sin_addr.s_addr, ptr, 4);
+ bzero(&start4, sizeof(start4));
+ start4.sin_family = AF_INET;
+ start4.sin_len = sizeof(start4);
+ memcpy(&start4.sin_addr.s_addr, ptr, 4);
ptr += 4;
left -= 4;
- print_host((struct sockaddr *)&s4,
- (char *)buf[0], sizeof(buf[0]));
- memcpy(&s4.sin_addr.s_addr, ptr, 4);
+ bzero(&end4, sizeof(end4));
+ end4.sin_family = AF_INET;
+ end4.sin_len = sizeof(end4);
+ memcpy(&end4.sin_addr.s_addr, ptr, 4);
left -= 4;
- print_host((struct sockaddr *)&s4,
- (char *)buf[1], sizeof(buf[1]));
log_debug("%s: start %s end %s", __func__,
- buf[0], buf[1]);
+ print_addr(&start4), print_addr(&end4));
break;
case IKEV2_TS_IPV6_ADDR_RANGE:
if (left < 2 * 16) {
@@ -1563,21 +1561,21 @@ ikev2_pld_ts(struct iked *env, struct ik
__func__, left, 2 * 16);
return (-1);
}
- bzero(&s6, sizeof(s6));
- s6.sin6_family = AF_INET6;
- s6.sin6_len = sizeof(s6);
- memcpy(&s6.sin6_addr, ptr, 16);
+ bzero(&start6, sizeof(start6));
+ start6.sin6_family = AF_INET6;
+ start6.sin6_len = sizeof(start6);
+ memcpy(&start6.sin6_addr, ptr, 16);
ptr += 16;
left -= 16;
- print_host((struct sockaddr *)&s6,
- (char *)buf[0], sizeof(buf[0]));
- memcpy(&s6.sin6_addr, ptr, 16);
+ bzero(&end6, sizeof(end6));
+ end6.sin6_family = AF_INET6;
+ end6.sin6_len = sizeof(end6);
+ memcpy(&end6.sin6_addr, ptr, 16);
left -= 16;
- print_host((struct sockaddr *)&s6,
- (char *)buf[1], sizeof(buf[1]));
+
log_debug("%s: start %s end %s", __func__,
- buf[0], buf[1]);
+ print_addr(&start6), print_addr(&end6));
break;
default:
log_debug("%s: ignoring unknown TS type %u", __func__, type);
@@ -1871,7 +1869,6 @@ ikev2_pld_cp(struct iked *env, struct ik
uint8_t *msgbuf = ibuf_data(msg->msg_data);
uint8_t *ptr;
size_t len;
- uint8_t buf[128];
int cfg_type;
if (ikev2_validate_cp(msg, offset, left, &cp))
@@ -1949,17 +1946,20 @@ ikev2_pld_cp(struct iked *env, struct ik
in4->sin_family = AF_INET;
in4->sin_len = sizeof(*in4);
memcpy(&in4->sin_addr.s_addr, ptr, 4);
- print_host((struct sockaddr *)in4, (char *)buf,
- sizeof(buf));
- log_debug("%s: cfg %s", __func__, buf);
switch(cfg_type) {
case IKEV2_CFG_INTERNAL_IP4_ADDRESS:
msg->msg_parent->msg_cp_addr = addr;
- log_debug("%s: IP4_ADDRESS %s", __func__, buf);
+ log_debug("%s: IP4_ADDRESS %s", __func__,
+ print_addr(&addr->addr));
break;
case IKEV2_CFG_INTERNAL_IP4_DNS:
msg->msg_parent->msg_cp_dns = addr;
- log_debug("%s: IP4_DNS %s", __func__, buf);
+ log_debug("%s: IP4_DNS %s", __func__,
+ print_addr(&addr->addr));
+ break;
+ default:
+ log_debug("%s: cfg %s", __func__,
+ print_addr(&addr->addr));
break;
}
break;
@@ -1999,17 +1999,20 @@ ikev2_pld_cp(struct iked *env, struct ik
in6->sin6_family = AF_INET6;
in6->sin6_len = sizeof(*in6);
memcpy(&in6->sin6_addr, ptr, 16);
- print_host((struct sockaddr *)in6, (char *)buf,
- sizeof(buf));
- log_debug("%s: cfg %s/%d", __func__, buf, ptr[16]);
switch(cfg_type) {
case IKEV2_CFG_INTERNAL_IP6_ADDRESS:
msg->msg_parent->msg_cp_addr6 = addr;
- log_debug("%s: IP6_ADDRESS %s", __func__, buf);
+ log_debug("%s: IP6_ADDRESS %s", __func__,
+ print_addr(&addr->addr));
break;
case IKEV2_CFG_INTERNAL_IP6_DNS:
msg->msg_parent->msg_cp_dns = addr;
- log_debug("%s: IP6_DNS %s", __func__, buf);
+ log_debug("%s: IP6_DNS %s", __func__,
+ print_addr(&addr->addr));
+ break;
+ default:
+ log_debug("%s: cfg %s/%d", __func__,
+ print_addr(&addr->addr), ptr[16]);
break;
}
break;
Index: parse.y
===================================================================
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.142
diff -u -p -r1.142 parse.y
--- parse.y 19 Apr 2023 13:33:37 -0000 1.142
+++ parse.y 13 Jun 2023 13:14:51 -0000
@@ -2880,8 +2880,7 @@ create_ike(char *name, int af, struct ip
if (dstid)
strlcpy(idstr, dstid, sizeof(idstr));
else if (!pol.pol_peer.addr_net)
- print_host((struct sockaddr *)&pol.pol_peer.addr, idstr,
- sizeof(idstr));
+ strlcpy(idstr, print_addr(&pol.pol_peer.addr), sizeof(idstr));
ikeauth = &pol.pol_auth;
switch (ikeauth->auth_method) {
