hashlib: Fix shadowing of sha256 and sha512 constructors.

[mbuesch]

hashlib.sha256() and hashlib.sha512() fails with:
TypeError: 'module' object is not callable

This fixes it by renaming the algorithm implementation modules.
Note that 'import hashlib.sha256' is not supported on CPython either,
so we don't need to support that.

[dpgeorge]

I don't understand the need for this. The following works for me on uPy:

>>> import hashlib
>>> hashlib.sha256()
<sha256 object at fd56b560>

and all other variants of the sha.

[mbuesch]

I'm currently unsure. The above message was a cut and paste from yesterday.

I was trying to implement the hashlib.new() function, because my application uses this.
Without this patch it was not possible to do so, because from inside of hashlib's init.py accessing sha256 referenced the module instead of the function.

But I think it's a hell of confusing anyway, to have a subfile named the same as a function that is exported from the package.

[mbuesch]

See this commit: https://github.com/mbuesch/micropython-lib/commit/b86ca286b6fb65c4460e62850d45a3b9c502eb41

Without the file rename it will fail the test at line 6:

  File ".../micropython-lib/hashlib/test_hashlib.py", line 6, in <module>
TypeError: 'module' object is not callable

[mbuesch]

it seems that this is the issue:

>>> from hashlib.sha256 import test
>>> import hashlib
>>> hashlib.sha256
<module 'hashlib.sha256' from '.../micropython-lib/build//hashlib/sha256.py'>

[pfalcon]

So, the underlying issue appears to be ambiguity of what "hashlib.sha256" is. It can be hashlib/sha256.py , or "sha256" symbol from "hashlib". It should be the latter, but something glitches, exactly when "test" is imported.

from .sha256 import sha224, sha256
from .sha512 import sha384, sha512

print(sha224, sha256, sha384, sha512)

def new(name):
    print(sha224, sha256, sha384, sha512)
    return globals()[name]()

With implementation like that, and test_hashlib.py appended with:

import hashlib
h = hashlib.new("sha256")

It prints:

<class 'sha224'> <class 'sha256'> <class 'sha384'> <class 'sha512'>
OK
<class 'sha224'> <module 'hashlib.sha256' from '/mnt/hdd/projects-3rdparty/micropython-lib/hashlib/hashlib/sha256.py'> <class 'sha384'> <class 'sha512'>

So, doing it like that triggers some weird corner case. Anybody wants to get to the bottom of it? ;-)

Otherwise, hashlib.new() implementation is trivial, and if there's a separate test file for it, works as expected.

[mbuesch]

I think the problem is that the from hashlib.sha256 import test as sha256_test imports hashlib and sets hashlib.sha256 as the module, because it only imports from that submodule explicitly. A subsequent import hashlib, which is supposed to trigger the __init__.py to override the sha256 attribute has no effect, because hashlib already is imported.

[mbuesch]

def new(name):
    return globals()[name]()

Just a quick comment on that:
I was also thinking about implementing it like that. But I'm not so sure it's a good idea. Can the name string be trusted? What if it is a user/network supplied name?
CPython does it in a similar way, though. (But way more complicated). I'm not sure if that would be vulnerable, too.
So as to avoid calling random stuff, I hardcoded the dict of allowed functions in my new().

[pfalcon]

I think the problem is that the from hashlib.sha256 import test as sha256_test imports hashlib and sets hashlib.sha256 as the module, because it only imports from that submodule explicitly. A subsequent import hashlib, which is supposed to trigger the init.py to override the sha256 attribute has no effect, because hashlib already is imported.

Apparently, importing any component of qualified module name should always process init.py if exists (this needs to be tested, actually, testcase for this added to uPy testsuite). If that doesn't happen, that's apparently a bug. If you can point to related code in the import implementation, then the issue is confirmed. Otherwise, someone else will need to confirm it.

[pfalcon]

I was also thinking about implementing it like that. But I'm not so sure it's a good idea. Can the name string be trusted? What if it is a user/network supplied name?

You should validate anything like that yourself. Yet better, avoid using over-dynamic features like that.

So as to avoid calling random stuff, I hardcoded the dict of allowed functions in my new().

Any Python app already can call random stuff. If the concern is app feeding random stuff to outside of it (like to stdlib function), then this vulnerability was covered in a more reliably way - by not providing function which may accept random stuff. CPython itself discourages using it: "The named constructors are much faster than new() and should be preferred." https://docs.python.org/3/library/hashlib.html#hashlib.new . Finally, the implementation was posted to show how it's trivial and that it doesn't have to be adhoc function in the module - it's just generic look up of an arbitrary class in a namespace. So, it's the same situation was with socket.error - it's not really needed. Of course, if people want it for compatibility with legacy code, it can be added. But doing "it in a similar way (But way more complicated)" doesn't buy much here IMHO, accepting that the best solution is just not using that function.

[mbuesch]

Ok, well. I'm not that interested in arguing whether this feature is needed or whether it should be implemented or not. The micropython-lib responsible developers will have to answer the question whether CPython compatibility is wanted here on their own. If new() is not implemented by micropython-lib, that is ok. I'm perfectly fine with either decision.
So I'll simply adapt my code regardless of the decision. That's pretty easy and works for both cases.

[pfalcon]

@mbuesch : Sounds good, please don't close this issue, sometimes more data/input, or even just time passed are needed to come to a decision.

[pfalcon]

Thanks, that was indeed needed, applied independently in pfalcon/pycopy-lib@f8ee045. Sorry for all the delays and lack of proper investigation in the first place (too many things to care about) :-(.