review needs-fixing On Fri, Mar 02, 2012 at 06:08:20PM +0000, Diego Biurrun wrote: > Diego Biurrun has proposed merging lp:~hipl-core/hipl/certificate-exchange into lp:hipl. > > --- hipd/cert.c 2012-03-01 14:06:24 +0000 > +++ hipd/cert.c 2012-03-02 18:07:40 +0000 The functions in this file are HUUUUGE - IMO this needs some refactoring. > @@ -682,89 +688,109 @@ > + > + /* Were we sent a timestamp which indicates a requested cert validity? */ > + validity_param = hip_get_param(msg, HIP_PARAM_UINT); > + > + if (validity_param) { > + const uint32_t valid_until_n = *(const uint32_t *) hip_get_param_contents_direct(validity_param); > + const uint32_t valid_until_h = ntohl(valid_until_n); Gah, pointer type punning. > @@ -797,33 +821,67 @@ > + > + X509_get_notBefore(cert)->type = V_ASN1_GENERALIZEDTIME; > + X509_get_notAfter(cert)->type = V_ASN1_GENERALIZEDTIME; > + > + { > + const time_t now = time(NULL); > + time_t starttime = 0; > + time_t endtime = 0; > + time_t *starttime_p = NULL; > + time_t *endtime_p = NULL; The block braces are unnecessary, we allow mixed declarations and statements. > switch (algo) { > case HIP_HI_RSA: > - HIP_IFEL(!EVP_PKEY_assign_RSA(pkey, rsa), -1, > + HIP_IFEL(!EVP_PKEY_set1_RSA(pkey, (RSA *) ha->peer_pub_key), -1, > "Failed to convert RSA to EVP_PKEY\n"); > HIP_IFEL(X509_set_pubkey(cert, pkey) != 1, -1, > "Failed to set public key of the certificate\n"); > break; > case HIP_HI_DSA: > - HIP_IFEL(!EVP_PKEY_assign_DSA(pkey, dsa), -1, > + HIP_IFEL(!EVP_PKEY_set1_DSA(pkey, (DSA *) ha->peer_pub_key), -1, > "Failed to convert DSA to EVP_PKEY\n"); peer_pub_key is a void*, the casts are unnecessary. > @@ -926,7 +984,25 @@ > > + switch (host_id->rdata.algorithm) { > + case HIP_HI_RSA: > + HIP_IFEL(!EVP_PKEY_set1_RSA(sig_key, (RSA *) key), -1, > + "Failed to convert RSA to EVP_PKEY\n"); > + break; > + case HIP_HI_DSA: > + HIP_IFEL(!EVP_PKEY_set1_DSA(sig_key, (DSA *) key), -1, > + "Failed to convert DSA to EVP_PKEY\n"); > + break; same here > --- hipfw/pisa.c 2011-11-25 13:52:20 +0000 > +++ hipfw/cert.c 2012-03-02 18:07:40 +0000 > @@ -25,551 +25,192 @@ > +#include "rule_management.h" > +#include "cert.h" > + > +// runtime configuration > +int use_cert = false; This could be static. > +const char *root_certificate_file = NULL; This is unused. > --- lib/core/builder.c 2012-03-01 14:06:24 +0000 > +++ lib/core/builder.c 2012-03-02 18:07:40 +0000 > @@ -3388,16 +3395,21 @@ > int hip_build_param_cert_x509_ver(struct hip_common *msg, char *der, int len) > { > + int err = -1; > + struct hip_cert_x509_resp subj = { 0 }; > + > + if (len > 0 && (unsigned int) len <= sizeof(subj.der)) { > + hip_set_param_type((struct hip_tlv_common *) &subj, HIP_PARAM_CERT_X509_REQ); > + hip_calc_param_len((struct hip_tlv_common *) &subj, > + sizeof(struct hip_cert_x509_resp) > + - sizeof(struct hip_tlv_common)); > + memcpy(&subj.der, der, len); > + > + const uint32_t der_len_h = len; > + subj.der_len = htonl(der_len_h); > + err = hip_build_param(msg, &subj); > + } > + return err; > } len should just be made unsigned instead of casting and checking. > @@ -3414,16 +3426,22 @@ > int hip_build_param_cert_x509_resp(struct hip_common *msg, char *der, int len) > { > + int err = -1; > + struct hip_cert_x509_resp local = { 0 }; > + > + if (len > 0 && (unsigned int) len <= sizeof(local.der)) { > + hip_set_param_type((struct hip_tlv_common *) &local, > + HIP_PARAM_CERT_X509_RESP); > + hip_calc_param_len((struct hip_tlv_common *) &local, > + sizeof(struct hip_cert_x509_resp) > + - sizeof(struct hip_tlv_common)); > + memcpy(&local.der, der, len); > + > + const uint32_t der_len_h = len; > + local.der_len = htonl(der_len_h); > + err = hip_build_param(msg, &local); > + } > + return err; > } Same - oh wait, this is code duplication at its worst ... > --- lib/core/cert.c 1970-01-01 00:00:00 +0000 > +++ lib/core/cert.c 2012-03-02 18:07:40 +0000 > @@ -0,0 +1,292 @@ > +/** > + * This function encodes the given certificate to its DER encoding for > + * transmission over the wire. The encoded certificate is written to @ *buf. > + * > + * @param cert the certificate to encode > + * @param buf the output is written here > + * > + * @return the length of the encoded data > + * > + * @note: The encoded data is in binary form and may contain embedded zeroes. > + * Functions such as strlen() will not return the correct length of the > + * encoded structure. Therefore length value returned by this function > + * should always be used. > + */ > +int cert_X509_to_DER(X509 *const cert, unsigned char **buf) > +{ > + int len; > + > + if (!cert) { > + HIP_ERROR("Cannot encode NULL-certificate\n"); > + return -1; > + } > + if (!buf) { > + HIP_ERROR("Cannot create output buffer at NULL-pointer.\n"); > + return -1; > + } > + > + *buf = NULL; > + len = i2d_X509(cert, buf); > + > + if (len < 0) { > + HIP_ERROR("Could not DER-encode the given certificate.\n"); > + return -1; > + } > + return len; > +} The *buf = NULL; assignment looks suspicious to me, can somebody explain it? > +X509 *cert_DER_to_X509(const unsigned char *const buf, const int len) > +{ > + const unsigned char *p; > + > + if (!buf) { > + HIP_ERROR("Cannot decode from NULL-buffer\n"); > + return NULL; > + } > + if (len <= 0) { > + HIP_ERROR("Cannot decode certificate of length <= 0\n"); > + return NULL; > + } > + p = buf; > + return d2i_X509(NULL, &p, len); > +} Is there a point in the variable indirection through p? It seems to be to work around a const-related warning, but if d2i_x509 does not expect a const pointer, don't pass one to it through pointer aliasing. This takes away the compiler's ability to check correctness! > +/** > + * Load a X509 certificate from @ file. If @ file contains more > + * than one certificate, the certificate at the top of the file is > + * returned. > + * > + * @param file the file to load the certficate from > + * @param fmt the input format of the certificate > + * > + * @return a X509 certificate on success, NULL on error > + */ > +X509 *cert_load_x509_certificate(const char *const file, > + enum encoding_format fmt) What's the idea behind the "@ file"? This is not correct Doxygen syntax. I'm assuming @p was meant here to refer to the function parameter and show a marked-up version of the parameter name in the generated HTML. > +{ > + FILE *fp = NULL; > + X509 *cert = NULL; > + > + if (!file) { > + HIP_ERROR("Cannot read certificate from NULL-filename.\n"); > + return NULL; > + } > + > + fp = fopen(file, "rb"); > + if (!fp) { > + HIP_ERROR("Could not open file for reading: %s\n", file); > + return NULL; > + } > + > + if (fmt == ENCODING_FORMAT_PEM) { > + cert = PEM_read_X509(fp, NULL, NULL, NULL); > + } else if (fmt == ENCODING_FORMAT_DER) { > + cert = d2i_X509_fp(fp, NULL); > + } else { > + HIP_ERROR("Invalid encoding format %i \n", fmt); > + return NULL; > + } switch/case? Might more formats get added here? > + if (fclose(fp)) { > + HIP_ERROR("Error closing file: %s\n", file); > + X509_free(cert); > + return NULL; > + } else if (!cert) { > + HIP_ERROR("Could not decode certificate from file.\n"); > + return NULL; > + } > + > + return cert; > +} If I read this correctly, fp will not get closed on encountering an invalid encoding format. > +/** > + * Search for hip_cert parameter in @ msg and try to decode the data in > + * the first certificate parameter of a X509 certificate. > + * > + * @param msg the message to extract the certificate from > + * > + * @return the first X509 certificate found in the message on success, > + * NULL on error and if no certificates were found > + */ > +X509 *cert_get_X509_from_msg(const struct hip_common *const msg) > +{ > + const struct hip_cert *param_cert = NULL; > + > + if (!(param_cert = hip_get_param(msg, HIP_PARAM_CERT))) { > + HIP_ERROR("Message contains no certificate.\n"); > + return NULL; > + } > + > + /* The contents of the certificate begin after the header of the > + * hip_cert parameter. */ > + return cert_DER_to_X509((const unsigned char *) (param_cert + 1), > + ntohs(param_cert->length) - > + sizeof(struct hip_cert) + > + sizeof(struct hip_tlv_common)); > +} What is "param_cert + 1" supposed to be? Are you trying to move the pointer past the struct? This is extremely brittle, see the cast and the length calculation that are necessary. > +/** > + * Compare a given public key @ pkey with the public key > + * contained in @ cert. > + * > + * @param cert the X509 certificate > + * @param pkey the public key to match > + * > + * @return 1 if match, 0 otherwise > + */ > +int cert_match_public_key(X509 *cert, const EVP_PKEY *pkey) same comment wrt the @. > +{ > + EVP_PKEY *pkey2 = NULL; > + > + if (!cert) { > + return 0; > + } > + if (!pkey) { > + return 0; > + } These two could be merged. > --- lib/core/conf.c 2012-01-13 16:25:15 +0000 > +++ lib/core/conf.c 2012-03-02 18:07:40 +0000 > @@ -2424,6 +2435,122 @@ > > /** > + * Utility function which compactly checks whether a string represents > + * a positive natural number, since scanf() is too lenient. > + * > + * @param string NULL-terminated string to be checked > + * > + * @return 1 if the only characters in the string are digits, > + * optionally with one leading '+' sign; 0 otherwise > + */ > +static int is_pos_natural_number(const char *string) > +{ > + if (string) { > + size_t len; > + > + // allow one leading '+' > + if (string[0] == '+') { > + string++; > + } > + > + if ((len = strlen(string)) > 0) { > + for (size_t i = 0; i < len; i++) { > + if (string[i] < '0' || string[i] > '9') { > + return 0; > + } > + } > + return 1; > + } > + } > + return 0; > +} > + > +static int conf_handle_certificate(struct hip_common *msg, > + int action, > + const char *opt[], > + int optc, > + UNUSED int send_only) > +{ > + hip_hit_t subject_hit; > + uint32_t valid_until_h = 0, valid_until_n = 0; > + int scanf_res = 0, err = 0; > + X509 *cert = NULL; > + const struct hip_cert_x509_resp *p = NULL; > + > + > + /* convert the parameter strings input by the user into the > + * appropriate binary formats for further processing */ > + const int pton_res = inet_pton(AF_INET6, opt[0], &subject_hit); > + > + if (optc == 2 && is_pos_natural_number(opt[1])) { OK, what exactly is the problem here? Why do you need the utility function? > + /* need to use sscanf() here: strtol() won't do since we want the > + * result of the conversion to fit into an uint32_t, whereas > + * strtol() and relatives would write to a variable of platform- > + * dependent size */ > + scanf_res = sscanf(opt[1], "%"SCNu32, &valid_until_h); > + } Compare the value to UINT32_MAX and raise an error alternatively? Probably not important ... > + /* The second parameter after "acquire certificate" is optional, so we > + * accept optc == 1 as well as optc == 2 (the latter is implied if > + * scanf_res is 1). */ > + if (pton_res == 1 && action == ACTION_ACQUIRE && > + (optc == 1 || scanf_res == 1)) { > + HIP_DEBUG_HIT("Requesting certificate for subject", &subject_hit); > + > + if (hip_build_user_hdr(msg, HIP_MSG_CERT_X509V3_SIGN, 0)) { > + HIP_ERROR("Failed to build user message header.\n"); > + return -1; > + } > + > + /* If the user provides a second parameter (following the HIT), it is > + * meant to indicate the point in time when validity of the issued > + * certificate ends; expressed in "number of seconds since the epoch"; > + * we use a uint32_t for this, so we're creating a year-2106-problem > + * here (be scared). */ > + if (scanf_res == 1) { > + valid_until_n = htonl(valid_until_h); > + if (hip_build_param_contents(msg, &valid_until_n, > + HIP_PARAM_UINT, sizeof(uint32_t))) { > + HIP_ERROR("Failed to build validity parameter.\n"); > + return -1; > + } > + } > + > + if (hip_build_param_cert_x509_req(msg, &subject_hit)) { > + HIP_ERROR("Failed to build cert_x509_req parameter.\n"); > + return -1; > + } > + } else { > + err = -1; > + HIP_ERROR("Invalid arguments.\n"); > + } > + > + /* Send the message to the daemon. The daemon fills the > + * message. */ > + if (hip_send_recv_daemon_info(msg, send_only, 0)) { > + return -ECOMM; > + } > + > + /* Handle the answer */ > + if (!(p = hip_get_param(msg, HIP_PARAM_CERT_X509_RESP))) { > + HIP_ERROR("No name x509 struct found\n"); > + return -1; > + } > + const uint32_t der_len_h = ntohl(p->der_len); > + if (!(cert = cert_DER_to_X509(p->der, der_len_h))) { > + HIP_ERROR("Could not get X509 certificate from DER encoding\n."); > + return -1; > + } > + if (fwrite(p->der, der_len_h, 1, stdout) != 1) { > + HIP_ERROR("Couldn't output certificate.\n"); > + return -1; > + } > + > + return err; > +} Continuing here after noticing that the arguments are invalid looks wrong. > --- test/lib/core/cert.c 1970-01-01 00:00:00 +0000 > +++ test/lib/core/cert.c 2012-03-02 18:07:40 +0000 > @@ -0,0 +1,189 @@ > + > +START_TEST(test_cert_DER_encoding) > +{ > + int len = 0; > + X509 *cert = NULL; > + X509 *cert_decoded = NULL; > + unsigned char *buf = NULL; > + > + HIP_DEBUG("Test DER en/decoding of X509 certificates.\n"); > + > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT, > + ENCODING_FORMAT_PEM)) != NULL, > + NULL); > + fail_unless((len = cert_X509_to_DER(cert, &buf)) > 0, NULL); > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len)) != NULL, NULL); > + fail_unless(X509_cmp(cert, cert_decoded) == 0, NULL); > + > + fail_unless((len = cert_X509_to_DER(NULL, &buf)) < 0, NULL); > + fail_unless((len = cert_X509_to_DER(cert, NULL)) < 0, NULL); > + fail_unless((len = cert_X509_to_DER(NULL, NULL)) < 0, NULL); > + > + fail_unless((cert_decoded = cert_DER_to_X509(NULL, len)) == NULL, NULL); > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len - 1)) == NULL, NULL); > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len + 1)) == NULL, NULL); > + fail_unless((cert_decoded = cert_DER_to_X509(buf, 0)) == NULL, NULL); > + > + X509_free(cert); > + X509_free(cert_decoded); buf does not need to be freed? > +START_TEST(test_cert_match_public_key) > +{ > + int err = 0; > + RSA *rsa = NULL; > + X509 *cert = NULL; > + EVP_PKEY *pkey = NULL; > + > + HIP_DEBUG("Test matching of public keys.\n"); > + > + fail_unless((err = load_rsa_private_key(TEST_KEY, &rsa)) == 0, NULL); > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT, > + ENCODING_FORMAT_PEM)) != NULL, > + NULL); > + pkey = EVP_PKEY_new(); > + EVP_PKEY_assign_RSA(pkey, rsa); > + fail_unless((err = cert_match_public_key(cert, pkey)) == 1, NULL); > + > + fail_unless((err = cert_match_public_key(NULL, pkey)) == 0, NULL); > + fail_unless((err = cert_match_public_key(cert, NULL)) == 0, NULL); > + fail_unless((err = cert_match_public_key(NULL, NULL)) == 0, NULL); > + > + EVP_PKEY_free(pkey); > + X509_free(cert); RSA_free(rsa); .. at least if the doxy of load_rsa_private_key() is to be believed. > +START_TEST(test_cert_verify_chain) > +{ > + int err = 0; > + X509 *cert = NULL; > + STACK_OF(X509) * chain = NULL; > + > + HIP_DEBUG("Test verification of certificate chains.\n"); > + > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT, > + ENCODING_FORMAT_PEM)) != NULL, > + NULL); > + fail_unless((chain = sk_X509_new_null()) != NULL, NULL); > + sk_X509_push(chain, cert); > + fail_unless((err = cert_verify_chain(cert, NULL, chain, NULL)) == 0, NULL); > + > + fail_unless((err = cert_verify_chain(NULL, NULL, chain, NULL)) != 0, NULL); > + fail_unless((err = cert_verify_chain(cert, NULL, NULL, NULL)) != 0, NULL); > + > + HIP_DEBUG("Successfully passed test for verification of certificate chains.\n"); > +} I would guess that cert needs to be freed and chain popped or so - OpenSSL documentation is nothing but horrific. > +START_TEST(test_cert_get_X509_from_msg) > +{ > + int len = 0; > + X509 *cert = NULL, *cert2 = NULL; > + struct hip_common *msg = NULL; > + unsigned char *buf = NULL; > + > + HIP_DEBUG("Test certificate extraction functionality.\n"); > + > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT, > + ENCODING_FORMAT_PEM)) != NULL, > + NULL); > + msg = hip_msg_alloc(); Memory allocation can fail. > + hip_build_network_hdr(msg, HIP_UPDATE, 0, &in6addr_any, &in6addr_any); > + fail_unless((len = cert_X509_to_DER(cert, &buf)) > 0, NULL); > + fail_unless(hip_build_param_cert(msg, 0, 1, 1, HIP_CERT_X509V3, buf, len) == 0, NULL); > + fail_unless((cert2 = cert_get_X509_from_msg(msg)) != NULL, NULL); > + fail_unless(X509_cmp(cert, cert2) == 0, NULL); > + > + X509_free(cert); > + X509_free(cert2); What about msg and buf? Diego