Pyinstaller: replace Pycrypto - which is unsupported and buggy

[ghost]

[htgoebel is taking over this formerly empt comment]

pycrypto, which PyInstaller uses for byte-code encryption, needs to be replaced. Its officially "dead" and seriously buggy. Further pycrypto doe not work for Python 3.6 (see below). Replacement options seem to be:

• pycryptdome - which calls it self a drop in replacement but does not work as expected (see below)
• cryptography.

Maintainer note: If you want this to be fixed, please place an adequate(!) bounty or submit a pull-request. The requirement for byte-encryption is a commercial one (free software does not require byte-encryption since the cod is available anyway). So if you are earning money with using PyInstaller you should take parts of the development.

[ProjectThreepio]

Just wanted to point out in this bug too that PyCrypto is abandoned and it may actually be less effort to move to pycryptodome then to try to get PyCrypto working on Python 3.6.

[ghost]

Unfortunately using pycryptodome looks like a non-trivial problem. Aside from some easy-to-fix errors on Python 3, I received this daunting error:

Traceback (most recent call last):
  File "PyInstaller/loader/pyiboot01_bootstrap.py", line 25, in <module>
  File "/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod03_importers.py", line 315, in load_module
    is_pkg, bytecode = self._pyz_archive.extract(real_fullname)
  File "/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod02_archive.py", line 352, in extract
    obj = zlib.decompress(obj)
zlib.error: Error -3 while decompressing data: incorrect header check

I personally wouldn't benefit from this feature and I estimate that this could take up to a week to debug. If I may be so bold, I predict that this could turn into another fiasco like MERGE (which has been unfixed for well over a year now).

[ProjectThreepio]

Agreed it's non-trivial. pycryptodome is far from a drop-in replacement in this particular case. It may end up being a matter of re-writing the feature from the ground up. However, the feature is pretty limited in scope, so maybe that's not an absolute dealbreaker. Still easier than getting PyCrypto working in 3.6 ;)

[ProjectThreepio]

For what it's worth, there are non-commercial uses for byte-encryption, if it's not the code you're trying to obfuscate per se, but sensitive information within that code (generally things you wish you didn't have to do, like hard-coded passwords). While the perfect solution would be to not do this at all, byte-encryption is a desirable band-aid until the perfect solution can be implemented. We use this for in-house scripts, and we're not commercial. I'm afraid I've seen this sort of sensitive data in infrastructure/maintenance scripts in lots of places, so I don't think we're unique.

[lambdaq]

+1 for https://github.com/pyca/cryptography

[jftuga]

FWIW, I've been using https://github.com/pyca/cryptography in conjunction with netmiko for controlling SSH sessions under Python 3.6 and this works very well.

[answerquest]

Copying this over from #3316
Use pycryptodomex, and replace all Crypto terms in the python program's import statements with Cryptodome. The disambiguation is important : it eliminates so many complicated workarounds like hooks etc. It's a wholly different package now and pyinstaller is able to compile it without a hitch. I've posted details (what worked at my end.. I'm not any expert in this subject FYI) in this answer on stackoverflow.

Note: If pycryptodome is working fine for you in your program then this pycryptodomex replacement will help you pyinstaller your program without needing to do any hiddenimports etc. If you haven't able to move from pycrypto to pycryptodome even in your .py program then this solution is not for you.

[MichaelWS]

what would be a sufficient bounty for this?

[ProjectThreepio]

I placed a modest bounty on this bug. It may not be sufficient, but if everyone chips in what they can, it may become adequate to get it fixed.

[ProjectThreepio]

FWIW, a developer has placed a $600 bounty goal on this bug. I've already committed $300 and am tapped out. Assistance would be appreciated. I don't know how these bounty goals work exactly, but presumably there's some interest at least right now, and that's a good thing.

[kuberlog]

I'll work on this issue tomorrow.

[kuberlog]

It seems someone has already made a pull request regarding this issue.

https://github.com/pyinstaller/pyinstaller/pull/4241

If that PR is insufficient, then I will work on the issue.

[gpraceman]

I'll chip in $100 to the bounty pool to get this issue fixed. It worked for me with Python 3.4, but now I've had to move up to 3.7.

[kuberlog]

Depends on if PR #4241 is insufficient to fix this issue. We may just be waiting for a merge.

[ProjectThreepio]

That pull request may be the solution we're looking for... but it hasn't been touched in a month, and it fails some automated checks, so it's not getting merged without some love first.

Right now, the bug bounty is probably the place where we can do the most to make this happen:
https://www.bountysource.com/issues/40772962-replace-pycrypto-which-is-unsupported-and-buggy

[famous1622]

@ProjectThreepio @SlightlyCyborg Sorry, app for checking PR notifications was being buggy. I'm not sure what check it's failing, but I'm pretty sure it's codestyle

[famous1622]

Yep, it was line length :P, not used to it being 79, mine is usually 120

[ProjectThreepio]

PR #4241 has been passing all the automated checks for a month, and seems to be the real deal as far as resolving this bug. Not knowing exactly how this all works, is there something else that needs to be done (by us or others) to help this get merged? (and help famous1622 get his bounty?). Presumably someone needs to review the code in question and decide if it should be merged or needs further changes. It couldn't hurt if more people piled on the bounty, if only to raise the visibility.

[veeshi]

PR #4241 uses pyaes which hasn't had any commits since Sep 20, 2017. Since PyInstaller is well used IMHO it seems more appropriate to use PyCryptodome as @answerquest said as it is regularly updated, feature rich, large community and has performant block ciphers written in C unlike pyaes which is pure python.

[htgoebel]

Neverthe less one either needs to provide a pull-request read to be merged - including test-cases and complying to all the PyInstaller development requirements. Or on needs to fund development by a appropriate amount for a freelance senior expert developer.

[famous1622]

@veeshi I'll look into trying pycryptodome or cryptography then. Just need to study the import machinery more to avoid a problem I kept running into.

[famous1622]

This is out of my skill level at the moment, and I don't have time for it between other projects, driving classes, and school. Maybe I'll look back into it in a few months if it's still open, but I agree with @htgoebel that it'd make sense to have a more experienced developer to work on the problem.

[htgoebel]

You may want to contribute to this bounty. The requirement for byte-encryption is a commercial one (free software does not require byte-encryption since the cod is available anyway). So if you are earning money with using PyInstaller you should take parts of the development.

[ohk2kt3t4]

There are non-commercial use cases too:

For what it's worth, there are non-commercial uses for byte-encryption, if it's not the code you're trying to obfuscate per se, but sensitive information within that code (generally things you wish you didn't have to do, like hard-coded passwords). While the perfect solution would be to not do this at all, byte-encryption is a desirable band-aid until the perfect solution can be implemented. We use this for in-house scripts, and we're not commercial. I'm afraid I've seen this sort of sensitive data in infrastructure/maintenance scripts in lots of places, so I don't think we're unique.

[mnboos]

Unfortunately using pycryptodome looks like a non-trivial problem. Aside from some easy-to-fix errors on Python 3, I received this daunting error:

Traceback (most recent call last):
  File "PyInstaller/loader/pyiboot01_bootstrap.py", line 25, in <module>
  File "/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod03_importers.py", line 315, in load_module
    is_pkg, bytecode = self._pyz_archive.extract(real_fullname)
  File "/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod02_archive.py", line 352, in extract
    obj = zlib.decompress(obj)
zlib.error: Error -3 while decompressing data: incorrect header check

I personally wouldn't benefit from this feature and I estimate that this could take up to a week to debug. If I may be so bold, I predict that this could turn into another fiasco like MERGE (which has been unfixed for well over a year now).

I spent some time investigating how PyCryptodome could be integrated. The error above results however from a failing import, i.e. PyCryptodome cannot be imported during bootstrapping and as a result of that, self.cipher remains unintialized:

https://github.com/pyinstaller/pyinstaller/blob/bb906c0970b41941fc7afc2e93b7237496f0a161/PyInstaller/loader/pyimod02_archive.py#L307-L310

Because cipher is now uninitialized, the decompression obviously fails, because the content needs to be decrypted first.

Unfortunately, I couldn't get the import of PyCryptodome working at this point of bootstrapping, because several builtine modules are not (fully) imported yet and thus the import fails.

Following questions remain unclear for me:

• Why are the builtin modules encrypted?
• Would it be reasonable, to first load and import the builtin modules and finally decrypt and import the usercode?

[mnboos]

Okay, I got a working prototype based on PyCryptodome 🥳

[ohk2kt3t4]

Do you have a working branch online somewhere? (Or perhaps submit as a work-in-progress PR to this repo?)

[mnboos]

Not yet, will do it soon (i.e. probably tomorrow).

[mnboos]

Okay, I pushed a WIP at https://github.com/mnboos/pyinstaller/tree/pycryptodome-for-encryption

Currently, there are some limitations:

• Only --onedir mode is working due to the following dependency problems:
• ctypes package has to be copied to the generated dist folder before execution of the binary
• there is a hardcoded module name buried in the code, which determines if the file should be encrypted or not (this should not be a big problem to solve. I assume the info if a module is usercode and thus should be encrypted can be determined from some dependency graph)

The above issues could probably be solved, by doing this:

• Delay the loading of usercode until bootstrapping is complete, i.e. all dependencies that are required for decryption have been loaded (basically os, platform and ctypes)

[ProjectThreepio]

Is mnboos still working on this or does he need someone else to work on addressing those limitations he identified?

[naufraghi]

Hi all, I have and half baked cython wrapper for the https://github.com/kokke/tiny-AES-c library, the interesting function is the CTR streaming encryption mode, that should be a drop in replacement for the CBC (ups, we are using CFB, that is somewhat similar to CBC) mode we are currently using (https://crypto.stackexchange.com/questions/6029/aes-cbc-mode-or-aes-ctr-mode-recommended and links).

The resulting module will be a single so / pyd file, so we should be able to use the old approach without big changes, avoiding the chicken egg problem of bootstrapping a ctypes/cffi wrapper.

Here is the wrapper for a review: https://github.com/naufraghi/tinyaes-py (WIP, no CI, no release)

If you are interested in having a PR using this minimal AES wrapper instead of a more complex alternative (PyCryptodome / cryptography), I'm available to work on it.

[ProjectThreepio]

veeshi commented earlier about another effort:

PR #4241 uses pyaes which hasn't had any commits since Sep 20, 2017. Since PyInstaller is well used IMHO it seems more appropriate to use PyCryptodome as @answerquest said as it is regularly updated, feature rich, large community and has performant block ciphers written in C unlike pyaes which is pure python.

While I'm not sure that means you must use a big established, long-supported module like PyCryptodome, cryptography is hard, so we should make an effort to ensure cryptographers are actively maintaining the modules we're using. That said, this is ultimately about code obfuscation and can be circumvented by a motivated attacker even under the best of circumstances, so it's not about the quality of the encryption per se, but about the long-term quality of the module's code.

No matter what solution ends up working, consider contributing to the bug bounty:
https://www.bountysource.com/issues/40772962-replace-pycrypto-which-is-unsupported-and-buggy

[mnboos]

Is mnboos still working on this or does he need someone else to work on addressing those limitations he identified?

Maybe I will continue working on it in the near future.

[naufraghi]

The "problem" with bigger libraries is the the attack surface is bigger, and, considering AES CTR mode was introduced in 1979 (wikipedia), I expect most implementations to be quite stable.

The https://github.com/kokke/tiny-AES-c seems to be maintained (the author is responsive on issues and PRs), tested with official codes, and used / starred enough to have a lot of eyes pointed to highlight any security problem may arise in the future.

The wrapper itself is so minimal that could be printed on a t-shirt, and exposes the plain API, please audit it for any problem I may have overlooked.

In the case of PyCrypto, beside the unmaintained state, all the security advisories where not against the AES module, that is the only one included for encryption, but other modules.

So, IMHO, we should not search for a library with a commit each week, if not adding new unrelated features, that is not a good sign for a base crypto library, each commit may introduce bugs, and if there are no known security problems, the library (using 1979 knowledge) is fair to be stale.

That said, we already know that the encryption mode in PyInstaller is an easy obstacle for a motivated cracker, even without involving any crypto attack, so I'd better favor an easy implementation that has minimal impact on the overall code, easier to maintain and evolve, just to represent the security trade-off the feature is offering, avoid tampering and easy reverse engineering and not much more.

Here is a WIP branch: https://github.com/pyinstaller/pyinstaller/pull/4652 (opened a PR to trigger the CI)

PS. just in case, I'm not interested in reclaiming the bounty

[ProjectThreepio]

The bounty for this bug has been boosted. BountySource's site appears to be having troubles right now, so it's hard to see this change.

[Legorooj]

@ProjectThreepio and anyone else who had committed to the bounty - is there a way for @htgoebel to claim it due to the fact @naufraghi isn't interested? To fund PyInstaller maintenance and development

[Legorooj]

Oh and if @naufraghi's solution doesn't work/satisfy, I'll be happy to implement this with cryptography.

[ProjectThreepio]

The bounty is for whoever can write code that makes whatever cut is required to get accepted into the production version of PyInstaller. So in a sense @htgoebel decides the matter, not me. If the person who writes that code doesn't want to claim it, I believe I can (and would) revert it to general PyInstaller project maintenance, or another PyInstaller bug.

[Legorooj]

@ProjectThreepio I agree. It should be repurposed at the least. As my second comment says, I could do this, but don't see the point of doing it if naufraghi's solution works - and if I do end up doing it, I'd donate half of the bounty to the team.

[naufraghi]

I'm OK to give the bounty to the dev team, I very thank them for their work with PyInstaller!

I added a test suite to the wrapper, using hypothesis and found a bug, the bug is fixed, so I think I'll release tinyaes-py 1.0.0 later today and rebase the PR pointing the release (or a semver 1-ish one).

[Legorooj]

I'll review the wrapper at some point soon as well. It's always good to have a second pair of eyes look over it! @naufraghi

[ProjectThreepio]

I had no idea PyInstaller was in such financial straits until now! Yes, absolutely redirect my bounty to general PyInstaller operations, ASAP!

[Legorooj]

@ProjectThreepio thanks! I've only been on the team a week, and I offered htgoebel help with the GitHub end to start with. If he doesn't have to sort through the many support issues and other ones that don't need core developer attention, he'll be able to put more if his funded time towards PyInstaller core...