On 26 April 2016 at 10:27, Linus van Geuns <li...@vangeuns.name> wrote: > On Tue, Apr 26, 2016 at 10:08 AM, Raphael Geissert <atom...@gmail.com> wrote: >> On 19 February 2016 at 09:35, Linus van Geuns <li...@vangeuns.name> wrote: >>> On Thu, Feb 18, 2016 at 8:35 PM, Thorsten Alteholz <deb...@alteholz.de> >>> 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); } }