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 memory corruption crash #1823

Merged
merged 2 commits into from Apr 14, 2023
Merged

Conversation

H3rnand3zzz
Copy link
Contributor

Read commit messages. For crash payload, contact me.

@H3rnand3zzz H3rnand3zzz force-pushed the fix/msg-crash branch 2 times, most recently from d71cf84 to f7b8bb6 Compare April 13, 2023 08:17
@jubalh jubalh requested review from sjaeckel and jubalh April 13, 2023 09:59
@jubalh jubalh added this to the next milestone Apr 13, 2023
Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I agree with the changes, but I don't see a potential double free here.

The issue we're seeing is an out-of-bounds access of the data returned by gpgme_data_release_and_get_mem(). The call only allocates len bytes, but profanity is accessing len+1 bytes.

Didn't we already have a similar issue in the same part of the code not long ago? Maybe those parts should be reviewed!?

Please update the commit message, PR title&description accordingly.

src/pgp/gpg.c Outdated Show resolved Hide resolved
src/pgp/ox.c Outdated Show resolved Hide resolved
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Apr 13, 2023

Thanks for the PR!

I agree with the changes, but I don't see a potential double free here.

The issue we're seeing is an out-of-bounds access of the data returned by gpgme_data_release_and_get_mem(). The call only allocates len bytes, but profanity is accessing len+1 bytes.

Didn't we already have a similar issue in the same part of the code not long ago? Maybe those parts should be reviewed!?

Please update the commit message, PR title&description accordingly.

Thank you for the review. In fact, there was a consistent crash caused by some payload, which glib claimed to be "double free or corruption". I don't see double free here as well, so probably it's a false positive or rarther "memory corruption" issue, which breaks the execution anyway. I renamed to "memory corruption".

Line from where of GDB:
#5 0x00007f48a05e8657 in malloc_printerr (str=str@entry=0x7f48a06f45f8 "double free or corruption (!prev)") at malloc.c:5651

@H3rnand3zzz H3rnand3zzz changed the title Fix double free crash Fix memory corruption crash Apr 13, 2023
@sjaeckel
Copy link
Member

I renamed to "memory corruption".

please also reword the commit message. GH is ephemeral, the Git history will most likely survive this service.

@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Apr 13, 2023

I renamed to "memory corruption".

please also reword the commit message. GH is ephemeral, the Git history will most likely survive this service.

I thought that I did, probably messed it up on some stage :) Fixed, thanks.

@jubalh
Copy link
Member

jubalh commented Apr 13, 2023

Mmhh now we have a merge commit in there. Can you just rebase on master instead?
Also I would prefer if the 2nd commit (re cleanup) could be a bit more descriptive in the heading. Something like Cleanup p_ox_gpg_decrypt or something like that would already suffice. Then when skimming through the list of changes when looking for something specific I don't need to click on the entry to see which part is actually cleaned up/touched.

Under certain circumstances setting plain_str[len] to 0 might lead to crash
and it does not follow the best practices as well.

This change allows better handling of buffer copying and prevents crash.
In OX implementation gpgme's buffer remains untouched, thus not leading to the crash.

But code can be shorter and more concise.
@jubalh jubalh merged commit a99a4fa into profanity-im:master Apr 14, 2023
5 checks passed
@jubalh
Copy link
Member

jubalh commented Apr 14, 2023

Thanks!

@H3rnand3zzz H3rnand3zzz deleted the fix/msg-crash branch April 14, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants