Skip to content

Commit

Permalink
CVE-2016-0798: avoid memory leak in SRP
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@ekasper
ekasper committed Feb 25, 2016
1 parent 3752992 commit 380f18e
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 23 deletions.
15 changes: 15 additions & 0 deletions CHANGES
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@


Changes between 1.0.2f and 1.1.0 [xx XXX xxxx] 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 *) Configuration change; it's now possible to build dynamic engines
without having to build shared libraries and vice versa. This without having to build shared libraries and vice versa. This
only applies to the engines in engines/, those in crypto/engine/ only applies to the engines in engines/, those in crypto/engine/
Expand Down
44 changes: 28 additions & 16 deletions apps/s_server.c
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ typedef struct srpsrvparm_st {
static int ssl_srp_server_param_cb(SSL *s, int *ad, void *arg) static int ssl_srp_server_param_cb(SSL *s, int *ad, void *arg)
{ {
srpsrvparm *p = (srpsrvparm *) arg; srpsrvparm *p = (srpsrvparm *) arg;
int ret = SSL3_AL_FATAL;

if (p->login == NULL && p->user == NULL) { if (p->login == NULL && p->user == NULL) {
p->login = SSL_get_srp_username(s); p->login = SSL_get_srp_username(s);
BIO_printf(bio_err, "SRP username = \"%s\"\n", p->login); BIO_printf(bio_err, "SRP username = \"%s\"\n", p->login);
Expand All @@ -360,21 +362,25 @@ static int ssl_srp_server_param_cb(SSL *s, int *ad, void *arg)


if (p->user == NULL) { if (p->user == NULL) {
BIO_printf(bio_err, "User %s doesn't exist\n", p->login); BIO_printf(bio_err, "User %s doesn't exist\n", p->login);
return SSL3_AL_FATAL; goto err;
} }

if (SSL_set_srp_server_param if (SSL_set_srp_server_param
(s, p->user->N, p->user->g, p->user->s, p->user->v, (s, p->user->N, p->user->g, p->user->s, p->user->v,
p->user->info) < 0) { p->user->info) < 0) {
*ad = SSL_AD_INTERNAL_ERROR; *ad = SSL_AD_INTERNAL_ERROR;
return SSL3_AL_FATAL; goto err;
} }
BIO_printf(bio_err, BIO_printf(bio_err,
"SRP parameters set: username = \"%s\" info=\"%s\" \n", "SRP parameters set: username = \"%s\" info=\"%s\" \n",
p->login, p->user->info); 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->user = NULL;
p->login = NULL; p->login = NULL;
return SSL_ERROR_NONE; return ret;
} }


#endif #endif
Expand Down Expand Up @@ -2325,9 +2331,10 @@ static int sv_body(int s, int stype, unsigned char *context)
#ifndef OPENSSL_NO_SRP #ifndef OPENSSL_NO_SRP
while (SSL_get_error(con, k) == SSL_ERROR_WANT_X509_LOOKUP) { while (SSL_get_error(con, k) == SSL_ERROR_WANT_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP renego during write\n"); BIO_printf(bio_s_out, "LOOKUP renego during write\n");
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand Down Expand Up @@ -2393,9 +2400,10 @@ static int sv_body(int s, int stype, unsigned char *context)
#ifndef OPENSSL_NO_SRP #ifndef OPENSSL_NO_SRP
while (SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) { while (SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP renego during read\n"); BIO_printf(bio_s_out, "LOOKUP renego during read\n");
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand Down Expand Up @@ -2520,9 +2528,10 @@ static int init_ssl_connection(SSL *con)
while (i <= 0 && SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) { while (i <= 0 && SSL_get_error(con, i) == SSL_ERROR_WANT_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP during accept %s\n", BIO_printf(bio_s_out, "LOOKUP during accept %s\n",
srp_callback_parm.login); srp_callback_parm.login);
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand Down Expand Up @@ -2732,9 +2741,10 @@ static int www_body(int s, int stype, unsigned char *context)
if (BIO_should_io_special(io) if (BIO_should_io_special(io)
&& BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP renego during read\n"); BIO_printf(bio_s_out, "LOOKUP renego during read\n");
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand Down Expand Up @@ -3093,9 +3103,10 @@ static int rev_body(int s, int stype, unsigned char *context)
if (BIO_should_io_special(io) if (BIO_should_io_special(io)
&& BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP renego during accept\n"); BIO_printf(bio_s_out, "LOOKUP renego during accept\n");
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand All @@ -3121,9 +3132,10 @@ static int rev_body(int s, int stype, unsigned char *context)
if (BIO_should_io_special(io) if (BIO_should_io_special(io)
&& BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) { && BIO_get_retry_reason(io) == BIO_RR_SSL_X509_LOOKUP) {
BIO_printf(bio_s_out, "LOOKUP renego during read\n"); BIO_printf(bio_s_out, "LOOKUP renego during read\n");
SRP_user_pwd_free(srp_callback_parm.user);
srp_callback_parm.user = srp_callback_parm.user =
SRP_VBASE_get_by_user(srp_callback_parm.vb, SRP_VBASE_get1_by_user(srp_callback_parm.vb,
srp_callback_parm.login); srp_callback_parm.login);
if (srp_callback_parm.user) if (srp_callback_parm.user)
BIO_printf(bio_s_out, "LOOKUP done %s\n", BIO_printf(bio_s_out, "LOOKUP done %s\n",
srp_callback_parm.user->info); srp_callback_parm.user->info);
Expand Down
58 changes: 53 additions & 5 deletions crypto/srp/srp_vfy.c
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ static char *t_tob64(char *dst, const unsigned char *src, int size)
return olddst; 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) if (user_pwd == NULL)
return; return;
Expand Down Expand Up @@ -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); 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 *SRP_VBASE_new(char *seed_key)
{ {
SRP_VBASE *vb = OPENSSL_malloc(sizeof(*vb)); SRP_VBASE *vb = OPENSSL_malloc(sizeof(*vb));
Expand Down Expand Up @@ -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; int i;
SRP_user_pwd *user; SRP_user_pwd *user;
unsigned char digv[SHA_DIGEST_LENGTH];
unsigned char digs[SHA_DIGEST_LENGTH];
EVP_MD_CTX *ctxt = NULL;


if (vb == NULL) if (vb == NULL)
return NULL; return NULL;

for (i = 0; i < sk_SRP_user_pwd_num(vb->users_pwd); i++) { for (i = 0; i < sk_SRP_user_pwd_num(vb->users_pwd); i++) {
user = sk_SRP_user_pwd_value(vb->users_pwd, i); user = sk_SRP_user_pwd_value(vb->users_pwd, i);
if (strcmp(user->id, username) == 0) if (strcmp(user->id, username) == 0)
return user; 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) || if ((vb->seed_key == NULL) ||
(vb->default_g == NULL) || (vb->default_N == NULL)) (vb->default_g == NULL) || (vb->default_N == NULL))
return NULL; return NULL;
Expand Down
12 changes: 11 additions & 1 deletion include/openssl/srp.h
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -85,14 +85,19 @@ typedef struct SRP_gN_cache_st {
DEFINE_STACK_OF(SRP_gN_cache) DEFINE_STACK_OF(SRP_gN_cache)


typedef struct SRP_user_pwd_st { typedef struct SRP_user_pwd_st {
/* Owned by us. */
char *id; char *id;
BIGNUM *s; BIGNUM *s;
BIGNUM *v; BIGNUM *v;
/* Not owned by us. */
const BIGNUM *g; const BIGNUM *g;
const BIGNUM *N; const BIGNUM *N;
/* Owned by us. */
char *info; char *info;
} SRP_user_pwd; } SRP_user_pwd;


void SRP_user_pwd_free(SRP_user_pwd *user_pwd);

DEFINE_STACK_OF(SRP_user_pwd) DEFINE_STACK_OF(SRP_user_pwd)


typedef struct SRP_VBASE_st { typedef struct SRP_VBASE_st {
Expand All @@ -118,7 +123,12 @@ DEFINE_STACK_OF(SRP_gN)
SRP_VBASE *SRP_VBASE_new(char *seed_key); SRP_VBASE *SRP_VBASE_new(char *seed_key);
void SRP_VBASE_free(SRP_VBASE *vb); void SRP_VBASE_free(SRP_VBASE *vb);
int SRP_VBASE_init(SRP_VBASE *vb, char *verifier_file); 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 *SRP_create_verifier(const char *user, const char *pass, char **salt,
char **verifier, const char *N, const char *g); char **verifier, const char *N, const char *g);
int SRP_create_verifier_BN(const char *user, const char *pass, BIGNUM **salt, int SRP_create_verifier_BN(const char *user, const char *pass, BIGNUM **salt,
Expand Down
4 changes: 3 additions & 1 deletion util/libeay.num
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -4073,7 +4073,7 @@ OPENSSL_memcmp 4565 1_1_0 EXIST::FUNCTION:
OPENSSL_strncasecmp 4566 1_1_0 EXIST::FUNCTION: OPENSSL_strncasecmp 4566 1_1_0 EXIST::FUNCTION:
OPENSSL_gmtime 4567 1_1_0 EXIST::FUNCTION: OPENSSL_gmtime 4567 1_1_0 EXIST::FUNCTION:
OPENSSL_gmtime_adj 4568 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_Calc_server_key 4570 1_1_0 EXIST::FUNCTION:SRP
SRP_create_verifier 4571 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 SRP_create_verifier_BN 4572 1_1_0 EXIST::FUNCTION:SRP
Expand Down Expand Up @@ -4711,3 +4711,5 @@ OPENSSL_thread_stop 5213 1_1_0 EXIST::FUNCTION:
OPENSSL_INIT_new 5215 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_free 5216 1_1_0 EXIST::FUNCTION:
OPENSSL_INIT_set_config_filename 5217 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

0 comments on commit 380f18e

Please sign in to comment.