Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Message-ID: <1411981660.20431.62.camel@mimir.webthing.com>
Subject: Re: Proposed simple shell-shock protection
From: Nick Kew <nick@webthing.com>
To: dev@httpd.apache.org
Date: Mon, 29 Sep 2014 10:07:40 +0100
In-Reply-To: 
 <CAKQ1sVNK4ZHj8U+oA=L=qkFs5ofdpxbtpf8JPT6u52MrPy1kZw@mail.gmail.com>
References: <20140926154143.64f5f81f@baldur> <54287938.8060308@kippdata.de>
	 <1411970347.20431.49.camel@mimir.webthing.com>
	 <CAKQ1sVNK4ZHj8U+oA=L=qkFs5ofdpxbtpf8JPT6u52MrPy1kZw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

On Mon, 2014-09-29 at 09:43 +0200, Yann Ylavic wrote:
> On Mon, Sep 29, 2014 at 7:59 AM, Nick Kew <nick@webthing.com> wrote:
> > On Sun, 2014-09-28 at 23:10 +0200, Rainer Jung wrote:
> >
> >> IMHO it is a useful approach. Whan I looked at the CGI topic, I noticed
> >> that the safest thing is cleaning up in ap_create_environment(), because
> >> you can be sure to get every env var in your hands there, not only the
> >> ones coming from headers.
> >
> > The "shellshock" recipe for mod_taint takes a bit of a kitchen-sink
> > approach:
> >  - The Request headers
> >  - The Request fields that haven't always been fully sanitised
> >    and that might try to smuggle something: PATH_INFO and
> >    QUERY_STRING (r->args).
> >  - subprocess_env
> 
> mod_taint uses the header_parser hook to untaint the request
> headers/fields, but this is too early for subprocess_env.

Yes.  It's catching potential attacks in r->headers_in.
The rest is paranoia-mode afterthoughts:
PATH_INFO/QUERY_STRING because they could contain something
interesting, subprocess_env just "because it's there" (there's
a code comment about "just to be paranoid").


> As Rainer noticed httpd is probably missing a env_parser hook that
> could be called by ap_create_environment(), but still that would not
> apply to [fs]cgi (which don't use it).
> 
> So maybe rather than defining a hook that would work on the whole char
> **env, we could have one working on any key/value pair like :
> 
> AP_IMPLEMENT_HOOK_RUN_ALL(int, env_parser,
>                           (apr_pool_t *p, const char **key, const char
> **value), (p, key, value), OK, DECLINED)
> 
> This seems quite costly though...

Agreed.  A whole new hook, just to catch a bug that isn't ours!

The taint principle is to focus precisely on data coming from
the untrusted source - i.e. over the Web.  That's what mod_taint does.
If it's missing anything, it would have to be something that
happens before header_parser: for example, could a RewriteMap
invoke a shell somewhere early?

The other vector if something in HTTPD parses input
and puts the parsed data into env.  For example, if
a custom module decrypts an encrypted identity token
and puts it into the subprocess environment.
I was going to say that's beyond the scope of
anything we can/should do, but I guess REMOTE_USER
is indeed our business.

-- 
Nick Kew