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
........

Merged revisions 429272 from http://svn.asterisk.org/svn/asterisk/branches/12
........

Merged revisions 429273 from http://svn.asterisk.org/svn/asterisk/branches/13

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@429274 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index cea01d729643f06271f3ca384d8a4743b903dc0b..c22807c875f263d8c28e1841220f406f50dc2236 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -2648,12 +2648,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 e47bcd2310b6e941ed21b22d2e60417156071bce..400e5eb1c295ed63eff502af21310dea287fa92b 100644 (file)
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -513,14 +513,6 @@ 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)))) {
-                       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;
-               }
-
                /* 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))) {
                        *payload_len = 0;
@@ -528,9 +520,22 @@ int AST_OPTIONAL_API_NAME(ast_websocket_read)(struct ast_websocket *session, cha
                        return 0;
                }
 
-               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 */

diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c
index 1db36bb5a97ddf7b9a476487c3dc580c69e33ad3..5616715e03ba90e77b382dabb33b7816cfbf7957 100644 (file)
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -200,7 +200,8 @@ static int transport_read(void *data)
 
        pj_gettimeofday(&rdata->pkt_info.timestamp);
 
-       pj_memcpy(rdata->pkt_info.packet, read_data->payload, sizeof(rdata->pkt_info.packet));
+       pj_memcpy(rdata->pkt_info.packet, read_data->payload,
+               PJSIP_MAX_PKT_LEN < read_data->payload_len ? PJSIP_MAX_PKT_LEN : read_data->payload_len);
        rdata->pkt_info.len = read_data->payload_len;
        rdata->pkt_info.zero = 0;