Commit 259b664f authored by Emilia Kasper's avatar Emilia Kasper
Browse files

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: default avatarRich Salz <rsalz@openssl.org>
parent 64333004

CHANGES

+19 −0
Original line number Diff line number Diff line
@@ -4,6 +4,25 @@ 


 Changes between 1.0.2f and 1.0.2g [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]



  *) Change the req app to generate a 2048-bit RSA/DSA key by default,

     if no keysize is specified with default_bits. This fixes an

     omission in an earlier change that changed all RSA/DSA key generation

apps/s_server.c

+31 −18
Original line number Diff line number Diff line
@@ -429,6 +429,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);
@@ -437,21 +439,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
@@ -2452,8 +2458,9 @@ static int sv_body(char *hostname, 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_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",
@@ -2508,8 +2515,9 @@ static int sv_body(char *hostname, 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_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",
@@ -2605,8 +2613,9 @@ 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_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",
@@ -2849,8 +2858,9 @@ static int www_body(char *hostname, int s, int stype, 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_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",
@@ -2891,8 +2901,9 @@ static int www_body(char *hostname, 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_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",
@@ -3236,8 +3247,9 @@ static int rev_body(char *hostname, 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_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",
@@ -3264,8 +3276,9 @@ static int rev_body(char *hostname, 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_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",

crypto/srp/srp.h

+10 −0
Original line number Diff line number Diff line
@@ -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,

crypto/srp/srp_vfy.c

+52 −5
Original line number Diff line number Diff line
@@ -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;

util/libeay.num

+2 −0
Original line number Diff line number Diff line
@@ -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: