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

Use execvp() instead of execvpe() #282

Merged
merged 1 commit into from
Aug 9, 2018

Conversation

khorben
Copy link
Contributor

@khorben khorben commented Aug 6, 2018

execvp() is (usually) equivalent to execvpe(), without enforcing any
change to the environment. However, unlike execvp(), execvpe() is not
standardized by POSIX, and may therefore not be available nor detected
when configuring the project (like on NetBSD).

No place could be found in fwknop to be using execvpe() and changing the
environment. Therefore it seems only logical (and safer) to use execvp()
instead.

This also updates the tests to reflect this change.

execvp() is (usually) equivalent to execvpe(), without enforcing any
change to the environment. However, unlike execvp(), execvpe() is not
standardized by POSIX, and may therefore not be available nor detected
when configuring the project (like on NetBSD).

No place could be found in fwknop to be using execvpe() and changing the
environment. Therefore it seems only logical (and safer) to use execvp()
instead.

This also updates the tests to reflect this change.
@mrash mrash self-assigned this Aug 7, 2018
@mrash
Copy link
Owner

mrash commented Aug 7, 2018

Question on this - for a call like 'execvpe(wget_argv[0], wget_argv, (char * const *)NULL);', the environment is explicitly being set to NULL so that it cannot contain any potentially malicious content. Is there an advantage to keeping this strategy? That is, does switching to execvp() imply that a non-NULL environment will influence how the exec'd binary will behave?

@khorben
Copy link
Contributor Author

khorben commented Aug 7, 2018

First, I misremembered that execvpe(file, argv, NULL) was equivalent to execvpe(file, argv, environ) and this is wrong. If we consider Linux, the environment is effectively set to be empty. However, the manual page says:

On Linux, argv and envp can
be specified as NULL. In both cases, this has the same effect as
specifying the argument as a pointer to a list containing a single null
pointer. Do not take advantage of this misfeature! It is nonstandard
and nonportable: on most other UNIX systems doing this will result in
an error (EFAULT).

(emphasis originally there)

Without even considering this, I do not see how the environment can be malicious. Commands are started either by the server or the client, but in both cases the user controls the environment. If it is already corrupted somehow, I would say there is a different, bigger problem, independently of fwknop. Actually, it might also be considered buggy to clear the environment free from environment variables like http_proxy and the like in the case of wget for instance.

@mrash
Copy link
Owner

mrash commented Aug 9, 2018

Ok, that is a good argument. Merging.

@mrash mrash merged commit 71b8f22 into mrash:master Aug 9, 2018
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