Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SRP6 calculating M1, M2 incorrectly #506

Open
jimm98y opened this issue Dec 21, 2023 · 4 comments · May be fixed by #507
Open

SRP6 calculating M1, M2 incorrectly #506

jimm98y opened this issue Dec 21, 2023 · 4 comments · May be fixed by #507

Comments

@jimm98y
Copy link

jimm98y commented Dec 21, 2023

I was trying to use BouncyCastle SRP6 on the server side for HomeKit and I found out that I can only generate B and S, but then M1 and M2 are calculated in BouncyCastle like:

M1 = H(A | B | S)
M2 = H(A | M1 | S)
K = H(S)

While they should be calculated as follows according to RFC2945:

K = H(S)
M1 = H(H(N) XOR H(g) | H(I) | s | A | B | K)
M2 = H(A | M1 | K)

In C#, I ended up using the following code:

private SecureRandom _random = new SecureRandom();
private readonly IDigest _digest = new Sha512Digest();
private Srp6Server _server;
private byte[] _N;
private byte[] _g;
private byte[] _s;
private byte[] _I;
private byte[] _B;
private byte[] _calculatedM1;
private byte[] _K;
private byte[] _A;

public byte[] GenerateSalt()
{
    byte[] s = new byte[16];
    _random.NextBytes(s);
    return s;
}

public byte[] GenerateServerCredentials(byte[] bN, byte[] bg, byte[] salt, string userName, string password)
{
    _N = bN;
    _g = bg;
    _s = salt;
    _I = Encoding.UTF8.GetBytes(userName);
    
    byte[] P = Encoding.UTF8.GetBytes(password);
    BigInteger N = new BigInteger(1, _N);
    BigInteger g = new BigInteger(1, _g);
    Srp6GroupParameters group = new Srp6GroupParameters(N, g);
    Srp6VerifierGenerator gen = new Srp6VerifierGenerator();

    gen.Init(group, _digest);

    BigInteger v = gen.GenerateVerifier(_s, _I, P);
    _server = new Srp6Server();
    _server.Init(group, v, _digest, _random);

    BigInteger B = _server.GenerateServerCredentials();
    _B = B.ToByteArrayUnsigned();
    return _B;
}

public bool VerifyClientEvidenceMessage(byte[] A, byte[] clientM1)
{
    //  BouncyCastle is using simplified calculation of M1 and M2 using only A, B and S.
    //_ = _server.CalculateSecret(new BigInteger(1, A));
    //return _server.VerifyClientEvidenceMessage(new BigInteger(1, clientM1));

    // let's calculate it using the correct formula: M1 = H(H(N) XOR H(g) | H(I) | s | A | B | K)
    _A = A;
    BigInteger S = _server.CalculateSecret(new BigInteger(1, A));
    _K = SHA(S.ToByteArrayUnsigned());
    _calculatedM1 = SHA(CONCAT(XOR(SHA(_N), SHA(_g)), CONCAT(SHA(_I), CONCAT(_s, CONCAT(A, CONCAT(_B, _K))))));
    bool result = _calculatedM1.SequenceEqual(clientM1);
    return result;
}

public byte[] CalculateServerEvidenceMessage()
{
    //return _server.CalculateServerEvidenceMessage().ToByteArrayUnsigned();
    return SHA(CONCAT(_A, CONCAT(_calculatedM1, _K)));
}

public byte[] CalculateSessionKey()
{
    //return _server.CalculateSessionKey().ToByteArrayUnsigned();   
    return _K;
}

private static byte[] CONCAT(byte[] a, byte[] b)
{
    return a.Concat(b).ToArray();
}

private byte[] SHA(byte[] bytes)
{
    _digest.Reset();
    _digest.BlockUpdate(bytes, 0, bytes.Length);
    byte[] rv = new byte[_digest.GetDigestSize()]; 
    _digest.DoFinal(rv, 0);
    return rv;
}

private static byte[] XOR(byte[] a, byte[] b)
{
    for (int i = 0; i < a.Length; i++)
    {
        a[i] ^= b[i];
    }
    return a;
}

This issue is not just related to C#, I can see the same code in Java as well. The current BouncyCastle implementation is working only when BouncyCastle is being used on both sides - client and the server.

@peterdettman
Copy link
Collaborator

Our implementation is for RFC 5054.

@jimm98y
Copy link
Author

jimm98y commented Dec 24, 2023

Yes, but RFC 5054 it does not specify the algorithm for calculating M1 and M2, or at least I haven't found it there. It delegates M1/M2 to RFC 2945 and it just states that failing to decrypt the finished message performs the same function as M1/M2 in RFC 2945. If I understand it correctly, M1/M2 are not even being used in RFC 5054, but BouncyCastle has the methods for calculating them. And I haven't found anything related to the calculation of M1/M2 using the algorithm currently implemented by BouncyCastle:

M1 = H(A | B | S)
M2 = H(A | M1 | S)

May I ask where does it come from, if not from RFC 5054/RFC 2945?

@jimm98y jimm98y linked a pull request Dec 24, 2023 that will close this issue
@cipherboy
Copy link
Collaborator

cipherboy commented Feb 7, 2024

@jimm98y I was able to hunt this down a little more. It looks like in 1999ish (according to here -- though other commentary points to a later date, perhaps 2002+), Thomas Wu proposed the SRP protocol with computation discussed here. Before (or perhaps after) standardization though (by the same author), it was changed to the version currently present in RFC 2945 (linked above), also sometimes called SRP-3 (perhaps?).

Wikipedia gives this clue about the introduction of our algorithm:

Alternatively, in a password-only proof the calculation of K can be skipped and the shared S proven with:

(with algorithm matching what is in Bouncy Castle, without citations).

The JavaDocs of a third-party package give us a clue as to citation for the reduced form:

The evidence messages 'M1' and 'M2' are computed according to Tom Wu's paper "SRP-6: Improvements and refinements to the Secure Remote Password protocol", table 5, from 2002.

Indeed, reading RFC 5054 linked by Peter above says:

The version of SRP used here is sometimes referred to as "SRP-6"
[SRP-6]. This version is a slight improvement over "SRP-3", which
was described in [SRP] and [SRP-RFC]. For convenience, this document
and [SRP-RFC] include the details necessary to implement SRP-6;
[SRP-6] is cited for informative purposes only.

Though, notably SRP-6 cites what is, presumably, the same paper mentioned by the JavaDocs and the Stanford link above. And, note my bolding: SRP-RFC links to RFC 2945. I believe Peter is thus correct and RFC 5054 is poorly cited/written: it is not RFC 2945 compatible and should be as implemented here in BC. This also matches with the file naming (SRP6).

(However, I do agree, RFC 5054 is very unclear about these differences and lacks the information it states it should have.)

Thus, you would be correct (as per your PR) in proposing alternative interfaces while retaining compatibility for existing users.


However, it (incorrectly) looks like there might be a third variant:

K = H(S)
M1 = H(A | B | K)

as discussed in this rust crate: https://docs.rs/srp/latest/srp/ -- though no source for this variant is mentioned on the README...

As far as I can tell, this variant isn't used in the code base: https://github.com/RustCrypto/PAKEs/blob/master/srp/src/server.rs#L149-L155 does not seem to include K = H(S) and instead includes S directly, aligning with Bouncy Castle's and RFC 5054's implementation. So, this crate should be compatible with our code.

cipherboy added a commit to cipherboy/PAKEs that referenced this issue Feb 7, 2024
As far as I can tell from the code, what is defined as:

 > key = compute_premaster_secret(...)

does not include the given hash invocation step. While RFC 5054 is
unclearly worded (see comment below), SRP-6 is clear that M1 should
not include K = H(S) and thus this description of the protocol is
incorrect. As far as I can tell, nobody else uses this computation
of M1 (with K = H(S) as a parameter) and thus it should be dropped from
the tabular and comment descriptions.

See also: bcgit/bc-csharp#506 (comment)

Signed-off-by: Alexander Scheel <[email protected]>
@jimm98y
Copy link
Author

jimm98y commented Feb 7, 2024

Wow, nice work! Thank you very much for this research, it's really interesting to see how many variants of SRP are out there. Digging a little bit more into HAP-nodejs revealed they also had to modify an existing implementation to make it HAP (RFC 2945) compatible: zarmack/fast-srp@24a32ff

redoz pushed a commit to redoz/PAKEs that referenced this issue Jun 25, 2024
As far as I can tell from the code, what is defined as:

 > key = compute_premaster_secret(...)

does not include the given hash invocation step. While RFC 5054 is
unclearly worded (see comment below), SRP-6 is clear that M1 should
not include K = H(S) and thus this description of the protocol is
incorrect. As far as I can tell, nobody else uses this computation
of M1 (with K = H(S) as a parameter) and thus it should be dropped from
the tabular and comment descriptions.

See also: bcgit/bc-csharp#506 (comment)

Signed-off-by: Alexander Scheel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants