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

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

Successfully merging this pull request may close these issues.

2 participants