[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/certificate-exchange into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+95634@xxxxxxxxxxxxxxxxxx
- Date: Mon, 12 Mar 2012 21:29:29 +0100
On Thu, Mar 08, 2012 at 06:41:47PM +0100, Diego Biurrun wrote:
> review needs-fixing
This branch still has serious issues, even after the number of fixes
I applied in the last few days. I'll ping the areas that I have
questions about and remain unfixed below.
> 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.
Any volunteers? The original authors maybe? I still don't really
understand the "big picture" of this branch.
> > --- 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?
Please somebody enlighten us.
> > +/**
> > + * 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)
> > +{
> > + 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?
Anybody?
> > + 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.
Is my assessment correct?
> > +/**
> > + * 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.
I'm still mystified about this one.
> > --- 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?
Can the original author shed some light on this?
> > + /* 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);
> > + }
> > + /* 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.
Christoph, you confirmed that the control flow returned an error at that
point in your original implementation. Who changed this? :)
Diego
Other related posts:
