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

There is no input validation whatsoever #6

Closed
habnabit opened this issue Nov 20, 2018 · 3 comments
Closed

There is no input validation whatsoever #6

habnabit opened this issue Nov 20, 2018 · 3 comments
Labels
enhancement New feature or request

Comments

@habnabit
Copy link

Just a few of the segfaults that are trivially achieved:

$ python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt()'
[1]    69091 segmentation fault  python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt()'
$ python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("", "", "")'
[1]    69121 segmentation fault  python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("", "", "")'
$ python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("x"*32, "x"*32, "x"*32)'
[1]    69153 segmentation fault  python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("x"*32, "x"*32, "x"*32)'
$ python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("x"*64, "x"*32, "x"*32)'
[1]    69184 segmentation fault  python3 -c 'import tgcrypto; tgcrypto.ige256_encrypt("x"*64, "x"*32, "x"*32)'
$ python3 -c 'import os, tgcrypto; tgcrypto.ige256_encrypt(os.urandom(32), os.urandom(32), os.urandom(32))'
$ python3 -c 'import os, tgcrypto; tgcrypto.ige256_encrypt(os.urandom(32), os.urandom(32), os.urandom(32), "")'
[1]    69231 segmentation fault  python3 -c
$ python3 -c 'import os, tgcrypto; tgcrypto.ige256_encrypt(os.urandom(32), os.urandom(32))'
[1]    69262 segmentation fault  python3 -c

I don't even understand how it's possible to segfault with 'x'*32 for every input. There's clearly some major issues here.

cryptography is the standard choice for cryptography in python. If you must use your own hand-written AES implementation (please, please don't) then at least take a page from cryptography's book and use cffi for calling from python into not-python.

@delivrance
Copy link
Member

@habnabit Hi. As you pointed out already, I confirm there's indeed no input validation anywhere in the lib. Being TgCrypto written for Pyrogram, I didn't bother too much implementing such checks here.

I don't even understand how it's possible to segfault with 'x'*32 for every input. There's clearly some major issues here.

The problem here is you using Python strings instead of bytes (i.e.: "x"*32 instead of b"x"*32). Any other segfault arised because of similar misusage. So, currently, you must precisely follow the method signatures in order to avoid issues and this boils down to:

  • Use the correct types (bytes and not str)
  • Pass the correct amount of arguments (all of them, as shown in the examples)
  • For IGE mode, pad data to match a multiple of 16 B

cryptography is the standard choice for cryptography in python. If you must use your own hand-written AES implementation (please, please don't) then at least take a page from cryptography's book and use cffi for calling from python into not-python.

Thanks for the suggestion. The reason about rolling my own crypto is firstly because of learning purposes and then because I wanted something lean and portable that can be easily installed and used right away; I could't find any library that would satisfy these requirements and in the current state of Pyrogram it is unlikely I'll switch to anything else.

@habnabit
Copy link
Author

My mistake; I'm thoroughly marinated in python 2, so I always go for '' strings for bytes. However, fixing that issue presents another:

>>> tgcrypto.ige256_decrypt(b'1', b'1', b'1')
b'\xb2'
>>> tgcrypto.ige256_decrypt(b'1', b'1', b'1 ')
b'\x94'
>>> tgcrypto.ige256_decrypt(b'1', b'1', b'1  ')
b'\x0c'
>>> tgcrypto.ige256_decrypt(b'1', b'1', b'1 ')
b'8'
>>> tgcrypto.ige256_decrypt(b'1', b'1', b'1')
b'W'

AES/IGE should definitely be a pure function, and the random output given these inputs indicates that there's illegal reads being done over the input strings.

@delivrance
Copy link
Member

What you say is correct. I'll keep this open as enhancement (as we discussed in the group).

If you are willing to implement input validations all around the library you're welcome, but for now you must follow the rules above to avoid segfaults. In addition of what I explained before, for your latest non-working examples, you must also provide a key and an iv of 32 bytes in IGE mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants