cocagne/csrp

Minimal C implementation of the Secure Remote Password protocol (version 6a)

csrp's Issues

windows init_random

#include <windows.h>
#include <bcrypt.h>
#include <ntstatus.h>
#include <wincred.h>

static void init_random()
{
unsigned char buff[64];

if (g_initialized)
    return;

#ifdef WIN32
{
BCRYPT_ALG_HANDLE hAlg = NULL;
void* hRng = NULL;
NTSTATUS status = 0;

    status = BCryptOpenAlgorithmProvider(&hAlg, BCRYPT_RNG_ALGORITHM, NULL, 0);
    if (status != STATUS_SUCCESS) {
        goto cleanup;
    }

    status = BCryptGenRandom(hAlg, buff, sizeof(buff), 0);
    if (status != STATUS_SUCCESS) {
        goto cleanup;
    }

    g_initialized = 1;
    RAND_seed(buff, sizeof(buff));

cleanup:
    if (hRng) {
        BCryptCloseAlgorithmProvider((BCRYPT_ALG_HANDLE)hRng, 0);
    }
    if (hAlg) {
        BCryptCloseAlgorithmProvider(hAlg, 0);
    }
}

#else
{
FILE* fp = fopen("/dev/urandom", "rb");
if (fp) {
size_t read = fread(buff, sizeof(buff), 1, fp);
g_initialized = read == 1;
fclose(fp);
}
}

if (g_initialized)
    RAND_seed(buff, sizeof(buff));

#endif
}

Enum SRP_NGType out of order

Values from enum SRP_NGType are used as indices into the array global_Ng_constants, but the enum and array values are not in the same order. Only group 1024 happens to be in the position declared by the enum. Asking for any other group gives you a different group's values.

srp_create_salted_verification_key can return incorrect size

We're seeing a symptom where this code:

srp_create_salted_verification_key(algo, group, I, P, strlen(P),
      &salt_buf, &salt_len, &ver_buf, &ver_len, 0, 0);

can return 3 or less in salt_len, not the expected 4. Based on how often it happens (1-4 times typical per ~500 tries) we're guessing that it's happening when the top 8 bits of the salt happen to be 0, and the value isn't getting zero-padded to 32 bits properly.

Calling the same function with the same values almost always fixes it, but we worry that we're effectively truncating the salt by doing so, since some values are now not possible, thus an attacker would know not to even try them.

This doesn't happen for the same inputs each time, either, which further indicates that it's the library's internal handling of the random numbers that's at fault here, not our call.

Warn more explicitly about RFC5054 compatibility

I have integrated this SRP6a implementation in my code base in 2017, only to realize now that it never was interoperable with the RFC5054 standard. I don't recall if the note was explicit enough back then, but even today, most people will default to using the master branch. Instead of a soft "NOTE" in the readme, this should be some sort of scary warning that can't be missed.

I understand the point of maintaining backwards compatibility for older implementations that used this code before RFC5054, but shouldn't the master branch conform to RFC5054, with a pre-RFC5054 compatibility branch instead of the other way around? It's not like people fetch the code directly from the master branch every single time they build their project.

incorrect implementation of H_nn

https://github.com/cocagne/csrp/blob/master/srp.c#L294

For example, when compute
k=H(N, g)

g must be padding to the same length of N before hashing, this cause the incorrect result to the SRP6a specs.

hash bug

Unused result of calculation

I noticed that in srp_user_process_challenge() the variable v holds the result of g^x but v is never used. In fact, a couple lines down the same calculation is done again and the result is stored in tmp1.

RFC 5054 paddings

RFC 5054 specifies left padding during calculation of k and u as expressed below:
k = H(N, PAD(g))
u = H(PAD(A), PAD(B))

PAD(X) inserts left padding (zeros) into the binary big-endian representation of X to match it's number of bytes with the number of bytes in N.

Current code doesn't compute the paddings.

problem with the added 1536-bit N compatibility in branch rfc5054_compat

the entry for 1536 bit N was added in second place in static struct NGHex global_Ng_constants[] in srp.c

but was added in last (sixth) place in enum SRP_NGType in srp.c

This caused me a lot of grief in adapting this code (for an Apple Airplay receiver, Apple uses a srp-6a variant) till I located the error...

Calculation for B = kv + g^b should not be conditional on rfc5054_compat

The difference between the if and else cases around line 600 in srp.c differ in whether they do modular arithmetic, but the 2002 Wu paper (SRP6) says "All values are computed modulo N" giving at most sizeof(N) bits. This behavior is not specific to RFC 5054.

With that flag unset, you get > sizeof(N) outputs for B from srp_verifier_new().