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>

2 files changed, 62 insertions, 5 deletions

diff --git a/crypto/srp/srp.h b/crypto/srp/srp.h
index d072536fec..028892a1ff 100644
--- a/crypto/srp/srp.h
+++ b/crypto/srp/srp.h
@@ -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,
diff --git a/crypto/srp/srp_vfy.c b/crypto/srp/srp_vfy.c
index a3f1a8a0a4..26ad3e07b4 100644
--- a/crypto/srp/srp_vfy.c
+++ b/crypto/srp/srp_vfy.c
@@ -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;
-    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;
     }
+
+    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;
+
+    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;