Fix some SRP issues

-> Remove memory allocation bugs
-> Merge changes from upstream, enabling customizeable memory allocation

diff --git a/src/client.cpp b/src/client.cpp
index 4a9398f7082c07b89f73242c7921cc8ba18592b2..b2e21743c4949fde69fca67b13758f697bc5c321 100644 (file)
--- a/src/client.cpp
+++ b/src/client.cpp
@@ -1059,8 +1059,10 @@ void Client::startAuth(AuthMechanism chosen_auth_mechanism)
                                m_password.length(), NULL, NULL);
                        char *bytes_A = 0;
                        size_t len_A = 0;
-                       srp_user_start_authentication((struct SRPUser *) m_auth_data,
-                               NULL, NULL, 0, (unsigned char **) &bytes_A, &len_A);
+                       SRP_Result res = srp_user_start_authentication(
+                               (struct SRPUser *) m_auth_data, NULL, NULL, 0,
+                               (unsigned char **) &bytes_A, &len_A);
+                       FATAL_ERROR_IF(res != SRP_OK, "Creating local SRP user failed.");
 
                        NetworkPacket resp_pkt(TOSERVER_SRP_BYTES_A, 0);
                        resp_pkt << std::string(bytes_A, len_A) << based_on;

diff --git a/src/util/auth.cpp b/src/util/auth.cpp
index df8940e87c03ad57542f79a9e86c95e060dcc5ee..0c17a92373b617031223dac34b2a5b20cd7d131f 100644 (file)
--- a/src/util/auth.cpp
+++ b/src/util/auth.cpp
@@ -24,6 +24,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "sha1.h"
 #include "srp.h"
 #include "string.h"
+#include "debug.h"
 
 // Get an sha-1 hash of the player's name combined with
 // the password entered. That's what the server uses as
@@ -50,10 +51,11 @@ void getSRPVerifier(const std::string &name,
        char **bytes_v, size_t *len_v)
 {
        std::string n_name = lowercase(name);
-       srp_create_salted_verification_key(SRP_SHA256, SRP_NG_2048,
+       SRP_Result res = srp_create_salted_verification_key(SRP_SHA256, SRP_NG_2048,
                n_name.c_str(), (const unsigned char *)password.c_str(),
                password.size(), (unsigned char **)salt, salt_len,
                (unsigned char **)bytes_v, len_v, NULL, NULL);
+       FATAL_ERROR_IF(res != SRP_OK, "Couldn't create salted SRP verifier");
 }
 
 // Get a db-ready SRP verifier
@@ -67,6 +69,7 @@ inline static std::string getSRPVerifier(const std::string &name,
        size_t len_v;
        getSRPVerifier(name, password, salt, &salt_len,
                &bytes_v, &len_v);
+       assert(*salt); // usually, srp_create_salted_verification_key promises us to return SRP_ERR when *salt == NULL
        std::string ret_val = encodeSRPVerifier(std::string(bytes_v, len_v),
                std::string(*salt, salt_len));
        free(bytes_v);

diff --git a/src/util/srp.cpp b/src/util/srp.cpp
index 94426db92950270121d11349948b135dffab7d94..b4af58d624ba582283d5e207ec7731ee575d3c8c 100644 (file)
--- a/src/util/srp.cpp
+++ b/src/util/srp.cpp
@@ -69,6 +69,19 @@ static int g_initialized = 0;
 static unsigned int g_rand_idx;
 static unsigned char g_rand_buff[RAND_BUFF_MAX];
 
+void *(*srp_alloc) (size_t) = &malloc;
+void *(*srp_realloc) (void *, size_t) = &realloc;
+void (*srp_free) (void *) = &free;
+
+void srp_set_memory_functions(
+               void *(*new_srp_alloc) (size_t),
+               void *(*new_srp_realloc) (void *, size_t),
+               void (*new_srp_free) (void *)) {
+       srp_alloc = new_srp_alloc;
+       srp_realloc = new_srp_realloc;
+       srp_free = new_srp_free;
+}
+
 typedef struct
 {
        mpz_t N;
@@ -171,13 +184,13 @@ static void delete_ng(NGConstant *ng)
        if (ng) {
                mpz_clear(ng->N);
                mpz_clear(ng->g);
-               free(ng);
+               srp_free(ng);
        }
 }
 
 static NGConstant *new_ng( SRP_NGType ng_type, const char *n_hex, const char *g_hex )
 {
-       NGConstant *ng = (NGConstant *) malloc(sizeof(NGConstant));
+       NGConstant *ng = (NGConstant *) srp_alloc(sizeof(NGConstant));
        mpz_init(ng->N);
        mpz_init(ng->g);
 
@@ -367,18 +380,18 @@ static int H_nn(mpz_t result, SRP_HashAlgorithm alg, const mpz_t N, const mpz_t
        size_t len_n1 = mpz_num_bytes(n1);
        size_t len_n2 = mpz_num_bytes(n2);
        size_t nbytes = len_N + len_N;
-       unsigned char *bin = (unsigned char *) malloc(nbytes);
+       unsigned char *bin = (unsigned char *) srp_alloc(nbytes);
        if (!bin)
                return 0;
        if (len_n1 > len_N || len_n2 > len_N) {
-               free(bin);
+               srp_free(bin);
                return 0;
        }
        memset(bin, 0, nbytes);
        mpz_to_bin(n1, bin + (len_N - len_n1));
        mpz_to_bin(n2, bin + (len_N + len_N - len_n2));
        hash( alg, bin, nbytes, buff );
-       free(bin);
+       srp_free(bin);
        mpz_from_bin(buff, hash_length(alg), result);
        return 1;
 }
@@ -387,13 +400,13 @@ static int H_ns(mpz_t result, SRP_HashAlgorithm alg, const unsigned char *n, siz
 {
        unsigned char buff[SHA512_DIGEST_LENGTH];
        size_t nbytes = len_n + len_bytes;
-       unsigned char *bin = (unsigned char *) malloc(nbytes);
+       unsigned char *bin = (unsigned char *) srp_alloc(nbytes);
        if (!bin)
                return 0;
        memcpy(bin, n, len_n);
        memcpy(bin + len_n, bytes, len_bytes);
        hash(alg, bin, nbytes, buff);
-       free(bin);
+       srp_free(bin);
        mpz_from_bin(buff, hash_length(alg), result);
        return 1;
 }
@@ -418,23 +431,23 @@ static int calculate_x(mpz_t result, SRP_HashAlgorithm alg, const unsigned char
 static void update_hash_n(SRP_HashAlgorithm alg, HashCTX *ctx, const mpz_t n)
 {
        size_t len = mpz_num_bytes(n);
-       unsigned char* n_bytes = (unsigned char *) malloc(len);
+       unsigned char* n_bytes = (unsigned char *) srp_alloc(len);
        if (!n_bytes)
                return;
        mpz_to_bin(n, n_bytes);
        hash_update(alg, ctx, n_bytes, len);
-       free(n_bytes);
+       srp_free(n_bytes);
 }
 
 static void hash_num( SRP_HashAlgorithm alg, const mpz_t n, unsigned char *dest )
 {
        int nbytes = mpz_num_bytes(n);
-       unsigned char *bin = (unsigned char *) malloc(nbytes);
+       unsigned char *bin = (unsigned char *) srp_alloc(nbytes);
        if(!bin)
                return;
        mpz_to_bin(n, bin);
        hash(alg, bin, nbytes, dest);
-       free(bin);
+       srp_free(bin);
 }
 
 static void calculate_M(SRP_HashAlgorithm alg, NGConstant *ng, unsigned char *dest,
@@ -510,7 +523,7 @@ static void srp_pcgrandom_seed(srp_pcgrandom *r, unsigned long long int state,
 }
 
 
-static int fill_buff()
+static SRP_Result fill_buff()
 {
        g_rand_idx = 0;
 
@@ -526,7 +539,7 @@ static int fill_buff()
        CryptGenRandom(wctx, sizeof(g_rand_buff), (BYTE*) g_rand_buff);
        CryptReleaseContext(wctx, 0);
 
-       return 1;
+       return SRP_OK;
 
 #else
        fp = fopen("/dev/urandom", "r");
@@ -535,41 +548,48 @@ static int fill_buff()
                fread(g_rand_buff, sizeof(g_rand_buff), 1, fp);
                fclose(fp);
        } else {
-               srp_pcgrandom *r = (srp_pcgrandom *) malloc(sizeof(srp_pcgrandom));
+               srp_pcgrandom *r = (srp_pcgrandom *) srp_alloc(sizeof(srp_pcgrandom));
+               if (!r)
+                       return SRP_ERR;
                srp_pcgrandom_seed(r, time(NULL) ^ clock(), 0xda3e39cb94b95bdbULL);
                size_t i = 0;
                for (i = 0; i < RAND_BUFF_MAX; i++) {
                        g_rand_buff[i] = srp_pcgrandom_next(r);
                }
+               srp_free(r);
        }
 #endif
-       return 1;
+       return SRP_OK;
 }
 
-static void mpz_fill_random(mpz_t num)
+static SRP_Result mpz_fill_random(mpz_t num)
 {
        // was call: BN_rand(num, 256, -1, 0);
        if (RAND_BUFF_MAX - g_rand_idx < 32)
-               fill_buff();
+               if (fill_buff() != SRP_OK)
+                       return SRP_ERR;
        mpz_from_bin((const unsigned char *) (&g_rand_buff[g_rand_idx]), 32, num);
        g_rand_idx += 32;
+       return SRP_OK;
 }
 
-static void init_random()
+static SRP_Result init_random()
 {
        if (g_initialized)
-               return;
-       g_initialized = fill_buff();
+               return SRP_OK;
+       SRP_Result ret = fill_buff();
+       g_initialized = (ret == SRP_OK);
+       return ret;
 }
 
 #define srp_dbg_num(num, text) ;
 /*void srp_dbg_num(mpz_t num, char * prevtext)
 {
        int len_num = mpz_num_bytes(num);
-       char *bytes_num = (char*) malloc(len_num);
+       char *bytes_num = (char*) srp_alloc(len_num);
        mpz_to_bin(num, (unsigned char *) bytes_num);
        srp_dbg_data(bytes_num, len_num, prevtext);
-       free(bytes_num);
+       srp_free(bytes_num);
 
 }*/
 
@@ -579,35 +599,42 @@ static void init_random()
  *
  ***********************************************************************************************************/
 
-void srp_create_salted_verification_key( SRP_HashAlgorithm alg,
+SRP_Result srp_create_salted_verification_key( SRP_HashAlgorithm alg,
        SRP_NGType ng_type, const char *username_for_verifier,
        const unsigned char *password, size_t len_password,
        unsigned char **bytes_s,  size_t *len_s,
        unsigned char **bytes_v, size_t *len_v,
        const char *n_hex, const char *g_hex )
 {
+       SRP_Result ret = SRP_OK;
+
        mpz_t v; mpz_init(v);
        mpz_t x; mpz_init(x);
        NGConstant *ng = new_ng(ng_type, n_hex, g_hex);
 
-       if(!ng)
-               goto cleanup_and_exit;
+       if (!ng)
+               goto error_and_exit;
 
-       init_random(); /* Only happens once */
+       if (init_random() != SRP_OK) /* Only happens once */
+               goto error_and_exit;
 
        if (*bytes_s == NULL) {
-               *len_s = 16;
-               if (RAND_BUFF_MAX - g_rand_idx < 16)
-                       fill_buff();
-               *bytes_s = (unsigned char*)malloc(sizeof(char) * 16);
-               memcpy(*bytes_s, &g_rand_buff + g_rand_idx, sizeof(char) * 16);
-               g_rand_idx += 16;
+               size_t size_to_fill = 16;
+               *len_s = size_to_fill;
+               if (RAND_BUFF_MAX - g_rand_idx < size_to_fill)
+                       if (fill_buff() != SRP_OK)
+                               goto error_and_exit;
+               *bytes_s = (unsigned char*)srp_alloc(size_to_fill);
+               if (!*bytes_s)
+                       goto error_and_exit;
+               memcpy(*bytes_s, &g_rand_buff + g_rand_idx, size_to_fill);
+               g_rand_idx += size_to_fill;
        }
 
 
        if (!calculate_x(x, alg, *bytes_s, *len_s, username_for_verifier,
                        password, len_password))
-               goto cleanup_and_exit;
+               goto error_and_exit;
 
        srp_dbg_num(x, "Server calculated x: ");
 
@@ -615,10 +642,10 @@ void srp_create_salted_verification_key( SRP_HashAlgorithm alg,
 
        *len_v = mpz_num_bytes(v);
 
-       *bytes_v = (unsigned char*)malloc(*len_v);
+       *bytes_v = (unsigned char*)srp_alloc(*len_v);
 
        if (!bytes_v)
-               goto cleanup_and_exit;
+               goto error_and_exit;
 
        mpz_to_bin(v, *bytes_v);
 
@@ -626,6 +653,10 @@ cleanup_and_exit:
        delete_ng( ng );
        mpz_clear(v);
        mpz_clear(x);
+       return ret;
+error_and_exit:
+       ret = SRP_ERR;
+       goto cleanup_and_exit;
 }
 
 
@@ -663,19 +694,23 @@ struct SRPVerifier *srp_verifier_new(SRP_HashAlgorithm alg,
        if (!ng)
                goto cleanup_and_exit;
 
-       ver = (struct SRPVerifier *) malloc( sizeof(struct SRPVerifier) );
+       ver = (struct SRPVerifier *) srp_alloc( sizeof(struct SRPVerifier) );
 
        if (!ver)
                goto cleanup_and_exit;
 
-       init_random(); /* Only happens once */
+       if (init_random() != SRP_OK) { /* Only happens once */
+               srp_free(ver);
+               ver = 0;
+               goto cleanup_and_exit;
+       }
 
-       ver->username = (char *) malloc(ulen);
+       ver->username = (char *) srp_alloc(ulen);
        ver->hash_alg = alg;
        ver->ng = ng;
 
        if (!ver->username) {
-               free(ver);
+               srp_free(ver);
                ver = 0;
                goto cleanup_and_exit;
        }
@@ -690,11 +725,15 @@ struct SRPVerifier *srp_verifier_new(SRP_HashAlgorithm alg,
                if (bytes_b) {
                        mpz_from_bin(bytes_b, len_b, b);
                } else {
-                       mpz_fill_random(b);
+                       if (mpz_fill_random(b) != SRP_OK) {
+                               srp_free(ver);
+                               ver = 0;
+                               goto cleanup_and_exit;
+                       }
                }
 
                if (!H_nn(k, alg, ng->N, ng->N, ng->g)) {
-                       free(ver);
+                       srp_free(ver);
                        ver = 0;
                        goto cleanup_and_exit;
                }
@@ -705,7 +744,7 @@ struct SRPVerifier *srp_verifier_new(SRP_HashAlgorithm alg,
                mpz_addm(B, tmp1, tmp2, ng->N, tmp3);
 
                if (!H_nn(u, alg, ng->N, A, B)) {
-                       free(ver);
+                       srp_free(ver);
                        ver = 0;
                        goto cleanup_and_exit;
                }
@@ -723,11 +762,11 @@ struct SRPVerifier *srp_verifier_new(SRP_HashAlgorithm alg,
                calculate_H_AMK(alg, ver->H_AMK, A, ver->M, ver->session_key);
 
                *len_B = mpz_num_bytes(B);
-               *bytes_B = (unsigned char*)malloc(*len_B);
+               *bytes_B = (unsigned char*)srp_alloc(*len_B);
 
                if (!*bytes_B) {
-                       free(ver->username);
-                       free(ver);
+                       srp_free(ver->username);
+                       srp_free(ver);
                        ver = 0;
                        *len_B = 0;
                        goto cleanup_and_exit;
@@ -737,7 +776,7 @@ struct SRPVerifier *srp_verifier_new(SRP_HashAlgorithm alg,
 
                ver->bytes_B = *bytes_B;
        } else {
-               free(ver);
+               srp_free(ver);
                ver = 0;
        }
 
@@ -762,10 +801,10 @@ void srp_verifier_delete(struct SRPVerifier *ver)
 {
        if (ver) {
                delete_ng(ver->ng);
-               free(ver->username);
-               free(ver->bytes_B);
+               srp_free(ver->username);
+               srp_free(ver->bytes_B);
                memset(ver, 0, sizeof(*ver));
-               free(ver);
+               srp_free(ver);
        }
 }
 
@@ -814,14 +853,15 @@ struct SRPUser *srp_user_new(SRP_HashAlgorithm alg, SRP_NGType ng_type,
        const unsigned char *bytes_password, size_t len_password,
        const char *n_hex, const char *g_hex)
 {
-       struct SRPUser *usr = (struct SRPUser *) malloc(sizeof(struct SRPUser));
+       struct SRPUser *usr = (struct SRPUser *) srp_alloc(sizeof(struct SRPUser));
        size_t ulen  = strlen(username) + 1;
        size_t uvlen = strlen(username_for_verifier) + 1;
 
        if (!usr)
                goto err_exit;
 
-       init_random(); /* Only happens once */
+       if (init_random() != SRP_OK) /* Only happens once */
+               goto err_exit;
 
        usr->hash_alg = alg;
        usr->ng = new_ng(ng_type, n_hex, g_hex);
@@ -833,12 +873,12 @@ struct SRPUser *srp_user_new(SRP_HashAlgorithm alg, SRP_NGType ng_type,
        if (!usr->ng)
                goto err_exit;
 
-       usr->username = (char*)malloc(ulen);
-       usr->username_verifier = (char*)malloc(uvlen);
-       usr->password = (unsigned char*)malloc(len_password);
+       usr->username = (char*)srp_alloc(ulen);
+       usr->username_verifier = (char*)srp_alloc(uvlen);
+       usr->password = (unsigned char*)srp_alloc(len_password);
        usr->password_len = len_password;
 
-       if (!usr->username || !usr->password)
+       if (!usr->username || !usr->password || !usr->username_verifier)
                goto err_exit;
 
        memcpy(usr->username, username, ulen);
@@ -858,15 +898,13 @@ err_exit:
                mpz_clear(usr->S);
                if (usr->ng)
                        delete_ng(usr->ng);
-               if (usr->username)
-                       free(usr->username);
-               if (usr->username_verifier)
-                       free(usr->username_verifier);
+               srp_free(usr->username);
+               srp_free(usr->username_verifier);
                if (usr->password) {
                        memset(usr->password, 0, usr->password_len);
-                       free(usr->password);
+                       srp_free(usr->password);
                }
-               free(usr);
+               srp_free(usr);
        }
 
        return 0;
@@ -885,15 +923,15 @@ void srp_user_delete(struct SRPUser *usr)
 
                memset(usr->password, 0, usr->password_len);
 
-               free(usr->username);
-               free(usr->username_verifier);
-               free(usr->password);
+               srp_free(usr->username);
+               srp_free(usr->username_verifier);
+               srp_free(usr->password);
 
                if (usr->bytes_A)
-                       free(usr->bytes_A);
+                       srp_free(usr->bytes_A);
 
                memset(usr, 0, sizeof(*usr));
-               free(usr);
+               srp_free(usr);
        }
 }
 
@@ -926,33 +964,38 @@ size_t srp_user_get_session_key_length(struct SRPUser *usr)
 
 
 /* Output: username, bytes_A, len_A */
-void srp_user_start_authentication(struct SRPUser *usr, char **username,
+SRP_Result srp_user_start_authentication(struct SRPUser *usr, char **username,
        const unsigned char *bytes_a, size_t len_a,
        unsigned char **bytes_A, size_t *len_A)
 {
        if (bytes_a) {
                mpz_from_bin(bytes_a, len_a, usr->a);
        } else {
-               mpz_fill_random(usr->a);
+               if (mpz_fill_random(usr->a) != SRP_OK)
+                       goto error_and_exit;
        }
 
        mpz_powm(usr->A, usr->ng->g, usr->a, usr->ng->N);
 
        *len_A = mpz_num_bytes(usr->A);
-       *bytes_A = (unsigned char*)malloc(*len_A);
+       *bytes_A = (unsigned char*)srp_alloc(*len_A);
 
-       if (!*bytes_A) {
-               *len_A = 0;
-               *bytes_A = 0;
-               *username = 0;
-               return;
-       }
+       if (!*bytes_A)
+               goto error_and_exit;
 
        mpz_to_bin(usr->A, *bytes_A);
 
        usr->bytes_A = *bytes_A;
        if (username)
                *username = usr->username;
+
+       return SRP_OK;
+
+error_and_exit:
+       *len_A = 0;
+       *bytes_A = 0;
+       *username = 0;
+       return SRP_ERR;
 }
 
 

diff --git a/src/util/srp.h b/src/util/srp.h
index 15a2b8a6867c39e0611ce5a6d5465f515ca662d8..c876e70e6c8dc253fc94dbf71510ec1ab6d216bd 100644 (file)
--- a/src/util/srp.h
+++ b/src/util/srp.h
@@ -78,6 +78,22 @@ typedef enum
        SRP_SHA512*/
 } SRP_HashAlgorithm;
 
+typedef enum
+{
+       SRP_OK,
+       SRP_ERR,
+} SRP_Result;
+
+/* Sets the memory functions used by srp.
+ * Note: this doesn't set the memory functions used by gmp,
+ * but it is supported to have different functions for srp and gmp.
+ * Don't call this after you have already allocated srp structures.
+ */
+void srp_set_memory_functions(
+       void *(*new_srp_alloc) (size_t),
+       void *(*new_srp_realloc) (void *, size_t),
+       void (*new_srp_free) (void *));
+
 /* Out: bytes_v, len_v
  *
  * The caller is responsible for freeing the memory allocated for bytes_v
@@ -86,8 +102,11 @@ typedef enum
  * If provided, they must contain ASCII text of the hexidecimal notation.
  *
  * If bytes_s == NULL, it is filled with random data. The caller is responsible for freeing.
+ *
+ * Returns SRP_OK on success, and SRP_ERR on error.
+ * bytes_s might be in this case invalid, don't free it.
  */
-void srp_create_salted_verification_key( SRP_HashAlgorithm alg,
+SRP_Result srp_create_salted_verification_key( SRP_HashAlgorithm alg,
        SRP_NGType ng_type, const char *username_for_verifier,
        const unsigned char *password, size_t len_password,
        unsigned char **bytes_s,  size_t *len_s,
@@ -101,6 +120,8 @@ void srp_create_salted_verification_key( SRP_HashAlgorithm alg,
  * The n_hex and g_hex parameters should be 0 unless SRP_NG_CUSTOM is used for ng_type
  *
  * If bytes_b == NULL, random data is used for b.
+ *
+ * Returns pointer to SRPVerifier on success, and NULL on error.
  */
 struct SRPVerifier* srp_verifier_new(SRP_HashAlgorithm alg, SRP_NGType ng_type,
        const char *username,
@@ -114,7 +135,7 @@ struct SRPVerifier* srp_verifier_new(SRP_HashAlgorithm alg, SRP_NGType ng_type,
 
 void srp_verifier_delete( struct SRPVerifier* ver );
 
-
+// srp_verifier_verify_session must have been called before
 int srp_verifier_is_authenticated( struct SRPVerifier* ver );
 
 
@@ -128,7 +149,9 @@ const unsigned char* srp_verifier_get_session_key( struct SRPVerifier* ver,
 size_t srp_verifier_get_session_key_length(struct SRPVerifier* ver);
 
 
-/* user_M must be exactly srp_verifier_get_session_key_length() bytes in size */
+/* Verifies session, on success, it writes bytes_HAMK.
+ * user_M must be exactly srp_verifier_get_session_key_length() bytes in size
+ */
 void srp_verifier_verify_session( struct SRPVerifier* ver,
        const unsigned char* user_M, unsigned char** bytes_HAMK );
 
@@ -154,7 +177,7 @@ size_t srp_user_get_session_key_length(struct SRPUser* usr);
 
 /* Output: username, bytes_A, len_A. If you don't want it get written, set username to NULL.
  * If bytes_a == NULL, random data is used for a. */
-void srp_user_start_authentication(struct SRPUser* usr, char** username,
+SRP_Result srp_user_start_authentication(struct SRPUser* usr, char** username,
        const unsigned char* bytes_a, size_t len_a,
        unsigned char** bytes_A, size_t* len_A);