From: Emilia Kasper Date: Wed, 24 Feb 2016 11:59:59 +0000 (+0100) Subject: CVE-2016-0798: avoid memory leak in SRP X-Git-Tag: OpenSSL_1_1_0-pre4~521 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=380f18ed5f140e0ae1b68f3ab8f4f7c395658d9e;p=openssl 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 --- diff --git a/CHANGES b/CHANGES index d8496489be..51e7b08ce2 100644 --- a/CHANGES +++ b/CHANGES @@ -4,6 +4,21 @@ Changes between 1.0.2f and 1.1.0 [xx XXX xxxx] + *) Deprecate SRP_VBASE_get_by_user. + SRP_VBASE_get_by_user had inconsistent memory management behaviour. + In order to fix an unavoidable memory leak (CVE-2016-0798), + SRP_VBASE_get_by_user was changed to ignore the "fake user" SRP + seed, even if the seed is configured. + + Users should use SRP_VBASE_get1_by_user instead. Note that in + SRP_VBASE_get1_by_user, caller must free the returned value. Note + also that even though configuring the SRP seed attempts to hide + invalid usernames by continuing the handshake with fake + credentials, this behaviour is not constant time and no strong + guarantees are made that the handshake is indistinguishable from + that of a valid user. + [Emilia Käsper] + *) Configuration change; it's now possible to build dynamic engines without having to build shared libraries and vice versa. This only applies to the engines in engines/, those in crypto/engine/ 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); diff --git a/crypto/srp/srp_vfy.c b/crypto/srp/srp_vfy.c index 0a9de14011..0d3da7ef17 100644 --- a/crypto/srp/srp_vfy.c +++ b/crypto/srp/srp_vfy.c @@ -184,7 +184,7 @@ static char *t_tob64(char *dst, const unsigned char *src, int size) return olddst; } -static void SRP_user_pwd_free(SRP_user_pwd *user_pwd) +void SRP_user_pwd_free(SRP_user_pwd *user_pwd) { if (user_pwd == NULL) return; @@ -246,6 +246,24 @@ static int SRP_user_pwd_set_sv_BN(SRP_user_pwd *vinfo, BIGNUM *s, BIGNUM *v) return (vinfo->s != NULL && vinfo->v != NULL); } +static SRP_user_pwd *srp_user_pwd_dup(SRP_user_pwd *src) +{ + SRP_user_pwd *ret; + + if (src == NULL) + return NULL; + if ((ret = SRP_user_pwd_new()) == NULL) + return NULL; + + SRP_user_pwd_set_gN(ret, src->g, src->N); + if (!SRP_user_pwd_set_ids(ret, src->id, src->info) + || !SRP_user_pwd_set_sv_BN(ret, BN_dup(src->s), BN_dup(src->v))) { + SRP_user_pwd_free(ret); + return NULL; + } + return ret; +} + SRP_VBASE *SRP_VBASE_new(char *seed_key) { SRP_VBASE *vb = OPENSSL_malloc(sizeof(*vb)); @@ -467,21 +485,51 @@ int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file) } -SRP_user_pwd *SRP_VBASE_get_by_user(SRP_VBASE *vb, char *username) +static SRP_user_pwd *find_user(SRP_VBASE *vb, char *username) { int i; SRP_user_pwd *user; - unsigned char digv[SHA_DIGEST_LENGTH]; - unsigned char digs[SHA_DIGEST_LENGTH]; - EVP_MD_CTX *ctxt = NULL; if (vb == NULL) return NULL; + for (i = 0; i < sk_SRP_user_pwd_num(vb->users_pwd); i++) { user = sk_SRP_user_pwd_value(vb->users_pwd, i); if (strcmp(user->id, username) == 0) return user; } + + return NULL; +} + +/* + * DEPRECATED: use SRP_VBASE_get1_by_user instead. + * This method ignores the configured seed and fails for an unknown user. + * Ownership of the returned pointer is not released to the caller. + * In other words, caller must not free the result. + */ +SRP_user_pwd *SRP_VBASE_get_by_user(SRP_VBASE *vb, char *username) +{ + return find_user(vb, username); +} + +/* + * Ownership of the returned pointer is released to the caller. + * In other words, caller must free the result once done. + */ +SRP_user_pwd *SRP_VBASE_get1_by_user(SRP_VBASE *vb, char *username) +{ + SRP_user_pwd *user; + unsigned char digv[SHA_DIGEST_LENGTH]; + unsigned char digs[SHA_DIGEST_LENGTH]; + EVP_MD_CTX *ctxt = NULL; + + if (vb == NULL) + return NULL; + + if ((user = find_user(vb, username)) != NULL) + return srp_user_pwd_dup(user); + if ((vb->seed_key == NULL) || (vb->default_g == NULL) || (vb->default_N == NULL)) return NULL; diff --git a/include/openssl/srp.h b/include/openssl/srp.h index 83a3293f7c..4111d51827 100644 --- a/include/openssl/srp.h +++ b/include/openssl/srp.h @@ -85,14 +85,19 @@ typedef struct SRP_gN_cache_st { DEFINE_STACK_OF(SRP_gN_cache) typedef struct SRP_user_pwd_st { + /* Owned by us. */ char *id; BIGNUM *s; BIGNUM *v; + /* Not owned by us. */ const BIGNUM *g; const BIGNUM *N; + /* Owned by us. */ char *info; } SRP_user_pwd; +void SRP_user_pwd_free(SRP_user_pwd *user_pwd); + DEFINE_STACK_OF(SRP_user_pwd) typedef struct SRP_VBASE_st { @@ -118,7 +123,12 @@ DEFINE_STACK_OF(SRP_gN) SRP_VBASE *SRP_VBASE_new(char *seed_key); void SRP_VBASE_free(SRP_VBASE *vb); int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file); -SRP_user_pwd *SRP_VBASE_get_by_user(SRP_VBASE *vb, char *username); + +/* This method ignores the configured seed and fails for an unknown user. */ +DEPRECATEDIN_1_1_0(SRP_user_pwd *SRP_VBASE_get_by_user(SRP_VBASE *vb, char *username)) +/* NOTE: unlike in SRP_VBASE_get_by_user, caller owns the returned pointer.*/ +SRP_user_pwd *SRP_VBASE_get1_by_user(SRP_VBASE *vb, char *username); + char *SRP_create_verifier(const char *user, const char *pass, char **salt, char **verifier, const char *N, const char *g); int SRP_create_verifier_BN(const char *user, const char *pass, BIGNUM **salt, diff --git a/util/libeay.num b/util/libeay.num index ca8e9ece68..ad7ad9df85 100755 --- a/util/libeay.num +++ b/util/libeay.num @@ -4073,7 +4073,7 @@ OPENSSL_memcmp 4565 1_1_0 EXIST::FUNCTION: OPENSSL_strncasecmp 4566 1_1_0 EXIST::FUNCTION: OPENSSL_gmtime 4567 1_1_0 EXIST::FUNCTION: OPENSSL_gmtime_adj 4568 1_1_0 EXIST::FUNCTION: -SRP_VBASE_get_by_user 4569 1_1_0 EXIST::FUNCTION:SRP +SRP_VBASE_get_by_user 4569 1_1_0 EXIST::FUNCTION:DEPRECATEDIN_1_1_0,SRP SRP_Calc_server_key 4570 1_1_0 EXIST::FUNCTION:SRP SRP_create_verifier 4571 1_1_0 EXIST::FUNCTION:SRP SRP_create_verifier_BN 4572 1_1_0 EXIST::FUNCTION:SRP @@ -4711,3 +4711,5 @@ OPENSSL_thread_stop 5213 1_1_0 EXIST::FUNCTION: OPENSSL_INIT_new 5215 1_1_0 EXIST::FUNCTION: OPENSSL_INIT_free 5216 1_1_0 EXIST::FUNCTION: OPENSSL_INIT_set_config_filename 5217 1_1_0 EXIST::FUNCTION: +SRP_user_pwd_free 5218 1_1_0 EXIST::FUNCTION:SRP +SRP_VBASE_get1_by_user 5219 1_1_0 EXIST::FUNCTION:SRP