pyrogram / tgcrypto

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

fix potential write to read only memory #21

Closed Wirtos closed 3 years ago

delivrance commented 3 years ago

Hi, thanks for the PR. I think both of your PR title and commit messages were badly worded since there's no "potential write to read only memory", nor an "infinite loop": The IV is really intended to be written there and the issue you are referring to is what it seems to be just a copy pasta in the function signatures inside the header files (notice how the signature in the actual code files doesn't declare the IV to be constant). Not sure where you see the "infinite loop" either; all that loop you edited does is increment the counter with clear boundaries of where it needs to end.

I can accept changes to the wrong header signatures, but not the rest, since it's just unnecessary refactoring.

Wirtos commented 3 years ago

Hi, thanks for the PR. I think both of your PR title and commit messages were badly worded since there's no "potential write to read only memory", nor an "infinite loop": The IV is really intended to be written there and the issue you are referring to is what it seems to be just a copy pasta in the function signatures inside the header files (notice how the signature in the actual code files doesn't declare the IV to be constant). Not sure where you see the "infinite loop" either; all that loop you edited does is increment the counter with clear boundaries of where it needs to end.

I can accept changes to the wrong header signatures, but not the rest, since it's just unnecessary refactoring.

The loop is in fact infinite, k is unsigned, therefore its condition >= 0 is always true, even gcc warns about that. As for wrong signatures it's a pretty clear problem for data residing in read only memory, which segfaults silently when using these functions with constant iv.

Wirtos commented 3 years ago

whoops.

delivrance commented 3 years ago

k is unsigned, therefore its condition >= 0 is always true

Thanks for pointing that out, that's definitely a bug, but would still not represent an actual infinite loop since, once k goes below 0 (which means it goes to 2^32 - 1), the next instruction iv[k] will produce an undefined behaviour (it will most likely segfault).

Wirtos commented 3 years ago

k is unsigned, therefore its condition >= 0 is always true

Thanks for pointing that out, that's definitely a bug, but would still not represent an actual infinite loop since, once k goes below 0 (which means it goes to 2^32 - 1), the next instruction iv[k] will produce an undefined behaviour (it will most likely segfault).

It might never segfault on a 32-bit bridged address space since 4GB memory can be potentially owned by the parent 64 bit process. But overall segfault is the main problem