[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/certificate-exchange into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+95634@xxxxxxxxxxxxxxxxxx
- Date: Thu, 08 Mar 2012 18:41:47 +0100
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
Other related posts:
