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

pysmb-provided MD4 implementation for Python3 is broken #196

Closed
zentarim opened this issue Jun 3, 2022 · 6 comments
Closed

pysmb-provided MD4 implementation for Python3 is broken #196

zentarim opened this issue Jun 3, 2022 · 6 comments

Comments

@zentarim
Copy link

zentarim commented Jun 3, 2022

If there is no MD4 hash in hashlib then pysmb provides it's own pure Python class instead. It produces a wrong hash what render the lib unusable.

Test case:

import hashlib
from smb.utils.md4 import MD4


if __name__ == '__main__':
    test_bytes = b"The quick brown fox jumps over the lazy dog"
    md4_pysmb = MD4()
    md4_pysmb.update(test_bytes)
    digest_pysmb = md4_pysmb.digest()

    md4_builtin = hashlib.new('md4')
    md4_builtin.update(test_bytes)
    digest_builtin = md4_builtin.digest()

    print(digest_pysmb.hex())               # 1b00ee006900a4006b00a800110018005c00190047006200ab00ae00ae009000
    print(digest_builtin.hex())             # 1bee69a46ba811185c194762abaeae90
    assert digest_pysmb == digest_builtin   # Fails

pysmb version:
1.2.7

Python 3.8.13

@zentarim zentarim mentioned this issue Jun 3, 2022
@zentarim
Copy link
Author

zentarim commented Jun 3, 2022

I was able to make it work, although the code does look weird for me. Seems that s and padding in MD4.update() are intended to represent "lists of bytes". Moreover, they are initialized as such. Next both of them get filled with U32 objects which, I believe, represents an unsigned int 4 bytes long. It makes no sense for me.

As far as I can see, all it works because U32 objects always return a number within 1 byte range.

Since 9c472d42e3ac193c2e43d947b9c9c0c50aa5bd95 the code could not work properly any more, because a 16-bytes hex string result additionally getting encoded as 'UTF-16LE' and becoming 32-bytes long. Moreover, after 9c472d42e3ac193c2e43d947b9c9c0c50aa5bd95 all NTLM tests have broken:

$ nose2 -v tests.test_ntlm
tests.test_ntlm.test_NTLMv1_with_extended_security ... FAIL
tests.test_ntlm.test_NTLMv1_without_extended_security ... FAIL
tests.test_ntlm.test_NTLMv2 ... FAIL

======================================================================
FAIL: tests.test_ntlm.test_NTLMv1_with_extended_security
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zentarim/py/pysmb/python3/tests/test_ntlm.py", line 27, in test_NTLMv1_with_extended_security
    assert binascii.hexlify(nt_challenge_response).lower() == b'75 37 f8 03 ae 36 71 28 ca 45 82 04 bd e7 ca f8 1e 97 ed 26 83 26 72 32'.replace(b' ', b'')  # [MS-NLMP]: 4.2.3.2.2
AssertionError

======================================================================
FAIL: tests.test_ntlm.test_NTLMv1_without_extended_security
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zentarim/py/pysmb/python3/tests/test_ntlm.py", line 14, in test_NTLMv1_without_extended_security
    assert binascii.hexlify(nt_challenge_response).lower() == b'67 c4 30 11 f3 02 98 a2 ad 35 ec e6 4f 16 33 1c 44 bd be d9 27 84 1f 94'.replace(b' ', b'')  # [MS-NLMP]: 4.2.2.2.1
AssertionError

======================================================================
FAIL: tests.test_ntlm.test_NTLMv2
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zentarim/py/pysmb/python3/tests/test_ntlm.py", line 47, in test_NTLMv2
    assert binascii.hexlify(lm_challenge_response).lower() == b'86 c3 50 97 ac 9c ec 10 25 54 76 4a 57 cc cc 19 aa aa aa aa aa aa aa aa'.replace(b' ', b'')  # [MS-NLMP]: 4.2.4.2.1
AssertionError

----------------------------------------------------------------------
Ran 3 tests in 0.006s

FAILED (failures=3)

A pull-request is proposed in #198

After it, the tests start to pass again:

$ nose2 -v tests.test_ntlm
tests.test_ntlm.test_NTLMv1_with_extended_security ... ok
tests.test_ntlm.test_NTLMv1_without_extended_security ... ok
tests.test_ntlm.test_NTLMv2 ... ok

----------------------------------------------------------------------
Ran 3 tests in 0.006s

OK

@zentarim
Copy link
Author

zentarim commented Jun 3, 2022

I forgot to mention the root cause:

I hit this issue in Ubuntu 22.04 which has TLS 3 installed where MD4 algorithm is deprecated.
So, the embedded in your lib pure python version is used.

@zentarim zentarim changed the title pysmb-provided MD4 implementation is broken pysmb-provided MD4 implementation for Python3 is broken Jun 3, 2022
@pihdave
Copy link

pihdave commented Jun 3, 2022

looking into this too. __int__() support seems to be dropped in python 3.10, maybe? We're experimenting with this fix:

def int_array2str(array):
    str = ""
    for i in array:
        if isinstance(i, U32):
            str = str + chr(int(i.truth()))
        else:
            str = str + chr(i)
    return str

FWIW, we think this was the breaking change in 3.10. from the change docs: "Builtin and extension functions that take integer arguments no longer accept Decimals, Fractions and other objects that can be converted to integers only with a loss (e.g. that have the int() method but do not have the index() method). (Contributed by Serhiy Storchaka in bpo-37999.)
"

@zentarim
Copy link
Author

zentarim commented Jun 4, 2022

from the change docs: "Builtin and extension functions that take integer arguments no longer accept Decimals, Fractions an

This int_array2str function is utterly a Python2 one. I see no need for it in Python3.

@miketeo
Copy link
Owner

miketeo commented Jun 12, 2022

pysmb 1.2.8 has been released which might relieve this issue.

@zentarim
Copy link
Author

It works.

Thanks, @miketeo !

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