[Asterisk-code-review] AST-2018-009: Fix crash processing websocket HTTP Upgrade re... (asterisk[15])

[Richard Mudgett]

Richard Mudgett has submitted this change and it was merged. ( https://gerrit.asterisk.org/10218 )

Change subject: AST-2018-009: Fix crash processing websocket HTTP Upgrade requests
......................................................................

AST-2018-009: Fix crash processing websocket HTTP Upgrade requests

The HTTP request processing in res_http_websocket allocates additional
space on the stack for various headers received during an Upgrade request.
An attacker could send a specially crafted request that causes this code
to overflow the stack, resulting in a crash.

* No longer allocate memory from the stack in a loop to parse the header
values.  NOTE: There is a slight API change when using the passed in
strings as is.  We now require the passed in strings to no longer have
leading or trailing whitespace.  This isn't a problem as the only callers
have already done this before passing the strings to the affected
function.

ASTERISK-28013 #close

Change-Id: Ia564825a8a95e085fd17e658cb777fe1afa8091a
---
M res/res_http_websocket.c
1 file changed, 14 insertions(+), 11 deletions(-)

Approvals:
  Sean Bright: Looks good to me, but someone else must approve
  Jenkins2: Verified
  Richard Mudgett: Looks good to me, approved; Approved for Submit



diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index aaaba7d..e7ce830 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -758,7 +758,8 @@
 int AST_OPTIONAL_API_NAME(ast_websocket_uri_cb)(struct ast_tcptls_session_instance *ser, const struct ast_http_uri *urih, const char *uri, enum ast_http_method method, struct ast_variable *get_vars, struct ast_variable *headers)
 {
 	struct ast_variable *v;
-	char *upgrade = NULL, *key = NULL, *key1 = NULL, *key2 = NULL, *protos = NULL, *requested_protocols = NULL, *protocol = NULL;
+	const char *upgrade = NULL, *key = NULL, *key1 = NULL, *key2 = NULL, *protos = NULL;
+	char *requested_protocols = NULL, *protocol = NULL;
 	int version = 0, flags = 1;
 	struct ast_websocket_protocol *protocol_handler = NULL;
 	struct ast_websocket *session;
@@ -777,16 +778,15 @@
 	/* Get the minimum headers required to satisfy our needs */
 	for (v = headers; v; v = v->next) {
 		if (!strcasecmp(v->name, "Upgrade")) {
-			upgrade = ast_strip(ast_strdupa(v->value));
+			upgrade = v->value;
 		} else if (!strcasecmp(v->name, "Sec-WebSocket-Key")) {
-			key = ast_strip(ast_strdupa(v->value));
+			key = v->value;
 		} else if (!strcasecmp(v->name, "Sec-WebSocket-Key1")) {
-			key1 = ast_strip(ast_strdupa(v->value));
+			key1 = v->value;
 		} else if (!strcasecmp(v->name, "Sec-WebSocket-Key2")) {
-			key2 = ast_strip(ast_strdupa(v->value));
+			key2 = v->value;
 		} else if (!strcasecmp(v->name, "Sec-WebSocket-Protocol")) {
-			requested_protocols = ast_strip(ast_strdupa(v->value));
-			protos = ast_strdupa(requested_protocols);
+			protos = v->value;
 		} else if (!strcasecmp(v->name, "Sec-WebSocket-Version")) {
 			if (sscanf(v->value, "%30d", &version) != 1) {
 				version = 0;
@@ -800,7 +800,7 @@
 			ast_sockaddr_stringify(&ser->remote_address));
 		ast_http_error(ser, 426, "Upgrade Required", NULL);
 		return 0;
-	} else if (ast_strlen_zero(requested_protocols)) {
+	} else if (ast_strlen_zero(protos)) {
 		/* If there's only a single protocol registered, and the
 		 * client doesn't specify what protocol it's using, go ahead
 		 * and accept the connection */
@@ -821,9 +821,12 @@
 		return 0;
 	}
 
-	/* Iterate through the requested protocols trying to find one that we have a handler for */
-	while (!protocol_handler && (protocol = strsep(&requested_protocols, ","))) {
-		protocol_handler = ao2_find(server->protocols, ast_strip(protocol), OBJ_KEY);
+	if (!protocol_handler && protos) {
+		requested_protocols = ast_strdupa(protos);
+		/* Iterate through the requested protocols trying to find one that we have a handler for */
+		while (!protocol_handler && (protocol = strsep(&requested_protocols, ","))) {
+			protocol_handler = ao2_find(server->protocols, ast_strip(protocol), OBJ_KEY);
+		}
 	}
 
 	/* If no protocol handler exists bump this back to the requester */

-- 
To view, visit https://gerrit.asterisk.org/10218
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia564825a8a95e085fd17e658cb777fe1afa8091a
Gerrit-Change-Number: 10218
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180920/2a55c338/attachment-0001.html>