Remove leading zeros when hashing session keys

Issue #12 resolved
Nilesh Shah created an issue

Currently hashing of session key doesn't remove leading zeros from S. This creates mis-match with other platform.

Change -

public BigInteger getSessionKey(final boolean doHash) {

        if (S == null)
            return null;

        if (doHash) {
            return new BigInteger(config.getMessageDigestInstance().digest(S.toByteArray()));
        }
        else {
            return S;
        }
    }

to

public BigInteger getSessionKey(final boolean doHash) {

        if (S == null)
            return null;

        if (doHash) {
            return new BigInteger(config.getMessageDigestInstance().digest(bigIntegerToUnsignedByteArray(S)));
        }
        else {
            return S;
        }
    }

Comments (5)

  1. Connect2id OSS

    Thanks. We'll need to double check this with the spec, to ensure we're not breaking the standard.

  2. simon

    I put a long comment on #11 that we are not yet implementing the RFC spec. So using the same reasoning about breaking clients like Thinbus when we change this, and then breaking it again when we upgrade to the RFC spec on Nimbus3, I am going to mark this as an enhancement which isn't going to get closed until we do Nimbus3.

    My assumption (as mentioned on #11) is that the matter of the leading zeros isn't a security bug. If anyone knows better than I then please do speak up.

  3. simon

    See comments on the related ticket #11 ticket that its not a security bug and we are not compliant with any RFC. So we will fix this by implementing the RFC in Nimbus 3.

    The reasoning is that changing the current noncompliant message format would break Thinbus the JS client. Changing Thinbus then breaks a recent fork of pysrp a python library which can authenticate Thinbus clients. Then if we finally implement the RFC properly in Nimbus 3.0 we would break them both again. So given that that this isn't a security bug (???) we should break clients only once by upgrading to the RFC in a Thinbus 3.0 and not correct any current message format details in Thinbus 2.x unless changes are required to fix actual bugs.

  4. Log in to comment