[openssl-commits] [openssl] master update
Emilia Kasper
emilia at openssl.org
Thu Feb 25 14:48:57 UTC 2016
The branch master has been updated
via 380f18ed5f140e0ae1b68f3ab8f4f7c395658d9e (commit)
from 37529928faa8456e85a9c5ad9255517da8dd0c61 (commit)
- Log -----------------------------------------------------------------
commit 380f18ed5f140e0ae1b68f3ab8f4f7c395658d9e
Author: Emilia Kasper <emilia at openssl.org>
Date: Wed Feb 24 12:59:59 2016 +0100
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 at openssl.org>
-----------------------------------------------------------------------
Summary of changes:
CHANGES | 15 +++++++++++++
apps/s_server.c | 44 ++++++++++++++++++++++++--------------
crypto/srp/srp_vfy.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-----
include/openssl/srp.h | 12 ++++++++++-
util/libeay.num | 4 +++-
5 files changed, 110 insertions(+), 23 deletions(-)
diff --git a/CHANGES b/CHANGES
index d849648..51e7b08 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 1380628..6645dc8 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 0a9de14..0d3da7e 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 83a3293..4111d51 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 ca8e9ec..ad7ad9d 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
More information about the openssl-commits
mailing list