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

sanitize %ENV #607

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sanitize %ENV #607

wants to merge 1 commit into from

Conversation

rurban
Copy link

@rurban rurban commented Dec 18, 2017

See e.g. for a nice remote CGI exploit via
LD_PRELOAD: https://www.elttam.com.au/blog/goahead/

The env_sanitizer should probably go into a helper function.
That's up to you to decide where to.

I also haven't checked what Apache how does its ENV sanitizing.
I just hope it does.

See e.g. for a nice remote CGI exploit via
LD_PRELOAD: https://www.elttam.com.au/blog/goahead/

The env_sanitizer should probably go into a helper function.
That's up to you to decide where to.

I also haven't checked what Apache how does its ENV sanitizing.
I just hope it does.
@miyagawa
Copy link
Member

miyagawa commented Dec 18, 2017

The link you posted seems to describe a vulnerability in a particular web server implementation, and the existence of that CVE doesn't mean all CGI script implementation need to be patched. As you wrote, if Apache does not sanitize the external inputs and allows an attacker to change LD_PRELOAD, that's a vulnerability in Apache that needs to be fixed there.

That said it doesn't stop you from protecting from such a vulnerability in the CGI implementation side, and from hereon I assume that's your intent here.

For Plack::Handler::CGI, your patch deletes HTTP_AUTHORIZATION and REMOTE_HOST etc. which I do believe will break the existing applications which implements HTTP basic authentication.

I do not mind strongly about deleting LD_* environment names for precautions, but it could also break a valid use case where you host your CGI script with a pinned LD_PRELOAD environment variable to run, say, mysql client from a specific LD path.

For Plack::App::WrapCGI, it seems a bit more relevant since it is on the CGI executor side. However the patch again seems to have the same issue about the deletion of REMOTE_HOST and HTTP_AUTHORIZATION, and deletion of other variables could also break if it's explicitly set in the parent runner environment.

I think what we should look at instead is CGI::Emulate::PSGI to see if there's a possibility of turning user-supplied headers or parameters into an environment variable outside those prefixed with HTTP_.

I'm not convinced if this patch is necessary, or at least would break existing applications if applied without any optional behaviors to keep the current variables.

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