Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix buffer overflows when argv[0] or env variables are longer than PATH_MAX #38

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

l-pt
Copy link

@l-pt l-pt commented Jul 5, 2024

When the runtime is invoked, it copies (strcpy) argv[0] or the TARGET_APPIMAGE environemnt variable into a buffer that has a dimension of PATH_MAX, and same thing with the TMPDIR env variable.
If the values saved in argv[0] or those environemnt variables are longer than PATH_MAX, the runtime currently segfaults due to buffer overflow.

This patch introduces a simple check that exits with an error message if that happens instead of causing a segfault. This is obtained by changing strcpy to memccpy, a POSIX standard function that makes it easy to detect truncation (check if the return value is NULL).

@probonopd
Copy link
Member

Thanks @l-pt.

Could the code be simplified? Maybe we could avoid redundant calls to getenv and the strcpy before memccpy. Additionally, modern C suggests using snprintf instead of strcpy and memccpy for safer and more concise string handling. What do you think?

@probonopd probonopd requested a review from TheAssassin July 8, 2024 20:00
@l-pt
Copy link
Author

l-pt commented Jul 8, 2024

Thanks for reviewing @probonopd

Could the code be simplified? Maybe we could avoid redundant calls to getenv and the strcpy before memccpy.

Right, I extracted a variable to avoid repeated getenv("TARGET_APPIMAGE") calls and initialized argv0_path so we don't have to call strcpy to copy "/proc/self/exe"

Additionally, modern C suggests using snprintf instead of strcpy and memccpy for safer and more concise string handling. What do you think?

I do not have a strong preference towards one or the other, I believe them to be about the same in this context in terms of conciseness and safety.
I initially chose memccpy because there is no "formatting" involved and because we can check if the string is too big for the buffer with a simple NULL check on the result.
As for safety, I believe memccpy to be as safe as snprintf since both functions stop copying data after the buffer length we provide.

@TheAssassin TheAssassin mentioned this pull request Aug 5, 2024
@probonopd
Copy link
Member

Pending on @TheAssassin to get to reviewing this

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

Successfully merging this pull request may close these issues.

2 participants