CVE-2015-7519: Improper Input Validation (CWE-20)
value: agent/Core/Controller/SendRequest.cpp in Phusion Passenger before 4.0.60 and 5.0.x before 5.0.22, when used in Apache integration mode or in standalone mode without a filtering proxy, allows remote attackers to spoof headers passed to applications by using an _ (underscore) character instead of a - (dash) character in an HTTP header, as demonstrated by an X_User header.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 3.7
Published: 2016-01-08T19:59Z
Repository: https://github.com/phusion/passenger
Language: Other
Files changed: CHANGELOG, src/agent/Core/Controller/SendRequest.cpp
Commit date: 2015-12-07 12:17:49 +0100
Commit message: Fix CVE-2015-7519 header collision vulnerability

Diff: diff --git a/CHANGELOG b/CHANGELOG
index 494203acb..79360788f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,8 +1,9 @@
 Release 5.0.22
 --------------
 
+ * Fixes a header collision vulnerability (CVE-2015-7519, medium severity). Please see our blog for detailed vulnerability description and advisory. Thanks to the SUSE security team for reporting this issue.
  * [Apache] Fixes compatibility with Apache 2.4.17's mod_autoindex. Fix contributed by Eric Covener. Closes GH-1642.
  * [Standalone] Passenger Standalone now [accepts configuration options from environment variables](https://www.phusionpassenger.com/library/config/standalone/intro.html). This makes using Passenger Standalone significantly easier on Heroku or on systems that follow the 12-factor principle. Closes GH-1661.
  * [Standalone] The Nginx configuration template has been cleaned up. It is now significantly easier to edit the Nginx configuration template without breaking compatibility with future versions.
  * [Standalone] The `passenger start` command now performs a sanity check on the internally generated Nginx configuration file and advises you accordingly when there is a problem.
  * [Standalone] The `passenger status` and `passenger stop` commands now respect Passengerfile.json. Closes GH-1593.
diff --git a/src/agent/Core/Controller/SendRequest.cpp b/src/agent/Core/Controller/SendRequest.cpp
index 64d3d9e9d..6da59c286 100644
--- a/src/agent/Core/Controller/SendRequest.cpp
+++ b/src/agent/Core/Controller/SendRequest.cpp
@@ -202,10 +202,37 @@ Controller::sendBodyToAppWhenAppSinkIdle(Channel *_channel, unsigned int size) {
 		req->state = Request::WAITING_FOR_APP_OUTPUT;
 		req->appSource.startReading();
 	}
 }
 
+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 LString &s) {
+	const LString::Part *part = s.start;
+	while (part != NULL) {
+		for (unsigned int i = 0; i < part->size; i++) {
+			const char start = part->data[i];
+			if (start != '-' && !isAlphaNum(start)) {
+				return true;
+			}
+		}
+		part = part->next;
+	}
+	return false;
+}
+
 static void
 httpHeaderToScgiUpperCase(unsigned char *data, unsigned int size) {
 	static const boost::uint8_t toUpperMap[256] = {
 		'\0', 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, '\t',
 		'\n', 0x0b, 0x0c, '\r', 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
@@ -527,16 +554,22 @@ Controller::constructHeaderForSessionProtocol(Request *req, char * restrict buff
 		pos = appendData(pos, end, P_STATIC_STRING_WITH_NULL("upgrade"));
 	}
 
 	ServerKit::HeaderTable::Iterator it(req->headers);
 	while (*it != NULL) {
-		if ((it->header->hash == HTTP_CONTENT_LENGTH.hash()
-			|| it->header->hash == HTTP_CONTENT_TYPE.hash()
-			|| it->header->hash == HTTP_CONNECTION.hash())
-		 && (psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-type"))
-			|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-length"))
-			|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("connection"))))
+		// This header-skipping is not accounted for in determineHeaderSizeForSessionProtocol(), but
+		// since we are only reducing the size it just wastes some mem bytes.
+		if ((
+				(it->header->hash == HTTP_CONTENT_LENGTH.hash()
+						|| it->header->hash == HTTP_CONTENT_TYPE.hash()
+						|| it->header->hash == HTTP_CONNECTION.hash()
+				) && (psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-type"))
+						|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-length"))
+						|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("connection"))
+				)
+			) || containsNonAlphaNumDash(it->header->key)
+		   )
 		{
 			it.next();
 			continue;
 		}
 

Vulnerable code: 		if ((it->header->hash == HTTP_CONTENT_LENGTH.hash()
			|| it->header->hash == HTTP_CONTENT_TYPE.hash()
			|| it->header->hash == HTTP_CONNECTION.hash())
		 && (psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-type"))
			|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-length"))
			|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("connection"))))
Fixed code:  * Fixes a header collision vulnerability (CVE-2015-7519, medium severity). Please see our blog for detailed vulnerability description and advisory. Thanks to the SUSE security team for reporting this issue.
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 LString &s) {
	const LString::Part *part = s.start;
	while (part != NULL) {
		for (unsigned int i = 0; i < part->size; i++) {
			const char start = part->data[i];
			if (start != '-' && !isAlphaNum(start)) {
				return true;
			}
		}
		part = part->next;
	}
	return false;
}

		// This header-skipping is not accounted for in determineHeaderSizeForSessionProtocol(), but
		// since we are only reducing the size it just wastes some mem bytes.
		if ((
				(it->header->hash == HTTP_CONTENT_LENGTH.hash()
						|| it->header->hash == HTTP_CONTENT_TYPE.hash()
						|| it->header->hash == HTTP_CONNECTION.hash()
				) && (psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-type"))
						|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("content-length"))
						|| psg_lstr_cmp(&it->header->key, P_STATIC_STRING("connection"))
				)
			) || containsNonAlphaNumDash(it->header->key)
		   )