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

fix potential write to read only memory #21

Merged
merged 3 commits into from Apr 7, 2021
Merged

Conversation

Wirtos
Copy link
Contributor

@Wirtos Wirtos commented Mar 26, 2021

No description provided.

@delivrance
Copy link
Member

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
Copy link
Contributor Author

Wirtos commented Apr 3, 2021

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 Wirtos closed this Apr 3, 2021
@Wirtos Wirtos reopened this Apr 3, 2021
@Wirtos
Copy link
Contributor Author

Wirtos commented Apr 3, 2021

whoops.

@delivrance
Copy link
Member

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).

tgcrypto/ctr256.c Show resolved Hide resolved
tgcrypto/aes256.h Outdated Show resolved Hide resolved
@Wirtos
Copy link
Contributor Author

Wirtos commented Apr 7, 2021

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

@delivrance delivrance merged commit 9a9e55b into pyrogram:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants