cocagne / pysrp

Python implementation of the Secure Remote Password protocol (SRP)
MIT License
113 stars 42 forks source link

Issue when calculating X in pure Python implementation #55

Closed rubenmoral closed 1 month ago

rubenmoral commented 1 month ago

With certain combinations of username and password, the library does not calculate correctly X due to a wrong conversion between byte array and integer in H ( I | ":" | P ) , which loses the initial 0x00 byte(s), and the successive conversion from integer to byte array when hashing the salt along with the previous hash.

Here's a detailed example of this issue happening when username is apiservice and password 1257c:

  1. Hash username and password. H(apiservice:1257c) = 00d108f217cbcb3f960ddd7b776ba781ca8e079f21d454d27afaabaa6272f59c The library converts this value to integer, which results in 369332777322568772068616687266314200464576548617341020254200090987014059420.
  2. Hash salt and previous hash. The issue comes here, when the library converts again the previous hash, in integer format, to byte array, which results in d108f217cbcb3f960ddd7b776ba781ca8e079f21d454d27afaabaa6272f59c. Note that this byte array is 1 byte less than the original because the first byte was 00. This causes the result hash value to be different than it should, which generates a totally different X.

This only happens in the pure Python implementation, not in the ctypes one.

rubenmoral commented 1 month ago

I see there is a PR to fix this issue, #53. To minimize compatibility risks changing the H() function, another alternative would be to replace the gen_x() function with the following:

def gen_x( hash_class, salt, username, password ):
    username = username.encode() if hasattr(username, 'encode') else username
    password = password.encode() if hasattr(password, 'encode') else password
    if _no_username_in_x:
        username = six.b('')

    h = hash_class()
    h.update( username + six.b(':') + password )
    usr_pwd_hash = h.digest()
    h = hash_class()
    h.update( salt )
    h.update( usr_pwd_hash )

    return int( h.hexdigest(), 16 )
cocagne commented 1 month ago

Merged PR #53 to address this issue. Thanks for the report Ruben and thanks for reminding me that this PR was outstanding.