Re: iked, ibuf_length vs ibuf_size

2023-08-03 Thread Theo Buehler
On Thu, Aug 03, 2023 at 03:42:46PM +0200, Claudio Jeker wrote:
> iked has a special version of ibuf_size() called ibuf_length(). In the
> long run I want to remove this special case. The problem is that
> ibuf_length(NULL) returns 0 while ibuf_size() fails.
> Allowing the NULL pointer here results in bad code since it is no longer
> obvious if a buffer is initalised or not.
> 
> So here is a first step on cleaning up this mess. It switches all
> ibuf_length() calls to ibuf_size() where it is obvious that the argument
> is not NULL (e.g. when ibuf_data(buf) is just at the same time).
> Also in some cases the check should actually be if buf == NULL since
> in those cases the buf is later allocated. (ikev2_pld.c and
> ikev2.c::ikev2_sa_responder()).
> 
> Please double check if I did not introduce some error.

I agree that all the buffers you pass to ibuf_size() are non-NULL
either because they were previously checked, freshly allocated or
would lead to a code path where they would already be dereferenced
without your diff.

The conversions to NULL checks seem correct, so

ok tb.

One comment for tobhe and markus below

> -- 
> :wq Claudio
> 
> 
> Index: ca.c
> ===
> RCS file: /cvs/src/sbin/iked/ca.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 ca.c
> --- ca.c  28 Jun 2023 14:10:24 -  1.95
> +++ ca.c  28 Jul 2023 11:29:25 -
> @@ -207,7 +207,7 @@ int
>  ca_certbundle_add(struct ibuf *buf, struct iked_id *id)
>  {
>   uint8_t  type = id->id_type;
> - size_t   len = ibuf_length(id->id_buf);
> + size_t   len = ibuf_size(id->id_buf);
>   void*val = ibuf_data(id->id_buf);
>  
>   if (id == NULL ||

That id == NULL check doesn't make much sense given the above dereferences.



iked, ibuf_length vs ibuf_size

2023-08-03 Thread Claudio Jeker
iked has a special version of ibuf_size() called ibuf_length(). In the
long run I want to remove this special case. The problem is that
ibuf_length(NULL) returns 0 while ibuf_size() fails.
Allowing the NULL pointer here results in bad code since it is no longer
obvious if a buffer is initalised or not.

So here is a first step on cleaning up this mess. It switches all
ibuf_length() calls to ibuf_size() where it is obvious that the argument
is not NULL (e.g. when ibuf_data(buf) is just at the same time).
Also in some cases the check should actually be if buf == NULL since
in those cases the buf is later allocated. (ikev2_pld.c and
ikev2.c::ikev2_sa_responder()).

Please double check if I did not introduce some error.
-- 
:wq Claudio


Index: ca.c
===
RCS file: /cvs/src/sbin/iked/ca.c,v
retrieving revision 1.95
diff -u -p -r1.95 ca.c
--- ca.c28 Jun 2023 14:10:24 -  1.95
+++ ca.c28 Jul 2023 11:29:25 -
@@ -207,7 +207,7 @@ int
 ca_certbundle_add(struct ibuf *buf, struct iked_id *id)
 {
uint8_t  type = id->id_type;
-   size_t   len = ibuf_length(id->id_buf);
+   size_t   len = ibuf_size(id->id_buf);
void*val = ibuf_data(id->id_buf);
 
if (id == NULL ||
@@ -416,16 +416,16 @@ ca_setcert(struct iked *env, struct iked
/* Must send the cert and a valid Id to the ca process */
if (procid == PROC_CERT) {
if (id == NULL || id->id_type == IKEV2_ID_NONE ||
-   ibuf_length(id->id_buf) > IKED_ID_SIZE)
+   ibuf_size(id->id_buf) > IKED_ID_SIZE)
return (-1);
bzero(&idb, sizeof(idb));
 
/* Convert to a static Id */
idb.id_type = id->id_type;
idb.id_offset = id->id_offset;
-   idb.id_length = ibuf_length(id->id_buf);
+   idb.id_length = ibuf_size(id->id_buf);
memcpy(&idb.id_data, ibuf_data(id->id_buf),
-   ibuf_length(id->id_buf));
+   ibuf_size(id->id_buf));
 
iov[iovcnt].iov_base = &idb;
iov[iovcnt].iov_len = sizeof(idb);
@@ -491,13 +491,13 @@ ca_setreq(struct iked *env, struct iked_
if (ikev2_policy2id(localid, &id, 1) != 0)
return (-1);
 
-   if (ibuf_length(id.id_buf) > IKED_ID_SIZE)
+   if (ibuf_size(id.id_buf) > IKED_ID_SIZE)
return (-1);
bzero(&idb, sizeof(idb));
idb.id_type = id.id_type;
idb.id_offset = id.id_offset;
-   idb.id_length = ibuf_length(id.id_buf);
-   memcpy(&idb.id_data, ibuf_data(id.id_buf), ibuf_length(id.id_buf));
+   idb.id_length = ibuf_size(id.id_buf);
+   memcpy(&idb.id_data, ibuf_data(id.id_buf), ibuf_size(id.id_buf));
iov[iovcnt].iov_base = &idb;
iov[iovcnt].iov_len = sizeof(idb);
iovcnt++;
@@ -637,7 +637,7 @@ ca_getcert(struct iked *env, struct imsg
ret = ca_pubkey_serialize(certkey, &key);
if (ret == 0) {
ptr = ibuf_data(key.id_buf);
-   len = ibuf_length(key.id_buf);
+   len = ibuf_size(key.id_buf);
type = key.id_type;
break;
}
@@ -668,7 +668,7 @@ ca_getcert(struct iked *env, struct imsg
ret = ca_validate_pubkey(env, &id, NULL, 0, &key);
if (ret == 0) {
ptr = ibuf_data(key.id_buf);
-   len = ibuf_length(key.id_buf);
+   len = ibuf_size(key.id_buf);
type = key.id_type;
}
break;
@@ -1060,18 +1060,18 @@ ca_reload(struct iked *env)
}
}
 
-   if (ibuf_length(env->sc_certreq)) {
+   if (ibuf_size(env->sc_certreq)) {
env->sc_certreqtype = IKEV2_CERT_X509_CERT;
iov[0].iov_base = &env->sc_certreqtype;
iov[0].iov_len = sizeof(env->sc_certreqtype);
iovcnt++;
iov[1].iov_base = ibuf_data(env->sc_certreq);
-   iov[1].iov_len = ibuf_length(env->sc_certreq);
+   iov[1].iov_len = ibuf_size(env->sc_certreq);
iovcnt++;
 
log_debug("%s: loaded %zu ca certificate%s", __func__,
-   ibuf_length(env->sc_certreq) / SHA_DIGEST_LENGTH,
-   ibuf_length(env->sc_certreq) == SHA_DIGEST_LENGTH ?
+   ibuf_size(env->sc_certreq) / SHA_DIGEST_LENGTH,
+   ibuf_size(env->sc_certreq) == SHA_DIGEST_LENGTH ?
"" : "s");
 
(void)proc_composev(&env->sc_ps, PROC_IKEV2, IMSG_CERTREQ,
@@ -1252,7 +1252,7 @@ ca_cert_local(struct i