You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by hu...@apache.org on 2014/03/27 12:22:34 UTC
svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Author: humbedooh
Date: Thu Mar 27 11:22:33 2014
New Revision: 1582264
URL: http://svn.apache.org/r1582264
Log:
mod_lua: Prevent HTTP Response Splitting by not allowing tables in the request_rec to be set with values containing newlines.
Modified:
httpd/httpd/branches/2.4.x/CHANGES
httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
Modified: httpd/httpd/branches/2.4.x/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1582264&r1=1582263&r2=1582264&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Thu Mar 27 11:22:33 2014
@@ -12,6 +12,10 @@ Changes with Apache 2.4.10
from causing response splitting.
[Daniel Gruno, Felipe Daragon <filipe syhunt com>]
+ *) mod_lua: Disallow newlines in table values inside the request_rec,
+ to prevent HTTP Response Splitting via tainted headers.
+ [Daniel Gruno, Felipe Daragon <filipe syhunt com>]
+
Changes with Apache 2.4.9
*) mod_ssl: Work around a bug in some older versions of OpenSSL that
Modified: httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c?rev=1582264&r1=1582263&r2=1582264&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c (original)
+++ httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c Thu Mar 27 11:22:33 2014
@@ -40,6 +40,13 @@ static int lua_table_set(lua_State *L)
const char *key = luaL_checkstring(L, 2);
const char *val = luaL_checkstring(L, 3);
+ /* Prevent response/header splitting by not allowing newlines in tables.
+ * At this stage, we don't have the request_rec handy, and we can't change
+ * a const char*, so we'll redirect to a standard error value instead.
+ */
+ if (ap_strchr_c(val, '\n')) {
+ val = "[ERROR: Value contains newline, ignored.]";
+ }
apr_table_set(t, key, val);
return 0;
}
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES modules/lua/lua_apr.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Thu, Mar 27, 2014 at 8:06 AM, Daniel Gruno <ru...@cord.dk> wrote:
> FYI, I have implemented some restrictions and alterations to mod_lua, to
> prevent HTTP Response Splitting in cases where users fail to properly
> check their output or think mod_lua takes care of everything all by itself.
>
> This is not a security flaw in mod_lua itself, but rather a scripting
> accident waiting to happen, that I think is best handled by making
> mod_lua take some extra precautions, much like we have and recommend
> using prepared statements with our database API, to prevent SQL
> injection attacks, instead of the users having to escape values themselves.
>
> If anyone thinks this is a more serious matter (and requires a CVE
> or..?), please let me/us know.
>
IMO Lua scripts would be treated similarly to C-language extensions -- code
that httpd must be able to trust. As you noticed, core does not protect
against a module (mod_lua or anything else) inserting broken values. A CVE
would apply to a broken script or module that inserts broken values.
> With regards,
> Daniel.
>
> On 03/27/2014 12:22 PM, humbedooh@apache.org wrote:
> > Author: humbedooh
> > Date: Thu Mar 27 11:22:33 2014
> > New Revision: 1582264
> >
> > URL: http://svn.apache.org/r1582264
> > Log:
> > mod_lua: Prevent HTTP Response Splitting by not allowing tables in the
> request_rec to be set with values containing newlines.
> >
> > Modified:
> > httpd/httpd/branches/2.4.x/CHANGES
> > httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
> >
> > Modified: httpd/httpd/branches/2.4.x/CHANGES
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1582264&r1=1582263&r2=1582264&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
> > +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Thu Mar 27 11:22:33 2014
> > @@ -12,6 +12,10 @@ Changes with Apache 2.4.10
> > from causing response splitting.
> > [Daniel Gruno, Felipe Daragon <filipe syhunt com>]
> >
> > + *) mod_lua: Disallow newlines in table values inside the request_rec,
> > + to prevent HTTP Response Splitting via tainted headers.
> > + [Daniel Gruno, Felipe Daragon <filipe syhunt com>]
> > +
> > Changes with Apache 2.4.9
> >
> > *) mod_ssl: Work around a bug in some older versions of OpenSSL that
> >
> > Modified: httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c?rev=1582264&r1=1582263&r2=1582264&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c (original)
> > +++ httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c Thu Mar 27 11:22:33
> 2014
> > @@ -40,6 +40,13 @@ static int lua_table_set(lua_State *L)
> > const char *key = luaL_checkstring(L, 2);
> > const char *val = luaL_checkstring(L, 3);
> >
> > + /* Prevent response/header splitting by not allowing newlines in
> tables.
> > + * At this stage, we don't have the request_rec handy, and we can't
> change
> > + * a const char*, so we'll redirect to a standard error value
> instead.
> > + */
> > + if (ap_strchr_c(val, '\n')) {
> > + val = "[ERROR: Value contains newline, ignored.]";
> > + }
> > apr_table_set(t, key, val);
> > return 0;
> > }
> >
> >
>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
http://edjective.org/
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Daniel Gruno <ru...@cord.dk>.
On 03/27/2014 01:24 PM, Jeff Trawick wrote:
>
> Just remove it?
>
> And what about other control characters such as \r, or generally any
> character/byte sequence that is not valid here?
>
>
(My mail server is refusing my emails atm, so I'm not sure whether this
gets through *crosses fingers*)
If we just remove it, people might think it didn't work and complain
that setting headers don't work - I think it's better to _somehow_ let
the author know what's happening, and this was all I could think of,
taking into consideration how headers are currently being set in
mod_lua. Logging an error or sending a warning would require a rewrite
of how this happens, and I'm pressed for time as it is, so I plugged the
leak instead of building a new boat.
As I said to Nick, If anyone comes up with a better way of handling
this, feel free to fix it - I certainly won't get mad if others start to
work on mod_lua ;-)
With regards,
Daniel.
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES modules/lua/lua_apr.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Thu, Mar 27, 2014 at 8:21 AM, Daniel Gruno <ru...@cord.dk> wrote:
> On 03/27/2014 01:15 PM, Nick Kew wrote:
> > On Thu, 2014-03-27 at 13:06 +0100, Daniel Gruno wrote:
> >> FYI, I have implemented some restrictions and alterations to mod_lua, to
> >> prevent HTTP Response Splitting in cases where users fail to properly
> >> check their output or think mod_lua takes care of everything all by
> itself.
> >
> > Hmmm ...
> >
> >>> + if (ap_strchr_c(val, '\n')) {
> >>> + val = "[ERROR: Value contains newline, ignored.]";
> >>> + }
> >>> apr_table_set(t, key, val);
> >>> return 0;
> >>> }
> >
> > Is that exactly what you meant to do? You've set val
> > to something that conceivably be a legitimate value and
> > continued normally.
> >
> > Why not instead strip the newline character and log a warning?
> >
> You can't log a warning or strip the newline;
> 1) it's a const char* so magical things will happen if you edit it(?)
> 2) we don't have a pool handy to make a new string without the newline
> or log an error.
>
> As I said in the commit msg in trunk, it's an ugly hack, and if someone
> finds a more clever way of solving it, I'm all ears :) Maybe I'm
> forgetting something entirely obvious, who knows.
>
> With regards,
> Daniel.
>
Just remove it?
And what about other control characters such as \r, or generally any
character/byte sequence that is not valid here?
--
Born in Roswell... married an alien...
http://emptyhammock.com/
http://edjective.org/
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Daniel Gruno <ru...@cord.dk>.
On 03/27/2014 01:38 PM, Nick Kew wrote:
> On Thu, 2014-03-27 at 13:21 +0100, Daniel Gruno wrote:
>
>> You can't log a warning or strip the newline;
>> 1) it's a const char* so magical things will happen if you edit it(?)
>> 2) we don't have a pool handy to make a new string without the newline
>> or log an error.
>>
>> As I said in the commit msg in trunk, it's an ugly hack, and if someone
>> finds a more clever way of solving it, I'm all ears :) Maybe I'm
>> forgetting something entirely obvious, who knows.
>
> If you're going to do security checking, you need to work through it.
> The absence of a pool suggests this may be the wrong place for it.
>
> Perhaps what needs to happen is you set a "bogus-value seen" flag,
> then check it at an appropriate point when you can manipulate
> values or abort requests, and log errors? Would that require
> excessive shoehorning?
>
I follow your idea completely, but the way mod_lua is written, it's just
not possible at the moment to do what you and I really want. I'll look
into whether we can rewrite this completely or find another way around
it (we need to get a hold of the request_rec in a place where it's just
not being passed to us), but for the time being, I wanted to prevent
cases where users are not paranoid enough about what they output, and so
I implemented _what could be done with the current setup_.
I will definitely look into a prettier fix.
With regards,
Daniel.
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Nick Kew <ni...@webthing.com>.
On Thu, 2014-03-27 at 13:21 +0100, Daniel Gruno wrote:
> You can't log a warning or strip the newline;
> 1) it's a const char* so magical things will happen if you edit it(?)
> 2) we don't have a pool handy to make a new string without the newline
> or log an error.
>
> As I said in the commit msg in trunk, it's an ugly hack, and if someone
> finds a more clever way of solving it, I'm all ears :) Maybe I'm
> forgetting something entirely obvious, who knows.
If you're going to do security checking, you need to work through it.
The absence of a pool suggests this may be the wrong place for it.
Perhaps what needs to happen is you set a "bogus-value seen" flag,
then check it at an appropriate point when you can manipulate
values or abort requests, and log errors? Would that require
excessive shoehorning?
--
Nick Kew
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Daniel Gruno <ru...@cord.dk>.
On 03/27/2014 01:15 PM, Nick Kew wrote:
> On Thu, 2014-03-27 at 13:06 +0100, Daniel Gruno wrote:
>> FYI, I have implemented some restrictions and alterations to mod_lua, to
>> prevent HTTP Response Splitting in cases where users fail to properly
>> check their output or think mod_lua takes care of everything all by itself.
>
> Hmmm ...
>
>>> + if (ap_strchr_c(val, '\n')) {
>>> + val = "[ERROR: Value contains newline, ignored.]";
>>> + }
>>> apr_table_set(t, key, val);
>>> return 0;
>>> }
>
> Is that exactly what you meant to do? You've set val
> to something that conceivably be a legitimate value and
> continued normally.
>
> Why not instead strip the newline character and log a warning?
>
You can't log a warning or strip the newline;
1) it's a const char* so magical things will happen if you edit it(?)
2) we don't have a pool handy to make a new string without the newline
or log an error.
As I said in the commit msg in trunk, it's an ugly hack, and if someone
finds a more clever way of solving it, I'm all ears :) Maybe I'm
forgetting something entirely obvious, who knows.
With regards,
Daniel.
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Nick Kew <ni...@webthing.com>.
On Thu, 2014-03-27 at 13:06 +0100, Daniel Gruno wrote:
> FYI, I have implemented some restrictions and alterations to mod_lua, to
> prevent HTTP Response Splitting in cases where users fail to properly
> check their output or think mod_lua takes care of everything all by itself.
Hmmm ...
> > + if (ap_strchr_c(val, '\n')) {
> > + val = "[ERROR: Value contains newline, ignored.]";
> > + }
> > apr_table_set(t, key, val);
> > return 0;
> > }
Is that exactly what you meant to do? You've set val
to something that conceivably be a legitimate value and
continued normally.
Why not instead strip the newline character and log a warning?
--
Nick Kew
Re: svn commit: r1582264 - in /httpd/httpd/branches/2.4.x: CHANGES
modules/lua/lua_apr.c
Posted by Daniel Gruno <ru...@cord.dk>.
FYI, I have implemented some restrictions and alterations to mod_lua, to
prevent HTTP Response Splitting in cases where users fail to properly
check their output or think mod_lua takes care of everything all by itself.
This is not a security flaw in mod_lua itself, but rather a scripting
accident waiting to happen, that I think is best handled by making
mod_lua take some extra precautions, much like we have and recommend
using prepared statements with our database API, to prevent SQL
injection attacks, instead of the users having to escape values themselves.
If anyone thinks this is a more serious matter (and requires a CVE
or..?), please let me/us know.
With regards,
Daniel.
On 03/27/2014 12:22 PM, humbedooh@apache.org wrote:
> Author: humbedooh
> Date: Thu Mar 27 11:22:33 2014
> New Revision: 1582264
>
> URL: http://svn.apache.org/r1582264
> Log:
> mod_lua: Prevent HTTP Response Splitting by not allowing tables in the request_rec to be set with values containing newlines.
>
> Modified:
> httpd/httpd/branches/2.4.x/CHANGES
> httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
>
> Modified: httpd/httpd/branches/2.4.x/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1582264&r1=1582263&r2=1582264&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
> +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Thu Mar 27 11:22:33 2014
> @@ -12,6 +12,10 @@ Changes with Apache 2.4.10
> from causing response splitting.
> [Daniel Gruno, Felipe Daragon <filipe syhunt com>]
>
> + *) mod_lua: Disallow newlines in table values inside the request_rec,
> + to prevent HTTP Response Splitting via tainted headers.
> + [Daniel Gruno, Felipe Daragon <filipe syhunt com>]
> +
> Changes with Apache 2.4.9
>
> *) mod_ssl: Work around a bug in some older versions of OpenSSL that
>
> Modified: httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c?rev=1582264&r1=1582263&r2=1582264&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/lua/lua_apr.c Thu Mar 27 11:22:33 2014
> @@ -40,6 +40,13 @@ static int lua_table_set(lua_State *L)
> const char *key = luaL_checkstring(L, 2);
> const char *val = luaL_checkstring(L, 3);
>
> + /* Prevent response/header splitting by not allowing newlines in tables.
> + * At this stage, we don't have the request_rec handy, and we can't change
> + * a const char*, so we'll redirect to a standard error value instead.
> + */
> + if (ap_strchr_c(val, '\n')) {
> + val = "[ERROR: Value contains newline, ignored.]";
> + }
> apr_table_set(t, key, val);
> return 0;
> }
>
>