res_http_websocket: Fix crash due to double freeing memory when receiving a payload...

Frames with a payload length of 0 were incorrectly handled in res_http_websocket.
Provided a frame with a payload had been received prior it was possible for a double
free to occur. The realloc operation would succeed (thus freeing the payload) but be
treated as an error. When the session was then torn down the payload would be
freed again causing a crash. The read function now takes this into account.

This change also fixes assumptions made by users of res_http_websocket. There is no
guarantee that a frame received from it will be NULL terminated.

ASTERISK-24472 #close
Reported by: Badalian Vyacheslav

Review: https://reviewboard.asterisk.org/r/4220/
Review: https://reviewboard.asterisk.org/r/4219/
........

Merged revisions 429270 from http://svn.asterisk.org/svn/asterisk/branches/11

git-svn-id: https://origsvn.digium.com/svn/asterisk/certified/branches/11.6@429271 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index d84a4a7e41e827cce18e1c48b39f52bc148a5651..e7a88d411d96db6b3959c4424effc37206fbacb6 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -2604,12 +2604,16 @@ static void sip_websocket_callback(struct ast_websocket *session, struct ast_var
 
                if (opcode == AST_WEBSOCKET_OPCODE_TEXT || opcode == AST_WEBSOCKET_OPCODE_BINARY) {
                        struct sip_request req = { 0, };
+                       char data[payload_len + 1];
 
                        if (!(req.data = ast_str_create(payload_len + 1))) {
                                goto end;
                        }
 
-                       if (ast_str_set(&req.data, -1, "%s", payload) == AST_DYNSTR_BUILD_FAILED) {
+                       strncpy(data, payload, payload_len);
+                       data[payload_len] = '\0';
+
+                       if (ast_str_set(&req.data, -1, "%s", data) == AST_DYNSTR_BUILD_FAILED) {
                                deinit_req(&req);
                                goto end;
                        }

diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index b3077a6a3da04e3e5bf2c38b78b84316c93e019a..2aba4900c967a7ce4d7e8e9a367ada58eaef121c 100644 (file)
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -402,20 +402,27 @@ int AST_OPTIONAL_API_NAME(ast_websocket_read)(struct ast_websocket *session, cha
                        }
                }
 
-               if (!(new_payload = ast_realloc(session->payload, session->payload_len + *payload_len))) {
-                       *payload_len = 0;
-                       ast_websocket_close(session, 1009);
-                       return 0;
-               }
-
                /* Per the RFC for PING we need to send back an opcode with the application data as received */
                if (*opcode == AST_WEBSOCKET_OPCODE_PING) {
                        ast_websocket_write(session, AST_WEBSOCKET_OPCODE_PONG, *payload, *payload_len);
                }
 
-               session->payload = new_payload;
-               memcpy(session->payload + session->payload_len, *payload, *payload_len);
-               session->payload_len += *payload_len;
+               if (*payload_len) {
+                       if (!(new_payload = ast_realloc(session->payload, (session->payload_len + *payload_len)))) {
+                               ast_log(LOG_WARNING, "Failed allocation: %p, %zu, %"PRIu64"\n",
+                                       session->payload, session->payload_len, *payload_len);
+                               *payload_len = 0;
+                               ast_websocket_close(session, 1009);
+                               return 0;
+                       }
+ 
+                       session->payload = new_payload;
+                       memcpy((session->payload + session->payload_len), (*payload), (*payload_len));
+                       session->payload_len += *payload_len;
+               } else if (!session->payload_len && session->payload) {
+                       ast_free(session->payload);
+                       session->payload = NULL;
+               }
 
                if (!fin && session->reconstruct && (session->payload_len < session->reconstruct)) {
                        /* If this is not a final message we need to defer returning it until later */