Re: CVE-2015-7519

2016-04-26 Thread Raphael Geissert
On 26 April 2016 at 10:27, Linus van Geuns  wrote:
> On Tue, Apr 26, 2016 at 10:08 AM, Raphael Geissert  wrote:
>> On 19 February 2016 at 09:35, Linus van Geuns  wrote:
>>> On Thu, Feb 18, 2016 at 8:35 PM, Thorsten Alteholz  
>>> wrote:
>>>> On irc you wrote:
>>>> 15:05 < Nirkus> have some old redmine running on squeeze-lts (yeah..) and 
>>>> since the update yesterday the following redmine code bails out with 
>>>> "private method `split' called for nil:NilClass" at the following line:
>>>> 15:06 < Nirkus> @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : 
>>>> (@env['REQUEST_URI'].split('?', 2)[1] || '')
>>>>
>>>> In CVE-2015-7519[1] it was detected, that it is possible to obtain
>>>> unauthorized access if you send http variables with "_" instead of "-". 
>>>> More information can be found here[2]. As a solution it was proposed to 
>>>> simply filter out all variables containing an "_". This was already done 
>>>> in mod_cgi of apache[3] and now I applied a similar patch to 
>>>> libapache2-mod-passenger as well.
>>>>
>>>> Unfortunately there seems to be software that relies on underscores in 
>>>> variable names. So if you need such variables you might want to use the 
>>>> workaround for apache, described in[2].
>>>
>>> I am only scratching the surface of Ruby, Passenger, Rack/Rails and
>>> Redminde, so corrections and clarifications welcome. :)
>>>
>> [...]
>>>
>>> I am not sure whether REQUEST_URI and QUERY_STRING are actually passed
>>> as per-request env. variables by Passenger or added to the env hash by
>>> Rack/Rails.
>>> Still, this looks like a regression to me, since it removes previously
>>> available variables, which should not be in scope of CVE-2015-7519.
>>
>> It is a regression, there's no way for applications using
>> mod_passenger to work after the latest update. Not only did the update
>> switch to a native package and drop some documentation, but it broke
>> the module.
>>
>> Granted, the package is safer now that it doesn't work.
>
> Yeah, granted
>
> "We" are no longer affected by this regression since the affected
> Redmine instance has been migrated to a current release running on
> Debian jessie.
> So, thank you for the incentive to do the right thing.

FWIW, attached patch should do it. It can be applied on top of the
non-LTS version of passenger.

Cheers,
-- 
Raphael Geissert
Index: passenger-2.2.11debian/ext/apache2/Hooks.cpp
===
--- passenger-2.2.11debian.orig/ext/apache2/Hooks.cpp	2016-04-26 11:29:20.620915082 +0200
+++ passenger-2.2.11debian/ext/apache2/Hooks.cpp	2016-04-26 14:20:05.944237319 +0200
@@ -779,6 +779,30 @@ private:
 	char *lookupEnv(request_rec *r, const char *name) {
 		return lookupName(r->subprocess_env, name);
 	}
+
+	static bool
+	isAlphaNum(char ch) {
+		return (ch >= '0' && ch <= '9') || (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z');
+	}
+
+	/**
+	 * For CGI, alphanum headers with optional dashes are mapped to UPP3R_CAS3. This
+	 * function can be used to reject non-alphanum/dash headers that would end up with
+	 * the same mapping (e.g. upp3r_cas3 and upp3r-cas3 would end up the same, and
+	 * potentially collide each other in the receiving application). This is
+	 * used to fix CVE-2015-7519.
+	 */
+	static bool
+	containsNonAlphaNumDash(const char *s) {
+		size_t len = strlen(s);
+		for (size_t i = 0; i < len; i++) {
+			const char start = s[i];
+			if (start != '-' && !isAlphaNum(start)) {
+return true;
+			}
+		}
+		return false;
+	}
 	
 	void inline addHeader(apr_table_t *table, const char *name, const char *value) {
 		if (name != NULL && value != NULL) {
@@ -847,7 +871,7 @@ private:
 		hdrs_arr = apr_table_elts(r->headers_in);
 		hdrs = (apr_table_entry_t *) hdrs_arr->elts;
 		for (i = 0; i < hdrs_arr->nelts; ++i) {
-			if (hdrs[i].key) {
+			if (hdrs[i].key && !containsNonAlphaNumDash(hdrs[i].key)) {
 addHeader(headers, http2env(r->pool, hdrs[i].key), hdrs[i].val);
 			}
 		}


Re: CVE-2015-7519

2016-04-26 Thread Raphael Geissert
Hi,

On 19 February 2016 at 09:35, Linus van Geuns  wrote:
> On Thu, Feb 18, 2016 at 8:35 PM, Thorsten Alteholz  wrote:
>> On irc you wrote:
>> 15:05 < Nirkus> have some old redmine running on squeeze-lts (yeah..) and 
>> since the update yesterday the following redmine code bails out with 
>> "private method `split' called for nil:NilClass" at the following line:
>> 15:06 < Nirkus> @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : 
>> (@env['REQUEST_URI'].split('?', 2)[1] || '')
>>
>> In CVE-2015-7519[1] it was detected, that it is possible to obtain
>> unauthorized access if you send http variables with "_" instead of "-". More 
>> information can be found here[2]. As a solution it was proposed to simply 
>> filter out all variables containing an "_". This was already done in mod_cgi 
>> of apache[3] and now I applied a similar patch to libapache2-mod-passenger 
>> as well.
>>
>> Unfortunately there seems to be software that relies on underscores in 
>> variable names. So if you need such variables you might want to use the 
>> workaround for apache, described in[2].
>
> I am only scratching the surface of Ruby, Passenger, Rack/Rails and
> Redminde, so corrections and clarifications welcome. :)
>
[...]
>
> I am not sure whether REQUEST_URI and QUERY_STRING are actually passed
> as per-request env. variables by Passenger or added to the env hash by
> Rack/Rails.
> Still, this looks like a regression to me, since it removes previously
> available variables, which should not be in scope of CVE-2015-7519.

It is a regression, there's no way for applications using
mod_passenger to work after the latest update. Not only did the update
switch to a native package and drop some documentation, but it broke
the module.

Granted, the package is safer now that it doesn't work.

Regards,
-- 
Raphael Geissert



Re: CVE-2015-7519

2016-04-26 Thread Linus van Geuns
Hi,



On Tue, Apr 26, 2016 at 10:08 AM, Raphael Geissert  wrote:
> Hi,
>
> On 19 February 2016 at 09:35, Linus van Geuns  wrote:
>> On Thu, Feb 18, 2016 at 8:35 PM, Thorsten Alteholz  
>> wrote:
>>> On irc you wrote:
>>> 15:05 < Nirkus> have some old redmine running on squeeze-lts (yeah..) and 
>>> since the update yesterday the following redmine code bails out with 
>>> "private method `split' called for nil:NilClass" at the following line:
>>> 15:06 < Nirkus> @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : 
>>> (@env['REQUEST_URI'].split('?', 2)[1] || '')
>>>
>>> In CVE-2015-7519[1] it was detected, that it is possible to obtain
>>> unauthorized access if you send http variables with "_" instead of "-". 
>>> More information can be found here[2]. As a solution it was proposed to 
>>> simply filter out all variables containing an "_". This was already done in 
>>> mod_cgi of apache[3] and now I applied a similar patch to 
>>> libapache2-mod-passenger as well.
>>>
>>> Unfortunately there seems to be software that relies on underscores in 
>>> variable names. So if you need such variables you might want to use the 
>>> workaround for apache, described in[2].
>>
>> I am only scratching the surface of Ruby, Passenger, Rack/Rails and
>> Redminde, so corrections and clarifications welcome. :)
>>
> [...]
>>
>> I am not sure whether REQUEST_URI and QUERY_STRING are actually passed
>> as per-request env. variables by Passenger or added to the env hash by
>> Rack/Rails.
>> Still, this looks like a regression to me, since it removes previously
>> available variables, which should not be in scope of CVE-2015-7519.
>
> It is a regression, there's no way for applications using
> mod_passenger to work after the latest update. Not only did the update
> switch to a native package and drop some documentation, but it broke
> the module.
>
> Granted, the package is safer now that it doesn't work.

Yeah, granted

"We" are no longer affected by this regression since the affected
Redmine instance has been migrated to a current release running on
Debian jessie.
So, thank you for the incentive to do the right thing.

Gruß, Linus



Re: CVE-2015-7519

2016-02-19 Thread Linus van Geuns
Hi Thorsten,

On Thu, Feb 18, 2016 at 8:35 PM, Thorsten Alteholz  wrote:
>
> [..]
> On irc you wrote:
> 15:05 < Nirkus> have some old redmine running on squeeze-lts (yeah..) and 
> since the update yesterday the following redmine code bails out with "private 
> method `split' called for nil:NilClass" at the following line:
> 15:06 < Nirkus> @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : 
> (@env['REQUEST_URI'].split('?', 2)[1] || '')
> [..]
>
> In CVE-2015-7519[1] it was detected, that it is possible to obtain
> unauthorized access if you send http variables with "_" instead of "-". More 
> information can be found here[2]. As a solution it was proposed to simply 
> filter out all variables containing an "_". This was already done in mod_cgi 
> of apache[3] and now I applied a similar patch to libapache2-mod-passenger as 
> well.
>
> Unfortunately there seems to be software that relies on underscores in 
> variable names. So if you need such variables you might want to use the 
> workaround for apache, described in[2].

I am only scratching the surface of Ruby, Passenger, Rack/Rails and
Redminde, so corrections and clarifications welcome. :)

This is my interprtation of the blog entry[1] for CVE-2015-7519:
- In order to make HTTP headers of a request available as per-request
environment variables, Passenger
  * prefixes the header names with "HTTP_"
  * converts those names to upper case
  * converts all non-alphanumeric characters in header names to underscore ("_")
- This behavior allows attackers to pass in per-request env. variables
that look like trusted, internal headers to applications
  (header names "X-User" and "X~User" both get converted to variable
name "HTTP_X_USER")

Judging from my above interpretation, CVE-2015-7519 should be
mitigated by discarding all request headers with names containing
other characters than alphanumeric and hyphen ("-").

This is my current understanding of the issue with out legacy Redmine system:
- After applying the update of libapache2-mod-passenger to mitigate
CVE-2015-7519, libactionpack-ruby1.8 fails to access either of
parameters 'QUERY_STRING' and 'REQUEST_URI'
- The above-mentioned parameters are retrieved from the Rack/Rails
request environment "hash"
- The Rails/Rack requests env. "hash"[L1] gets populated based on
per-request environment variables received from Passenger
- 'QUERY_STRING' and 'REQUEST_URI' are per-request env. variables
describing the request passed on py Passenger
- Both above-mentioned parameters are not prefixed by "HTTP_" and
therefore not in scope of CVE-2015-7519

I am not sure whether REQUEST_URI and QUERY_STRING are actually passed
as per-request env. variables by Passenger or added to the env hash by
Rack/Rails.
Still, this looks like a regression to me, since it removes previously
available variables, which should not be in scope of CVE-2015-7519.

Gruß, Linus

[..]
>
> [1] https://security-tracker.debian.org/tracker/CVE-2015-7519
> [2] https://blog.phusion.nl/2015/12/07/cve-2015-7519/
> [3] 
> http://mail-archives.apache.org/mod_mbox/httpd-dev/201010.mbox/<201010121630.19406@apache.org>
>

[L1]https://quickleft.com/blog/understanding-rack-apps-and-middleware/#the-environment-hash



CVE-2015-7519

2016-02-18 Thread Thorsten Alteholz

Hi Linus,

as others might be interested in the answer as well, I also send it to 
debian-lts@.


On irc you wrote:
15:05 < Nirkus> have some old redmine running on squeeze-lts (yeah..) and 
since the update yesterday the following redmine code bails out with 
"private method `split' called for nil:NilClass" at the following line:
15:06 < Nirkus> @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : 
(@env['REQUEST_URI'].split('?', 2)[1] || '')
15:11 < Nirkus> ah, the code is actually from: libactionpack-ruby1.8: 
/usr/lib/ruby/1.8/action_controller/request.rb
15:51 < Nirkus> downgrading to libapache2-mod-passenger=2.2.11debian-2 
fixes the above issue...




In CVE-2015-7519[1] it was detected, that it is possible to obtain
unauthorized access if you send http variables with "_" instead of "-". 
More information can be found here[2]. As a solution it was proposed to 
simply filter out all variables containing an "_". This was already done 
in mod_cgi of apache[3] and now I applied a similar patch to 
libapache2-mod-passenger as well.


Unfortunately there seems to be software that relies on underscores in 
variable names. So if you need such variables you might want to use the 
workaround for apache, described in[2].


  Thorsten



[1] https://security-tracker.debian.org/tracker/CVE-2015-7519
[2] https://blog.phusion.nl/2015/12/07/cve-2015-7519/
[3] 
http://mail-archives.apache.org/mod_mbox/httpd-dev/201010.mbox/<201010121630.19406@apache.org>