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