Some salts cause verification errors even if password is correct

[Jonathan Haas]

If the salt starts with a 0 byte and the next byte has the highest bit set, the verfier generated by SRP6VerifierGenerator will not be able to be verified later even if the correct user credentials are entered. Example code:

    public void test() throws Exception {
        SRP6CryptoParams instance = SRP6CryptoParams.getInstance();
        SRP6VerifierGenerator srp6VerifierGenerator = new SRP6VerifierGenerator(instance);
        BigInteger salt;
        byte[] saltByteArray = SRP6Routines.generateRandomSalt(16);
        saltByteArray[0] = 0;
        saltByteArray[1] |= (byte) 0x80;
        salt = new BigInteger(saltByteArray);
        BigInteger v = srp6VerifierGenerator.generateVerifier(salt, "user", "secret");
        SRP6ClientSession srp6ClientSession = new SRP6ClientSession();
        SRP6ServerSession srp6ServerSession = new SRP6ServerSession(instance);
        srp6ClientSession.step1("user", "secret");
        BigInteger b = srp6ServerSession.step1("user", salt, v);
        SRP6ClientCredentials srp6ClientCredentials = srp6ClientSession.step2(instance, salt, b);
        BigInteger m2 = srp6ServerSession.step2(srp6ClientCredentials.A, srp6ClientCredentials.M1);
        srp6ClientSession.step3(m2);
    }

Expected behaviour: No Exception

Actual behaviour: com.nimbusds.srp6.SRP6Exception: Bad client credentials

If you choose a salt without leading 0 byte, the code will complete without issues.

The current behaviour is that there is a (about) 1 in 512 chance, that the user will not be able to login later even though the pasword is correct.

If you generate the salt with

        salt = new BigInteger(1, saltByteArray);

there is a 1 in 2 chance that the user will not be able to login (that's how I discovered the bug, because I don't like signed salt)

[Jonathan Haas]

Likely caused by #10, Version 1.5.1 doesn't have the issue.

[Connect2id OSS]

Thanks, assuming fix in v1.5.1.

[Jonathan Haas]

Sorry, probably a misunderstanding. The issue is not present in 1.5.1, but it is present in 1.5.3 (and probably 1.5.2 but I didn't test it), so this bug isn't fixed. It was likely caused by the "fix" in #10.

[Jonathan Haas]

Reopened

[Connect2id OSS]

I see :) That it wasn't present in 1.5.1 is highly useful info, and hopefully should make it easier to fix it.

[Jonathan Haas]

The problem is that the class SRP6VerifierGenerator removes the leading zero byte from the salt (as of #10), but the class SRP6ClientSession does not. Solutions seems to be either revert #10, or adjust step2 in SRP6ClientSession, but that will probably cause breakage in existing verifiers when updating NimbisSRP.

[Connect2id OSS]

Do you reckon this could be related to #12?

[Jonathan Haas]

Seems to be a similar issue, but as far as I understand it, #12 is about the session key S and this is about the salt s. Either way, converting between byte arrays and BigIntegers probably should be checked across the code base.

Anyway, thanks for looking into this.

[Connect2id OSS]

@vdzhuvinov Bytes lost in translation? :)

[Jonathan Haas]

Any update on this?

[Connect2id OSS]

Hi Jonathan,

Given our current backlog of jobs and priorities (lots of OIDC stuff), I don't think we'll be able to open this one until the second half of next month (July). If you're able to look into this and submit a patch, this will be much appreciated.

[Jonathan Haas]

I can submit a patch if you want, but I worry about correctly implementing the spec and backwards compatibility.

[Vladimir Dzhuvinov]

Hi guys,

Braking backwards compatibility should be the least concern (we can do a new major release for this, clearly stating what's changed). The # 1 concern should be to get this right. New users will appreciate this, as well as all existing users who choose to go along and upgrade.

I'm really sorry that I'm not able to look into this right now, but at least the source code is available :)

[Jonathan Haas]

I created a pull request.

[Connect2id OSS]

Thanks, that was quick!

[Jonathan Haas]

I also fixed #11 and #12 while I was at it. It was rather quick, because I already had implemented local workarounds for the bugs.

[S Z]

When do you plan to release the fix to this issue?

[simon]

Hey apologies for the delay I am picking this up. Thanks for the reproduce and patch. Just double checking your test it is fails on 1.5.3 and master.

[simon]

I am downgrading this from critical to major as it has a work around which is to keep trying with another salt until success. That then keeps the critical category for security bugs or "product totally unusable with no workarounds".

[Jonathan Haas]

I am downgrading this from critical to major as it has a work around which is to keep trying with another salt until success.

That's not really a good workaround, as the salt is stored together with the verifier at the server, so the workaround is essentially to give yourself a new password until it works (which may or may not be easy depending on the implementation of your "password forgotten" mechanism). The only reasonable workaround currently is to locally patch the library (which I did) or to reject all salts that may cause issues like this.

But I'm happy that you're working on this and I hope to see a fixed official version soon, so we don't need to keep a fork around.

[simon]

fixed with merge of PR #6 by @jonathan_haas