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
