On Fri, 12 Sep 2014, Ted Unangst wrote:
> On Wed, Sep 10, 2014 at 16:38, Ted Unangst wrote:
> > On Fri, Aug 15, 2014 at 13:06, Ted Unangst wrote:
> >> Instead, ressl should copy all parameters as necessary and
> >> free them. This does introduce an error case into formerly void
> >> functions, but I think that's ok. The alternative would be to use
> >> fixed arrays inside ressl_config, but that seems much worse.
> >
> > Here's a complete diff.

Initial comments inline...

> > (I think we should also zero the key memory if not null, but that's not
> > included in this change.)
>
> Kent Spillner noticed I hadn't cleaned up the references to default
> config in ressl.c. Here's a fixed diff, that also zeroes key memory.
>
> Index: ressl.c
> ===================================================================
> RCS file: /cvs/src/lib/libressl/ressl.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 ressl.c
> --- ressl.c   15 Aug 2014 16:55:32 -0000      1.12
> +++ ressl.c   11 Sep 2014 19:18:49 -0000
> @@ -29,7 +29,7 @@
>  #include <ressl.h>
>  #include "ressl_internal.h"
>
> -extern struct ressl_config ressl_config_default;
> +static struct ressl_config *ressl_config_default;
>
>  int
>  ressl_init(void)
> @@ -42,6 +42,9 @@ ressl_init(void)
>       SSL_load_error_strings();
>       SSL_library_init();
>
> +     if ((ressl_config_default = ressl_config_new()) == NULL)
> +             return (-1);
> +
>       ressl_initialised = 1;
>
>       return (0);
> @@ -78,7 +81,7 @@ ressl_new(void)
>       if ((ctx = calloc(1, sizeof(*ctx))) == NULL)
>               return (NULL);
>
> -     ctx->config = &ressl_config_default;
> +     ctx->config = ressl_config_default;
>
>       ressl_reset(ctx);
>
> @@ -89,7 +92,7 @@ int
>  ressl_configure(struct ressl *ctx, struct ressl_config *config)
>  {
>       if (config == NULL)
> -             config = &ressl_config_default;
> +             config = ressl_config_default;
>
>       ctx->config = config;
>
> Index: ressl.h
> ===================================================================
> RCS file: /cvs/src/lib/libressl/ressl.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 ressl.h
> --- ressl.h   27 Aug 2014 10:46:53 -0000      1.13
> +++ ressl.h   10 Sep 2014 20:23:46 -0000
> @@ -31,15 +31,15 @@ const char *ressl_error(struct ressl *ct
>  struct ressl_config *ressl_config_new(void);
>  void ressl_config_free(struct ressl_config *config);
>
> -void ressl_config_set_ca_file(struct ressl_config *config, char *ca_file);
> -void ressl_config_set_ca_path(struct ressl_config *config, char *ca_path);
> -void ressl_config_set_cert_file(struct ressl_config *config, char
> *cert_file); -void ressl_config_set_cert_mem(struct ressl_config *config,
> char *cert, +int ressl_config_set_ca_file(struct ressl_config *config, char
> *ca_file); +int ressl_config_set_ca_path(struct ressl_config *config, char
> *ca_path); +int ressl_config_set_cert_file(struct ressl_config *config,
> char *cert_file); +int ressl_config_set_cert_mem(struct ressl_config
> *config, char *cert, size_t len);
> -void ressl_config_set_ciphers(struct ressl_config *config, char *ciphers);
> +int ressl_config_set_ciphers(struct ressl_config *config, char *ciphers);
>  int ressl_config_set_ecdhcurve(struct ressl_config *config, const char *);
> -void ressl_config_set_key_file(struct ressl_config *config, char
> *key_file); -void ressl_config_set_key_mem(struct ressl_config *config,
> char *key, +int ressl_config_set_key_file(struct ressl_config *config, char
> *key_file); +int ressl_config_set_key_mem(struct ressl_config *config, char
> *key, size_t len);
>  void ressl_config_set_verify_depth(struct ressl_config *config,
>      int verify_depth);
> Index: ressl_config.c
> ===================================================================
> RCS file: /cvs/src/lib/libressl/ressl_config.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 ressl_config.c
> --- ressl_config.c    27 Aug 2014 10:46:53 -0000      1.8
> +++ ressl_config.c    11 Sep 2014 19:14:27 -0000
> @@ -21,27 +21,55 @@
>  #include <ressl.h>
>  #include "ressl_internal.h"
>
> -/*
> - * Default configuration.
> - */
> -struct ressl_config ressl_config_default = {
> -     .ca_file = _PATH_SSL_CA_FILE,
> -     .ca_path = NULL,
> -     .ciphers = NULL,
> -     .ecdhcurve = NID_X9_62_prime256v1,
> -     .verify = 1,
> -     .verify_depth = 6,
> -};
> +#define SET_STRING(config, name, val) do {   \
> +     free(config->name);                     \
> +     config->name = NULL;                    \
> +     if (val != NULL) {                      \
> +             if ((config->name = strdup(val)) == NULL)       \
> +                     return -1;              \
> +     }                                       \
> +} while (0)
> +
> +#define SET_MEM(config, name, namelen, val, vallen) do {     \
> +     free(config->name);                     \
> +     config->name = NULL;                    \
> +     if (val != NULL) {                      \
> +             if ((config->name = memdup(val, vallen)) == NULL)       \
> +                     return -1;              \
> +             config->namelen = vallen;       \
> +     }                                       \
> +} while (0)

Yikes... macros with embedded returns... 

/me wonders if tedu has spent too much time in crypto/asn1!

Can we at least make these functions (inline if you prefer) - I'll shout the 
additional lines of code to check the return values... also, I think a copy 
or dup name would be more indicative of what it is doing, rather than set.

> +static void *
> +memdup(const void *in, size_t len)
> +{
> +     void *out;
> +
> +     if ((out = malloc(len)) == NULL)
> +             return NULL;
> +     memcpy(out, in, len);
> +     return out;
> +}
>
>  struct ressl_config *
>  ressl_config_new(void)
>  {
>       struct ressl_config *config;
>
> -     if ((config = malloc(sizeof(*config))) == NULL)
> +     if ((config = calloc(1, sizeof(*config))) == NULL)
>               return (NULL);
>
> -     memcpy(config, &ressl_config_default, sizeof(*config));
> +     /*
> +      * Default configuration.
> +      */
> +     if (ressl_config_set_ca_file(config, _PATH_SSL_CA_FILE) != 0) {
> +             ressl_config_free(config);
> +             return (NULL);
> +     }
> +     ressl_config_verify(config);
> +     ressl_config_set_verify_depth(config, 6);
> +     /* ? use function ? */
> +     config->ecdhcurve = NID_X9_62_prime256v1;
>
>       return (config);
>  }
> @@ -49,38 +77,54 @@ ressl_config_new(void)
>  void
>  ressl_config_free(struct ressl_config *config)
>  {
> +     if (config == NULL)
> +             return;
> +     free(config->ca_file);
> +     free(config->ca_path);
> +     free(config->cert_file);
> +     free(config->cert_mem);
> +     free(config->ciphers);
> +     free(config->key_file);
> +     if (config->key_mem != NULL) {
> +             explicit_bzero(config->key_mem, config->key_len);
> +             free(config->key_mem);
> +     }
>       free(config);
>  }
>
> -void
> +int
>  ressl_config_set_ca_file(struct ressl_config *config, char *ca_file)
>  {
> -     config->ca_file = ca_file;
> +     SET_STRING(config, ca_file, ca_file);
> +     return 0;
>  }
>
> -void
> +int
>  ressl_config_set_ca_path(struct ressl_config *config, char *ca_path)
>  {
> -     config->ca_path = ca_path;
> +     SET_STRING(config, ca_path, ca_path);
> +     return 0;
>  }
>
> -void
> +int
>  ressl_config_set_cert_file(struct ressl_config *config, char *cert_file)
>  {
> -     config->cert_file = cert_file;
> +     SET_STRING(config, cert_file, cert_file);
> +     return 0;
>  }
>
> -void
> +int
>  ressl_config_set_cert_mem(struct ressl_config *config, char *cert, size_t
> len) {
> -     config->cert_mem = cert;
> -     config->cert_len = len;
> +     SET_MEM(config, cert_mem, cert_len, cert, len);
> +     return 0;
>  }
>
> -void
> +int
>  ressl_config_set_ciphers(struct ressl_config *config, char *ciphers)
>  {
> -     config->ciphers = ciphers;
> +     SET_STRING(config, ciphers, ciphers);
> +     return 0;
>  }
>
>  int
> @@ -95,17 +139,18 @@ ressl_config_set_ecdhcurve(struct ressl_
>       return (0);
>  }
>
> -void
> +int
>  ressl_config_set_key_file(struct ressl_config *config, char *key_file)
>  {
> -     config->key_file = key_file;
> +     SET_STRING(config, key_file, key_file);
> +     return 0;
>  }
>
> -void
> +int
>  ressl_config_set_key_mem(struct ressl_config *config, char *key, size_t
> len) {
> -     config->key_mem = key;
> -     config->key_len = len;
> +     SET_MEM(config, key_mem, key_len, key, len);
> +     return 0;
>  }

I'm not convinced that we should be doing this with the *_mem() functions, as 
there is a benefit to the client owning this memory. Currently, in httpd the 
public/private key is removed as soon as ressl_configure() is called. 
Obviously libssl still has a copy squirrelled away in memory, however with 
this change we now have two copies floating around.

I guess the other option would be for ressl_configure() to clean up the keys 
as soon as it is done processing, however that then means that the caller 
would have to call the *_mem() functions again if it was going to call 
ressl_configure() again...

>  void
> Index: ressl_internal.h
> ===================================================================
> RCS file: /cvs/src/lib/libressl/ressl_internal.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 ressl_internal.h
> --- ressl_internal.h  27 Aug 2014 10:46:53 -0000      1.10
> +++ ressl_internal.h  10 Sep 2014 20:27:51 -0000
> @@ -26,14 +26,14 @@
>  #define _PATH_SSL_CA_FILE "/etc/ssl/cert.pem"
>
>  struct ressl_config {
> -     const char *ca_file;
> -     const char *ca_path;
> -     const char *cert_file;
> +     char *ca_file;
> +     char *ca_path;
> +     char *cert_file;
>       char *cert_mem;
>       size_t cert_len;
> -     const char *ciphers;
> +     char *ciphers;
>       int ecdhcurve;
> -     const char *key_file;
> +     char *key_file;
>       char *key_mem;
>       size_t key_len;
>       int verify;



-- 

    "Action without study is fatal. Study without action is futile."
        -- Mary Ritter Beard

Reply via email to