generic-library/openssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

CVE-2016-0798: avoid memory leak in SRP

Browse Source 
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>
This commit is contained in:
Emilia Kasper 2016-02-24 12:59:59 +01:00
parent 3ee48ada8c
commit 59a908f1e8
5 changed files with 109 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
CHANGES
View File

@ -4,7 +4,24 @@
Changes between 1.0.1r and 1.0.1s [xx XXX xxxx]
*)
*) Disable SRP fake user seed to address a server memory leak.
Add a new method SRP_VBASE_get1_by_user that handles the seed properly.
SRP_VBASE_get_by_user had inconsistent memory management behaviour.
In order to fix an unavoidable memory leak, 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.
(CVE-2016-0798)
[Emilia Käsper]
Changes between 1.0.1q and 1.0.1r [28 Jan 2016]

39
apps/s_server.c
View File

@ -416,6 +416,8 @@ typedef struct srpsrvparm_st {
static int MS_CALLBACK 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);
@ -424,21 +426,25 @@ static int MS_CALLBACK 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
@ -2244,9 +2250,10 @@ static int sv_body(char *hostname, int s, 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);
@ -2300,9 +2307,10 @@ static int sv_body(char *hostname, int s, 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);
@ -2387,9 +2395,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);
@ -2616,9 +2625,10 @@ static int www_body(char *hostname, int s, unsigned char *context)
&& 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);
@ -2658,9 +2668,10 @@ static int www_body(char *hostname, int s, 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);

10
crypto/srp/srp.h
View File

@ -82,16 +82,21 @@ typedef struct SRP_gN_cache_st {
DECLARE_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;
DECLARE_STACK_OF(SRP_user_pwd)
void SRP_user_pwd_free(SRP_user_pwd *user_pwd);
typedef struct SRP_VBASE_st {
STACK_OF(SRP_user_pwd) *users_pwd;
STACK_OF(SRP_gN_cache) *gN_cache;
@ -115,7 +120,12 @@ DECLARE_STACK_OF(SRP_gN)
SRP_VBASE *SRP_VBASE_new(char *seed_key);
int SRP_VBASE_free(SRP_VBASE *vb);
int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file);
/* This method ignores the configured seed and fails for an unknown user. */
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,

61
crypto/srp/srp_vfy.c
View File

@ -185,7 +185,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;
@ -247,6 +247,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 = (SRP_VBASE *)OPENSSL_malloc(sizeof(SRP_VBASE));
@ -468,21 +486,50 @@ 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;
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;
}
/*
* 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;
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;
}
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;

2
util/libeay.num
View File

@ -1807,6 +1807,8 @@ ASN1_UTCTIME_get 2350 NOEXIST::FUNCTION:
X509_REQ_digest 2362 EXIST::FUNCTION:EVP
X509_CRL_digest 2391 EXIST::FUNCTION:EVP
ASN1_STRING_clear_free 2392 EXIST::FUNCTION:
SRP_VBASE_get1_by_user 2393 EXIST::FUNCTION:SRP
SRP_user_pwd_free 2394 EXIST::FUNCTION:SRP
d2i_ASN1_SET_OF_PKCS7 2397 NOEXIST::FUNCTION:
X509_ALGOR_cmp 2398 EXIST::FUNCTION:
EVP_CIPHER_CTX_set_key_length 2399 EXIST::FUNCTION: