Replace H() function int return by bytearray

[bgouesbetnetatmo]

Hi everyone,

I realized that the library can have some issues when calculating the X parameter. This is because the hash function returns an int instead of a bytearray, which removes the padding zeros. Hash functions are supposed to return fixed size digest messages (for example, SHA512 should return a 64 byte digest message). But when converting the result to an int, we lose the preceding 0's and change the size of the parameter H( I | ':' | p ) in the calculation of x = H(s, H( I | ':' | p )).

To overcome this problem, I propose that the H() function returns a bytearray and that we convert to an int at assignment time.

[cocagne]

What problem have you run into that motivated this patch? The reason I ask is that this codebase has been stable for quite some time now and I'm reluctant to change it without a solid reason.

[bgouesbetnetatmo]

Hello @cocagne

I have been working on a C++ implementation of the SRP verifier key calculation. To validate my development, I wanted to generate test values, thanks to your library, to compare with the values returned by the function I developed.
I encountered a particular case when calculating x, where the 256 hash of my "user:password" was 00c6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e4a450c5e24eb7edf5e107921884fa50be37231738ff3b7de. I had prefixed my salt with the value 1b9749b25583aedb6b1fdca42223470a.
The next calculation to get x was to do the 256 hash of my salt (16 bytes) + the 256 hash of my "user:password" (64 bytes). This would normally give the call of sha256(1b9749b25583aedb6b1fdca42223470a00c6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e4a450c5e24eb7edf5e10921884fa50be37231738ff3b7dee).

But in converting the hash from bytearray to int, we lose the first padding byte, because it is 0. So we get sha256(1b9749b25583aedb6b1fdca42223470ac6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e450c5e24eb7edf5e107921884fa50be37231738ff3b7dee), giving a different result.

So I thought it would be better if the H() returned a bytearray rather than an int, which would remove the first two 0s and reduce the size of the digest message.

[cocagne]

Interesting. I'm not sure I'm ready to merge this without an actual compatibility problem. The risk is probably fairly low that it'd cause bugs but the core code for calculating the hashes and values hasn't been modified in several years and it's been a source of hard-to-track-down bugs previously. I don't know if you noticed it but there is a C implementation that's compatible with pysrp available at https://github.com/cocagne/csrp/tree/rfc5054_compat That might help with your C++ code.

…

On Wed, May 24, 2023 at 9:15 AM bgouesbetnetatmo ***@***.***> wrote: Hello @cocagne <https://github.com/cocagne> I have been working on a C++ implementation of the SRP verifier key calculation. To validate my development, I wanted to generate test values, thanks to your library, to compare with the values returned by the function I developed. I encountered a particular case when calculating x, where the 256 hash of my "user:password" was 00c6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e4a450c5e24eb7edf5e107921884fa50be37231738ff3b7de. I had prefixed my salt with the value 1b9749b25583aedb6b1fdca42223470a. The next calculation to get x was to do the 256 hash of my salt (16 bytes) + the 256 hash of my "user:password" (64 bytes). This would normally give the call of sha256(1b9749b25583aedb6b1fdca42223470a00c6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e4a450c5e24eb7edf5e10921884fa50be37231738ff3b7dee) . But in converting the hash from bytearray to int, we lose the first padding byte, because it is 0. So we get sha256(1b9749b25583aedb6b1fdca42223470ac6207aa1855108d52b3063b56614f57a26ca0ce1e85011e2566eac6cf10665f75be43a1ae550e450c5e24eb7edf5e107921884fa50be37231738ff3b7dee), giving a different result. So I thought it would be better if the H() returned a bytearray rather than an int, which would remove the first two 0s and reduce the size of the digest message. — Reply to this email directly, view it on GitHub <#53 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANMW7A2TA2BDJMV5RXITDTXHYJWJANCNFSM6AAAAAAYKNYYGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bgouesbetnetatmo]

Of course, I understand that such a change in the calculation function for the SRP verifier key could have a big impact. And this has been used for several years in this way.

Yes, I have seen your C library. I want to thank you for your work on pysrp, csrp and your documentation. They are very complete and well documented. It has been a great help to me🙏🏼

I just used your csrp library to check the values returned by my pysrp changes.
I created a new method, based on the srp_create_salted_verification_key() function, to compute a verifier key from a prefixed salt as input. I called this function srp_create_verification_key().

Here is my function:

void srp_create_verification_key( SRP_HashAlgorithm alg,
                                  SRP_NGType ng_type, const char * username,
                                  const unsigned char * password, int len_password,
                                  const unsigned char * bytes_s, int len_s,
                                  const unsigned char ** bytes_v, int * len_v,
                                  const char * n_hex, const char * g_hex )
{
    BIGNUM     * s   = BN_new();
    BIGNUM     * v   = BN_new();
    BIGNUM     * x   = 0;
    BN_CTX     * ctx = BN_CTX_new();
    NGConstant * ng  = new_ng( ng_type, n_hex, g_hex );

    if( !s || !v || !ctx || !ng )
       goto cleanup_and_exit;

    init_random(); /* Only happens once */

    BN_bin2bn(bytes_s, len_s, s);

    x = calculate_x( alg, s, username, password, len_password );

    if( !x )
       goto cleanup_and_exit;

    BN_mod_exp(v, ng->g, x, ng->N, ctx);

    *len_v   = BN_num_bytes(v);

    *bytes_v = (const unsigned char *) malloc( *len_v );

    if (!bytes_s || !bytes_v)
       goto cleanup_and_exit;

    BN_bn2bin(v, (unsigned char *) *bytes_v);

 cleanup_and_exit:
    delete_ng( ng );
    BN_free(s);
    BN_free(v);
    BN_free(x);
    BN_CTX_free(ctx);
}

I give you my test values so that you can reproduce the problem on the calculation of x.
username = "Pair-Setup"
password = "159-70-351"
alg = SRP_SHA512
I hope this would help you to reproduce the problem.

In the csrp code, the variable used to compute the hash value of "username:password" is a byte array, named ucp_hash, with a fixed size SHA512_DIGEST_LENGTH (equal to 64 bytes). So there is no problem with the zero padding byte being removed when converting to int and changing the hash value.

[cocagne]

Given a second bug report for this same issue by @rubenmoral I've decided to go ahead and merge your PR. Thanks for the submission and thanks to @rubenmoral for finding this PR.