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

Still buffer overflows with rDNS tags #62

Open
ocococococ opened this issue Jan 13, 2023 · 4 comments
Open

Still buffer overflows with rDNS tags #62

ocococococ opened this issue Jan 13, 2023 · 4 comments

Comments

@ocococococ
Copy link

ocococococ commented Jan 13, 2023

Using ASAN options, I tried to fix buffer overflows with rDNS tags.

Navigating through the original source code is not easy and even if I did not find where the actual overflow happens, I think the proposed workaround would/could be acceptable.
Adding 4 more bytes than normally necessary when allocating memory storing rDNS name seems to bypass the problem.
So maybe someone would be interested in testing this in another context or even finding a better solution.

Here comes a potential patch to solve the problem.
reverseDNS.patch

image

Here comes a sample .sh script that adds many rDNS tags to a .m4a file converted from a .flac file.
try.sh.zip

image

Here comes the ASAN report without the patch.
asan.txt

@github-actions
Copy link

Thanks for filing an issue! Please note that this project is only passively maintained, so your best bet for getting an issue resolved is through a pull request that is easy to verify! Please read this for more information.

@miraclx
Copy link
Contributor

miraclx commented Jan 13, 2023

Cross-referencing prior context: #44

@wez
Copy link
Owner

wez commented Jan 13, 2023

Thanks! Could you include the ASAN overflows you see without this patch in place, and turn it into a PR so that it is reviewable?
I'm troubled by the comments about +4 padding and there's a comment in the picture above that has a hardcoded 16 that doesn't appear in the before changes; is that really 12 + 4 bytes of padding?
I think getting to the bottom of that magic #4 value would be wise before assuming that this is fixed, as it feels a bit funky.

@ocococococ
Copy link
Author

ASAN report uploaded above.
PR #63 created.

By the way, much better, no more +4 magic number in proposed patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants