On Sat, Sep 13, 2014 at 04:13, Joel Sing wrote:
>
> 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...
I think there are some interface problems here.
Let's look at what httpd is doing.
ressl_config_set_key_mem(srv->srv_ressl_config,
srv->srv_conf.ssl_key, srv->srv_conf.ssl_key_len);
if (ressl_configure(srv->srv_ressl_ctx, srv->srv_ressl_config) != 0) {
log_warn("%s: failed to configure SSL - %s", __func__,
ressl_error(srv->srv_ressl_ctx));
return (-1);
}
/* We're now done with the public/private key... */
explicit_bzero(srv->srv_conf.ssl_cert, srv->srv_conf.ssl_cert_len);
explicit_bzero(srv->srv_conf.ssl_key, srv->srv_conf.ssl_key_len);
free(srv->srv_conf.ssl_cert);
free(srv->srv_conf.ssl_key);
srv->srv_conf.ssl_cert = NULL;
srv->srv_conf.ssl_key = NULL;
This is dangerous. ressl_config now has a dangling pointer to the
freed ssl_key. The config object's lifetime extends beyond that of
this function. Not a problem in this instance, but it's easy to
imagine somebody creating a second ressl ctx with the same config and
the resulting fireworks.
If we're going to keep the interface as is, we would need to document
how long the caller needs to keep the key memory alive and for what
operations. I think that's a tangled mess I'd rather avoid by making
everything "pass by value".
Right now it's not clear if a config object needs to outlive an
associated context or not. Something else to clarify.
Back to your original point, if the caller desires security, it's easy
to clear the memory by setting the key to NULL. (I need to add another
bzero in that case.)
