pyrogram / tgcrypto

Fast and Portable Cryptography Extension Library for Pyrogram
https://pyrogram.org
GNU Lesser General Public License v3.0
175 stars 41 forks source link

There is no input validation whatsoever #6

Closed habnabit closed 5 years ago

habnabit commented 5 years ago

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 commented 5 years ago

@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:

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 commented 5 years ago

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 commented 5 years ago

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.