diff options
author | Emilia Kasper <emilia@openssl.org> | 2016-02-24 12:59:59 +0100 |
---|---|---|
committer | Emilia Kasper <emilia@openssl.org> | 2016-02-25 15:42:48 +0100 |
commit | 380f18ed5f140e0ae1b68f3ab8f4f7c395658d9e (patch) | |
tree | 83e686e480f176176595a3b2f388be366b774b08 /apps/s_server.c | |
parent | 37529928faa8456e85a9c5ad9255517da8dd0c61 (diff) |
CVE-2016-0798: avoid memory leak in SRP
The SRP user database lookup method SRP_VBASE_get_by_user had confusing
memory management semantics; the returned pointer was sometimes newly
allocated, and sometimes owned by the callee. The calling code has no
way of distinguishing these two cases.
Specifically, SRP servers that configure a secret seed to hide valid
login information are vulnerable to a memory leak: an attacker
connecting with an invalid username can cause a memory leak of around
300 bytes per connection.
Servers that do not configure SRP, or configure SRP but do not configure
a seed are not vulnerable.
In Apache, the seed directive is known as SSLSRPUnknownUserSeed.
To mitigate the memory leak, the seed handling in SRP_VBASE_get_by_user
is now disabled even if the user has configured a seed.
Applications are advised to migrate to SRP_VBASE_get1_by_user. However,
note that OpenSSL makes no strong guarantees about the
indistinguishability of valid and invalid logins. In particular,
computations are currently not carried out in constant time.
Reviewed-by: Rich Salz <rsalz@openssl.org>
Diffstat (limited to 'apps/s_server.c')
-rw-r--r-- | apps/s_server.c | 44 |
1 files changed, 28 insertions, 16 deletions
diff --git a/apps/s_server.c b/apps/s_server.c index 1380628483..6645dc8d8e 100644 --- a/apps/s_server.c +++ b/apps/s_server.c @@ -352,6 +352,8 @@ typedef struct srpsrvparm_st { static int ssl_srp_server_param_cb(SSL *s, int *ad, void *arg) { srpsrvparm *p = (srpsrvparm *) arg; + int ret = SSL3_AL_FATAL; + if (p->login == NULL && p->user == NULL) { p->login = SSL_get_srp_username(s); BIO_printf(bio_err, "SRP username = \"%s\"\n", p->login); @@ -360,21 +362,25 @@ static int ssl_srp_server_param_cb(SSL *s, int *ad, void *arg) if (p->user == NULL) { BIO_printf(bio_err, "User %s doesn't exist\n", p->login); - return SSL3_AL_FATAL; + goto err; } + if (SSL_set_srp_server_param (s, p->user->N, p->user->g, p->user->s, p->user->v, p->user->info) < 0) { *ad = SSL_AD_INTERNAL_ERROR; - return SSL3_AL_FATAL; + goto err; } BIO_printf(bio_err, "SRP parameters set: username = \"%s\" info=\"%s\" \n", p->login, p->user->info); - /* need to check whether there are memory leaks */ + ret = SSL_ERROR_NONE; + +err: + SRP_user_pwd_free(p->user); p->user = NULL; p->login = NULL; - return SSL_ERROR_NONE; + return ret; } #endif @@ -2325,9 +2331,10 @@ static int sv_body(int s, int stype, unsigned char *context) #ifndef OPENSSL_NO_SRP while (SSL_get_error(con, k) == SSL_ERROR_WANT_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP renego during write\n"); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); @@ -2393,9 +2400,10 @@ static int sv_body(int s, int stype, unsigned char *context) #ifndef OPENSSL_NO_SRP while (SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP renego during read\n"); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); @@ -2520,9 +2528,10 @@ static int init_ssl_connection(SSL *con) while (i <= 0 && SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP during accept %s\n", srp_callback_parm.login); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); @@ -2732,9 +2741,10 @@ static int www_body(int s, int stype, unsigned char *context) if (BIO_should_io_special(io) && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP renego during read\n"); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); @@ -3093,9 +3103,10 @@ static int rev_body(int s, int stype, unsigned char *context) if (BIO_should_io_special(io) && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP renego during accept\n"); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); @@ -3121,9 +3132,10 @@ static int rev_body(int s, int stype, unsigned char *context) if (BIO_should_io_special(io) && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { BIO_printf(bio_s_out, "LOOKUP renego during read\n"); + SRP_user_pwd_free(srp_callback_parm.user); srp_callback_parm.user = - SRP_VBASE_get_by_user(srp_callback_parm.vb, - srp_callback_parm.login); + SRP_VBASE_get1_by_user(srp_callback_parm.vb, + srp_callback_parm.login); if (srp_callback_parm.user) BIO_printf(bio_s_out, "LOOKUP done %s\n", srp_callback_parm.user->info); |