httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Malte S. Stretz" <>
Subject Re: [PATCH] mod_cgi: Mitigating some header injections by dropping invalid headers?
Date Mon, 22 Nov 2010 22:25:06 GMT
On Monday 18 October 2010 12:28:12 Malte S. Stretz wrote:
> On Tuesday 12 October 2010 19:49:02 Malte S. Stretz wrote:
> > On Tuesday 12 October 2010 18:13:46 William A. Rowe Jr. wrote:
> > > On 10/12/2010 10:06 AM, Dirk-Willem van Gulik wrote:
> > > > On 12 Oct 2010, at 15:30, Malte S. Stretz wrote:
> > > >> I had a quick look at the Apache source and the solution was
> > > >> simple: Just drop headers which contain any character outside the
> > > >> range [a-zA-Z0-9-]. The patch against trunk is attached.
> > > > 
> > > > This made me think of something we had a while ago; and after
> > > > checking the logs - big +1 from me!
> > > 
> > > Agreed, with a caviat... we aught to be able to toggle this for the
> > > rare but significant legacy app that requires it... which implies a
> > > per-dir flag that can override just one CGI script out of an entire
> > > server.
> > 
> > I think an option is not needed as there is a workaround.  Eg. to
> > make an Accept_Encoding header work:
> > 
> > SetEnvIfNoCase ^Accept.Encoding$ ^(.*)$ fix_header=$1
> > RequestHeader set Accept-Encoding %{fix_header}e env=fix_header
> >
> >[...]
> Attached is an updated patch which also updates the docs.  It also
> includes the commit message I tried to commit it with (didn't realize
> that there are per-project commit flags).
> Is the documented workaround good enough or should something like an
> map- broken-headers environment variable be introduced?

Time flies by... :)

As it seems like an option is preferred to a workaround, here's are a 
bunch of patches.  The first implements an option (an environment variable 
map-invalid-headers) to switch on the backwards compatibility.  It got 
delayed because I didn't write any documentation yet.  I'll do so if it 
gets accepted :)

The second patch transforms the #ifdef check against Auth headers into an 
option, too (option map-authorization-headers).  If it is accepted, I'll 
write some documentation, highlighting why it is a bad idea, too.  I added 
it because I think an option is better than an obscure #ifdef which opens 
up a rarely tested code path.

And finally, the third patch is a bit bigger, it contains a bunch of 
refactoring, moving some stuff to helper routines, getting rid of unneeded 
variables (which probably would have been optimized away anyway) and 
adding some comments.  It is probably easier to follow if you have a look 
at the single commits at Github:

If you prefer the old way (a documented workaround instead of an option), 
I rebased that code against trunk (two days old), so it applies again:


View raw message