Remove leading zeros when hashing session keys
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)
-
-
@nileshshah can you provide a little more information. Is this related to
#11? -
I put a long comment on
#11that 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. -
-
assigned issue to
- marked as enhancement
- marked as minor
See comments on the related ticket
#11ticket 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.
-
assigned issue to
-
- changed status to resolved
fixed with merge of PR #6 by @jonathan_haas
- Log in to comment
Thanks. We'll need to double check this with the spec, to ensure we're not breaking the standard.