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.c 28 Jun 2023 14:10:24 -0000 1.95 +++ ca.c 28 Jul 2023 11:29:25 -0000 @@ -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 iked *env, X509 *c int ret = 0; if ((localpub = ca_bytes_to_pkey(ibuf_data(store->ca_pubkey.id_buf), - ibuf_length(store->ca_pubkey.id_buf))) == NULL) + ibuf_size(store->ca_pubkey.id_buf))) == NULL) goto done; if ((certkey = X509_get0_pubkey(cert)) == NULL) { @@ -1579,7 +1579,7 @@ ca_privkey_to_method(struct iked_id *pri break; case IKEV2_CERT_ECDSA: if ((rawcert = BIO_new_mem_buf(ibuf_data(privkey->id_buf), - ibuf_length(privkey->id_buf))) == NULL) + ibuf_size(privkey->id_buf))) == NULL) goto out; if ((ec = d2i_ECPrivateKey_bio(rawcert, NULL)) == NULL) goto out; Index: config.c =================================================================== RCS file: /cvs/src/sbin/iked/config.c,v retrieving revision 1.92 diff -u -p -r1.92 config.c --- config.c 23 May 2023 13:12:19 -0000 1.92 +++ config.c 28 Jul 2023 11:30:16 -0000 @@ -1042,7 +1042,7 @@ config_setkeys(struct iked *env) iov[0].iov_base = &privkey; iov[0].iov_len = sizeof(privkey); iov[1].iov_base = ibuf_data(privkey.id_buf); - iov[1].iov_len = ibuf_length(privkey.id_buf); + iov[1].iov_len = ibuf_size(privkey.id_buf); if (proc_composev(&env->sc_ps, PROC_CERT, IMSG_PRIVKEY, iov, 2) == -1) { log_warnx("%s: failed to send private key", __func__); @@ -1052,7 +1052,7 @@ config_setkeys(struct iked *env) iov[0].iov_base = &pubkey; iov[0].iov_len = sizeof(pubkey); iov[1].iov_base = ibuf_data(pubkey.id_buf); - iov[1].iov_len = ibuf_length(pubkey.id_buf); + iov[1].iov_len = ibuf_size(pubkey.id_buf); if (proc_composev(&env->sc_ps, PROC_CERT, IMSG_PUBKEY, iov, 2) == -1) { log_warnx("%s: failed to send public key", __func__); Index: crypto.c =================================================================== RCS file: /cvs/src/sbin/iked/crypto.c,v retrieving revision 1.45 diff -u -p -r1.45 crypto.c --- crypto.c 28 Jul 2023 07:31:38 -0000 1.45 +++ crypto.c 28 Jul 2023 11:31:44 -0000 @@ -328,7 +328,7 @@ void hash_init(struct iked_hash *hash) { HMAC_Init_ex(hash->hash_ctx, ibuf_data(hash->hash_key), - ibuf_length(hash->hash_key), hash->hash_priv, NULL); + ibuf_size(hash->hash_key), hash->hash_priv, NULL); } void @@ -923,7 +923,7 @@ dsa_init(struct iked_dsa *dsa, const voi if (dsa->dsa_hmac) { if (!HMAC_Init_ex(dsa->dsa_ctx, ibuf_data(dsa->dsa_keydata), - ibuf_length(dsa->dsa_keydata), dsa->dsa_priv, NULL)) + ibuf_size(dsa->dsa_keydata), dsa->dsa_priv, NULL)) return (-1); return (0); } Index: ikev2.c =================================================================== RCS file: /cvs/src/sbin/iked/ikev2.c,v retrieving revision 1.376 diff -u -p -r1.376 ikev2.c --- ikev2.c 28 Jul 2023 11:23:03 -0000 1.376 +++ ikev2.c 28 Jul 2023 12:30:37 -0000 @@ -671,7 +671,7 @@ ikev2_recv(struct iked *env, struct iked msg->msg_msgid, print_addr(&msg->msg_peer), print_addr(&msg->msg_local), - ibuf_length(msg->msg_data), + ibuf_size(msg->msg_data), msg->msg_policy->pol_name); log_debug("%s: ispi %s rspi %s", __func__, print_spi(betoh64(hdr->ike_ispi), 8), @@ -733,9 +733,9 @@ ikev2_recv(struct iked *env, struct iked if (sa->sa_state == IKEV2_STATE_CLOSED && sa->sa_1stmsg && hdr->ike_exchange == IKEV2_EXCHANGE_IKE_SA_INIT && msg->msg_msgid == 0 && - (ibuf_length(msg->msg_data) != ibuf_length(sa->sa_1stmsg) || + (ibuf_size(msg->msg_data) != ibuf_size(sa->sa_1stmsg) || memcmp(ibuf_data(msg->msg_data), ibuf_data(sa->sa_1stmsg), - ibuf_length(sa->sa_1stmsg)) != 0)) { + ibuf_size(sa->sa_1stmsg)) != 0)) { ikev2_ike_sa_setreason(sa, NULL); sa_free(env, sa); msg->msg_sa = sa = NULL; @@ -897,7 +897,7 @@ ikev2_auth_verify(struct iked *env, stru ret = ikev2_msg_authverify(env, sa, &ikeauth, ibuf_data(sa->sa_peerauth.id_buf), - ibuf_length(sa->sa_peerauth.id_buf), + ibuf_size(sa->sa_peerauth.id_buf), authmsg); ibuf_free(authmsg); if (ret != 0) { @@ -1115,7 +1115,7 @@ ikev2_ike_auth_recv(struct iked *env, st if (msg->msg_cert.id_type) { certtype = msg->msg_cert.id_type; cert = ibuf_data(msg->msg_cert.id_buf); - certlen = ibuf_length(msg->msg_cert.id_buf); + certlen = ibuf_size(msg->msg_cert.id_buf); } sa->sa_stateflags &= ~IKED_REQ_CERTVALID; if (ca_setcert(env, &sa->sa_hdr, id, certtype, cert, certlen, PROC_CERT) == -1) @@ -1471,7 +1471,7 @@ ikev2_init_ike_sa_peer(struct iked *env, ke->kex_dhgroup = htobe16(group->id); if (ikev2_add_buf(buf, sa->sa_dhiexchange) == -1) goto done; - len = sizeof(*ke) + ibuf_length(sa->sa_dhiexchange); + len = sizeof(*ke) + ibuf_size(sa->sa_dhiexchange); if (ikev2_next_payload(pld, len, IKEV2_PAYLOAD_NONCE) == -1) goto done; @@ -2215,7 +2215,7 @@ ikev2_add_vendor_id(struct ibuf *e, stru if (ibuf_add_buf(e, id) == -1) return (-1); - return (ibuf_length(id)); + return (ibuf_size(id)); } ssize_t @@ -3744,7 +3744,7 @@ ikev2_handle_certreq(struct iked* env, s ca_setreq(env, sa, &sa->sa_policy->pol_localid, cr->cr_type, more, ibuf_data(cr->cr_data), - ibuf_length(cr->cr_data), + ibuf_size(cr->cr_data), PROC_CERT); ibuf_free(cr->cr_data); @@ -4210,7 +4210,7 @@ ikev2_send_create_child_sa(struct iked * ke->kex_dhgroup = htobe16(group->id); if (ikev2_add_buf(e, sa->sa_dhiexchange) == -1) goto done; - len = sizeof(*ke) + ibuf_length(sa->sa_dhiexchange); + len = sizeof(*ke) + ibuf_size(sa->sa_dhiexchange); } if ((len = ikev2_add_ts(e, &pld, len, sa, !initiator)) == -1) @@ -4343,7 +4343,7 @@ ikev2_ike_sa_rekey(struct iked *env, voi ke->kex_dhgroup = htobe16(group->id); if (ikev2_add_buf(e, nsa->sa_dhiexchange) == -1) goto done; - len = sizeof(*ke) + ibuf_length(nsa->sa_dhiexchange); + len = sizeof(*ke) + ibuf_size(nsa->sa_dhiexchange); if (ikev2_next_payload(pld, len, IKEV2_PAYLOAD_NONE) == -1) goto done; @@ -4377,8 +4377,8 @@ ikev2_nonce_cmp(struct ibuf *a, struct i size_t alen, blen, len; int ret; - alen = ibuf_length(a); - blen = ibuf_length(b); + alen = ibuf_size(a); + blen = ibuf_size(b); len = MINIMUM(alen, blen); ret = memcmp(ibuf_data(a), ibuf_data(b), len); if (ret == 0) @@ -5078,7 +5078,7 @@ ikev2_resp_create_child_sa(struct iked * ke->kex_dhgroup = htobe16(kex->kex_dhgroup->id); if (ikev2_add_buf(e, kex->kex_dhrexchange) == -1) goto done; - len = sizeof(*ke) + ibuf_length(kex->kex_dhrexchange); + len = sizeof(*ke) + ibuf_size(kex->kex_dhrexchange); } if (protoid != IKEV2_SAPROTO_IKE) @@ -5641,7 +5641,7 @@ ikev2_sa_responder(struct iked *env, str return (-1); } - if (!ibuf_length(sa->sa_rnonce) && + if (sa->sa_rnonce == NULL && (sa->sa_rnonce = ibuf_random(IKED_NONCE_SIZE)) == NULL) { log_debug("%s: failed to get local nonce", __func__); return (-1); @@ -5737,7 +5737,7 @@ ikev2_sa_keys(struct iked *env, struct i } log_debug("%s: DHSECRET with %zu bytes", SPI_SA(sa, __func__), - ibuf_length(dhsecret)); + ibuf_size(dhsecret)); print_hexbuf(dhsecret); if (!key) { @@ -5763,7 +5763,7 @@ ikev2_sa_keys(struct iked *env, struct i } } - if ((hash_setkey(prf, ibuf_data(key), ibuf_length(key))) == NULL) { + if ((hash_setkey(prf, ibuf_data(key), ibuf_size(key))) == NULL) { log_info("%s: failed to set prf key", SPI_SA(sa, __func__)); goto done; } @@ -5776,7 +5776,7 @@ ikev2_sa_keys(struct iked *env, struct i tmplen = 0; hash_init(prf); - hash_update(prf, ibuf_data(dhsecret), ibuf_length(dhsecret)); + hash_update(prf, ibuf_data(dhsecret), ibuf_size(dhsecret)); hash_final(prf, ibuf_data(skeyseed), &tmplen); log_debug("%s: SKEYSEED with %zu bytes", __func__, tmplen); @@ -5809,7 +5809,7 @@ ikev2_sa_keys(struct iked *env, struct i goto done; } - log_debug("%s: S with %zu bytes", SPI_SA(sa, __func__), ibuf_length(s)); + log_debug("%s: S with %zu bytes", SPI_SA(sa, __func__), ibuf_size(s)); print_hexbuf(s); /* @@ -5848,28 +5848,27 @@ ikev2_sa_keys(struct iked *env, struct i goto done; } - log_debug("%s: SK_d with %zu bytes", __func__, - ibuf_length(sa->sa_key_d)); + log_debug("%s: SK_d with %zu bytes", __func__, ibuf_size(sa->sa_key_d)); print_hexbuf(sa->sa_key_d); if (!isaead) { log_debug("%s: SK_ai with %zu bytes", __func__, - ibuf_length(sa->sa_key_iauth)); + ibuf_size(sa->sa_key_iauth)); print_hexbuf(sa->sa_key_iauth); log_debug("%s: SK_ar with %zu bytes", __func__, - ibuf_length(sa->sa_key_rauth)); + ibuf_size(sa->sa_key_rauth)); print_hexbuf(sa->sa_key_rauth); } log_debug("%s: SK_ei with %zu bytes", __func__, - ibuf_length(sa->sa_key_iencr)); + ibuf_size(sa->sa_key_iencr)); print_hexbuf(sa->sa_key_iencr); log_debug("%s: SK_er with %zu bytes", __func__, - ibuf_length(sa->sa_key_rencr)); + ibuf_size(sa->sa_key_rencr)); print_hexbuf(sa->sa_key_rencr); log_debug("%s: SK_pi with %zu bytes", __func__, - ibuf_length(sa->sa_key_iprf)); + ibuf_size(sa->sa_key_iprf)); print_hexbuf(sa->sa_key_iprf); log_debug("%s: SK_pr with %zu bytes", __func__, - ibuf_length(sa->sa_key_rprf)); + ibuf_size(sa->sa_key_rprf)); print_hexbuf(sa->sa_key_rprf); ret = 0; @@ -5928,7 +5927,7 @@ ikev2_prfplus(struct iked_hash *prf, str for (i = 0; i < rlen; i++) { if (t1 != NULL) { - t2 = ibuf_new(ibuf_data(t1), ibuf_length(t1)); + t2 = ibuf_new(ibuf_data(t1), ibuf_size(t1)); ibuf_free(t1); } else t2 = ibuf_new(NULL, 0); @@ -5939,7 +5938,7 @@ ikev2_prfplus(struct iked_hash *prf, str ibuf_add(t2, &pad, 1); hash_init(prf); - hash_update(prf, ibuf_data(t2), ibuf_length(t2)); + hash_update(prf, ibuf_data(t2), ibuf_size(t2)); hash_final(prf, ibuf_data(t1), &hashlen); if (hashlen != hash_length(prf)) @@ -5949,11 +5948,11 @@ ikev2_prfplus(struct iked_hash *prf, str ibuf_add_buf(t, t1); log_debug("%s: T%d with %zu bytes", __func__, - pad, ibuf_length(t1)); + pad, ibuf_size(t1)); print_hexbuf(t1); } - log_debug("%s: Tn with %zu bytes", __func__, ibuf_length(t)); + log_debug("%s: Tn with %zu bytes", __func__, ibuf_size(t)); print_hexbuf(t); ibuf_free(t1); @@ -6179,7 +6178,7 @@ ikev2_childsa_negotiate(struct iked *env if (pfs) { log_debug("%s: using PFS", __func__); if (kex->kex_dhpeer == NULL || - ibuf_length(kex->kex_dhpeer) == 0 || + ibuf_size(kex->kex_dhpeer) == 0 || (group = kex->kex_dhgroup) == NULL) { log_debug("%s: no dh group for pfs", __func__); goto done; @@ -7649,7 +7648,7 @@ ikev2_log_cert_info(const char *msg, str certid->id_buf == NULL) return; if ((rawcert = BIO_new_mem_buf(ibuf_data(certid->id_buf), - ibuf_length(certid->id_buf))) == NULL || + ibuf_size(certid->id_buf))) == NULL || (cert = d2i_X509_bio(rawcert, NULL)) == NULL) goto out; ca_cert_info(msg, cert); Index: ikev2_msg.c =================================================================== RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v retrieving revision 1.99 diff -u -p -r1.99 ikev2_msg.c --- ikev2_msg.c 28 Jul 2023 11:23:03 -0000 1.99 +++ ikev2_msg.c 28 Jul 2023 11:41:23 -0000 @@ -292,7 +292,7 @@ ikev2_msg_send(struct iked *env, struct betoh32(hdr->ike_msgid), print_addr(&msg->msg_peer), print_addr(&msg->msg_local), - ibuf_length(buf), isnatt ? ", NAT-T" : ""); + ibuf_size(buf), isnatt ? ", NAT-T" : ""); if (isnatt) { struct ibuf *new; @@ -448,7 +448,7 @@ ikev2_msg_encrypt(struct iked *env, stru log_debug("%s: padded length %zu", __func__, ibuf_size(src)); print_hexbuf(src); - cipher_setkey(sa->sa_encr, ibuf_data(encr), ibuf_length(encr)); + cipher_setkey(sa->sa_encr, ibuf_data(encr), ibuf_size(encr)); cipher_setiv(sa->sa_encr, NULL, 0); /* XXX ivlen */ if (cipher_init_encrypt(sa->sa_encr) == -1) { log_info("%s: error initiating cipher.", __func__); @@ -466,8 +466,8 @@ ikev2_msg_encrypt(struct iked *env, stru /* Add AAD for AEAD ciphers */ if (sa->sa_integr->hash_isaead) - cipher_aad(sa->sa_encr, ibuf_data(aad), - ibuf_length(aad), &outlen); + cipher_aad(sa->sa_encr, ibuf_data(aad), ibuf_size(aad), + &outlen); if (cipher_update(sa->sa_encr, ibuf_data(src), encrlen, ibuf_data(out), &outlen) == -1) { @@ -620,7 +620,7 @@ ikev2_msg_decrypt(struct iked *env, stru goto done; hash_setkey(sa->sa_integr, ibuf_data(integr), - ibuf_length(integr)); + ibuf_size(integr)); hash_init(sa->sa_integr); hash_update(sa->sa_integr, ibuf_data(msg), ibuf_size(msg) - integrlen); @@ -649,7 +649,7 @@ ikev2_msg_decrypt(struct iked *env, stru goto done; } - cipher_setkey(sa->sa_encr, ibuf_data(encr), ibuf_length(encr)); + cipher_setkey(sa->sa_encr, ibuf_data(encr), ibuf_size(encr)); cipher_setiv(sa->sa_encr, ibuf_seek(src, ivoff, ivlen), ivlen); if (cipher_init_decrypt(sa->sa_encr) == -1) { log_info("%s: error initiating cipher.", __func__); @@ -675,13 +675,14 @@ ikev2_msg_decrypt(struct iked *env, stru * Add additional authenticated data for AEAD ciphers */ if (sa->sa_integr->hash_isaead) { - log_debug("%s: AAD length %zu", __func__, ibuf_length(msg) - ibuf_length(src)); - print_hex(ibuf_data(msg), 0, ibuf_length(msg) - ibuf_length(src)); + log_debug("%s: AAD length %zu", __func__, + ibuf_size(msg) - ibuf_size(src)); + print_hex(ibuf_data(msg), 0, ibuf_size(msg) - ibuf_size(src)); cipher_aad(sa->sa_encr, ibuf_data(msg), - ibuf_length(msg) - ibuf_length(src), &outlen); + ibuf_size(msg) - ibuf_size(src), &outlen); } - if ((outlen = ibuf_length(out)) != 0) { + if ((outlen = ibuf_size(out)) != 0) { if (cipher_update(sa->sa_encr, ibuf_seek(src, encroff, encrlen), encrlen, ibuf_data(out), &outlen) == -1) { log_info("%s: error updating cipher.", __func__); Index: ikev2_pld.c =================================================================== RCS file: /cvs/src/sbin/iked/ikev2_pld.c,v retrieving revision 1.131 diff -u -p -r1.131 ikev2_pld.c --- ikev2_pld.c 28 Jun 2023 14:10:24 -0000 1.131 +++ ikev2_pld.c 28 Jul 2023 11:33:56 -0000 @@ -685,7 +685,7 @@ ikev2_pld_ke(struct iked *env, struct ik print_hex(buf, 0, len); if (ikev2_msg_frompeer(msg)) { - if (ibuf_length(msg->msg_parent->msg_ke)) { + if (msg->msg_parent->msg_ke != NULL) { log_info("%s: duplicate KE payload", __func__); return (-1); } @@ -1008,7 +1008,7 @@ ikev2_pld_nonce(struct iked *env, struct print_hex(buf, 0, len); if (ikev2_msg_frompeer(msg)) { - if (ibuf_length(msg->msg_parent->msg_nonce)) { + if (msg->msg_parent->msg_nonce != NULL) { log_info("%s: duplicate NONCE payload", __func__); return (-1); } @@ -1665,7 +1665,7 @@ ikev2_pld_ef(struct iked *env, struct ik __func__, frag_num, frag_total); goto done; } - elen = ibuf_length(e); + elen = ibuf_size(e); /* Check new fragmented message */ if (sa_frag->frag_arr == NULL) {