From 5a1c206eceafe544292059d3749ad033010af37f Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Thu, 17 Sep 2020 13:04:46 +0100 Subject: mod_websocket: Switch partial frame buffering to util.dbuffer This improves performance and enforces stanza size limits earlier in the pipeline. --- plugins/mod_websocket.lua | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index 177259e6..ad94486b 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -18,6 +18,7 @@ local contains_token = require "util.http".contains_token; local portmanager = require "core.portmanager"; local sm_destroy_session = require"core.sessionmanager".destroy_session; local log = module._log; +local dbuffer = require "util.dbuffer"; local websocket_frames = require"net.websocket.frames"; local parse_frame = websocket_frames.parse; @@ -27,6 +28,8 @@ local parse_close = websocket_frames.parse_close; local t_concat = table.concat; +local stanza_size_limit = module:get_option_number("c2s_stanza_size_limit", 10 * 1024 * 1024); +local frame_fragment_limit = module:get_option_number("websocket_frame_fragment_limit", 8); local stream_close_timeout = module:get_option_number("c2s_close_timeout", 5); local consider_websocket_secure = module:get_option_boolean("consider_websocket_secure"); local cross_domain = module:get_option_set("cross_domain_websocket", {}); @@ -269,14 +272,16 @@ function handle_request(event) session.open_stream = session_open_stream; session.close = session_close; - local frameBuffer = ""; + -- max frame header is 22 bytes + local frameBuffer = dbuffer.new(stanza_size_limit + 22, frame_fragment_limit); add_filter(session, "bytes/in", function(data) + frameBuffer:write(data); + local cache = {}; - frameBuffer = frameBuffer .. data; local frame, length = parse_frame(frameBuffer); while frame do - frameBuffer = frameBuffer:sub(length + 1); + frameBuffer:discard(length); local result = handle_frame(frame); if not result then return; end cache[#cache+1] = filter_open_close(result); -- cgit v1.2.3 From a4e9e4815c0f14d438a57b40b838f9715da6f3f5 Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Thu, 17 Sep 2020 16:41:48 +0100 Subject: mod_websocket: handle full frame buffer and raise stream error --- plugins/mod_websocket.lua | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index ad94486b..d3b035db 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -275,7 +275,11 @@ function handle_request(event) -- max frame header is 22 bytes local frameBuffer = dbuffer.new(stanza_size_limit + 22, frame_fragment_limit); add_filter(session, "bytes/in", function(data) - frameBuffer:write(data); + if not frameBuffer:write(data) then + session.log("warn", "websocket frame buffer full - terminating session"); + session:close({ condition = "resource-constraint", text = "frame buffer exceeded" }); + return; + end local cache = {}; local frame, length = parse_frame(frameBuffer); -- cgit v1.2.3 From 3c0924eee1130bdcec884e58cc2e04efdb54afb2 Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Thu, 17 Sep 2020 16:42:14 +0100 Subject: mod_websocket: Add separate limit for frame buffer size --- plugins/mod_websocket.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index d3b035db..53a1d452 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -29,6 +29,7 @@ local parse_close = websocket_frames.parse_close; local t_concat = table.concat; local stanza_size_limit = module:get_option_number("c2s_stanza_size_limit", 10 * 1024 * 1024); +local frame_buffer_limit = module:get_option_number("websocket_frame_buffer_limit", 2 * stanza_size_limit); local frame_fragment_limit = module:get_option_number("websocket_frame_fragment_limit", 8); local stream_close_timeout = module:get_option_number("c2s_close_timeout", 5); local consider_websocket_secure = module:get_option_boolean("consider_websocket_secure"); @@ -272,8 +273,7 @@ function handle_request(event) session.open_stream = session_open_stream; session.close = session_close; - -- max frame header is 22 bytes - local frameBuffer = dbuffer.new(stanza_size_limit + 22, frame_fragment_limit); + local frameBuffer = dbuffer.new(frame_buffer_limit, frame_fragment_limit); add_filter(session, "bytes/in", function(data) if not frameBuffer:write(data) then session.log("warn", "websocket frame buffer full - terminating session"); -- cgit v1.2.3 From 364146dbb1d8b8facf1bc02b336f9ce38c1b4746 Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Thu, 17 Sep 2020 16:42:36 +0100 Subject: mod_websocket: Enforce stanza size limit and close stream --- plugins/mod_websocket.lua | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index 53a1d452..0bd001f4 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -285,6 +285,10 @@ function handle_request(event) local frame, length = parse_frame(frameBuffer); while frame do + if length > stanza_size_limit then + session:close({ condition = "policy-violation", text = "stanza too large" }); + return; + end frameBuffer:discard(length); local result = handle_frame(frame); if not result then return; end -- cgit v1.2.3 From 71256641f4f1603b9c70af0aec010fa98923e7a4 Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Mon, 28 Sep 2020 16:36:12 +0100 Subject: mod_websocket: Continue to process data already in the buffer after an error occurs Previously any error, or even a normal websocket close frame, would return early, leaving potentially entire frames in the buffer unprocessed and then discarded. This change stops processing new data, but returns an existing processed data up to the point of the error/close. --- plugins/mod_websocket.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index 0bd001f4..ecc12a45 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -291,7 +291,7 @@ function handle_request(event) end frameBuffer:discard(length); local result = handle_frame(frame); - if not result then return; end + if not result then break; end cache[#cache+1] = filter_open_close(result); frame, length = parse_frame(frameBuffer); end -- cgit v1.2.3 From a481251b29a745556bcb9a32786f3ccd02ad7102 Mon Sep 17 00:00:00 2001 From: Matthew Wild Date: Tue, 29 Sep 2020 15:18:32 +0100 Subject: mod_websocket: Refactor frame validity checking, also check partially-received frames against constraints --- plugins/mod_websocket.lua | 172 +++++++++++++++++++++++++--------------------- 1 file changed, 95 insertions(+), 77 deletions(-) (limited to 'plugins') diff --git a/plugins/mod_websocket.lua b/plugins/mod_websocket.lua index ecc12a45..a613672b 100644 --- a/plugins/mod_websocket.lua +++ b/plugins/mod_websocket.lua @@ -142,6 +142,65 @@ local function filter_open_close(data) return data; end + +local function validate_frame(frame, max_length) + local opcode, length = frame.opcode, frame.length; + + if max_length and length > max_length then + return false, 1009, "Payload too large"; + end + + -- Error cases + if frame.RSV1 or frame.RSV2 or frame.RSV3 then -- Reserved bits non zero + return false, 1002, "Reserved bits not zero"; + end + + if opcode == 0x8 and frame.data then -- close frame + if length == 1 then + return false, 1002, "Close frame with payload, but too short for status code"; + elseif length >= 2 then + local status_code = parse_close(frame.data) + if status_code < 1000 then + return false, 1002, "Closed with invalid status code"; + elseif ((status_code > 1003 and status_code < 1007) or status_code > 1011) and status_code < 3000 then + return false, 1002, "Closed with reserved status code"; + end + end + end + + if opcode >= 0x8 then + if length > 125 then -- Control frame with too much payload + return false, 1002, "Payload too large"; + end + + if not frame.FIN then -- Fragmented control frame + return false, 1002, "Fragmented control frame"; + end + end + + if (opcode > 0x2 and opcode < 0x8) or (opcode > 0xA) then + return false, 1002, "Reserved opcode"; + end + + -- Check opcode + if opcode == 0x2 then -- Binary frame + return false, 1003, "Only text frames are supported, RFC 7395 3.2"; + elseif opcode == 0x8 then -- Close request + return false, 1000, "Goodbye"; + end + + -- Other (XMPP-specific) validity checks + if not frame.FIN then + return false, 1003, "Continuation frames are not supported, RFC 7395 3.3.3"; + end + if opcode == 0x01 and frame.data and frame.data:byte(1, 1) ~= 60 then + return false, 1007, "Invalid payload start character, RFC 7395 3.3.3"; + end + + return true; +end + + function handle_request(event) local request, response = event.request, event.response; local conn = response.conn; @@ -172,90 +231,40 @@ function handle_request(event) conn:close(); end - local dataBuffer; - local function handle_frame(frame) - local opcode = frame.opcode; - local length = frame.length; - module:log("debug", "Websocket received frame: opcode=%0x, %i bytes", frame.opcode, #frame.data); - - -- Error cases - if frame.RSV1 or frame.RSV2 or frame.RSV3 then -- Reserved bits non zero - websocket_close(1002, "Reserved bits not zero"); - return false; - end - - if opcode == 0x8 then -- close frame - if length == 1 then - websocket_close(1002, "Close frame with payload, but too short for status code"); - return false; - elseif length >= 2 then - local status_code = parse_close(frame.data) - if status_code < 1000 then - websocket_close(1002, "Closed with invalid status code"); - return false; - elseif ((status_code > 1003 and status_code < 1007) or status_code > 1011) and status_code < 3000 then - websocket_close(1002, "Closed with reserved status code"); - return false; - end - end - end - - if opcode >= 0x8 then - if length > 125 then -- Control frame with too much payload - websocket_close(1002, "Payload too large"); - return false; - end - - if not frame.FIN then -- Fragmented control frame - websocket_close(1002, "Fragmented control frame"); - return false; - end - end - - if (opcode > 0x2 and opcode < 0x8) or (opcode > 0xA) then - websocket_close(1002, "Reserved opcode"); - return false; + local function websocket_handle_error(session, code, message) + if code == 1009 then -- stanza size limit exceeded + -- we close the session, rather than the connection, + -- otherwise a resuming client will simply resend the + -- offending stanza + session:close({ condition = "policy-violation", text = "stanza too large" }); + else + websocket_close(code, message); end + end - if opcode == 0x0 and not dataBuffer then - websocket_close(1002, "Unexpected continuation frame"); - return false; - end + local function handle_frame(frame) + module:log("debug", "Websocket received frame: opcode=%0x, %i bytes", frame.opcode, #frame.data); - if (opcode == 0x1 or opcode == 0x2) and dataBuffer then - websocket_close(1002, "Continuation frame expected"); - return false; + -- Check frame makes sense + local frame_ok, err_status, err_text = validate_frame(frame, stanza_size_limit); + if not frame_ok then + return frame_ok, err_status, err_text; end - -- Valid cases - if opcode == 0x0 then -- Continuation frame - dataBuffer[#dataBuffer+1] = frame.data; - elseif opcode == 0x1 then -- Text frame - dataBuffer = {frame.data}; - elseif opcode == 0x2 then -- Binary frame - websocket_close(1003, "Only text frames are supported"); - return; - elseif opcode == 0x8 then -- Close request - websocket_close(1000, "Goodbye"); - return; - elseif opcode == 0x9 then -- Ping frame + local opcode = frame.opcode; + if opcode == 0x9 then -- Ping frame frame.opcode = 0xA; frame.MASK = false; -- Clients send masked frames, servers don't, see #1484 conn:write(build_frame(frame)); return ""; elseif opcode == 0xA then -- Pong frame, MAY be sent unsolicited, eg as keepalive return ""; - else + elseif opcode ~= 0x1 then -- Not text frame (which is all we support) log("warn", "Received frame with unsupported opcode %i", opcode); return ""; end - if frame.FIN then - local data = t_concat(dataBuffer, ""); - dataBuffer = nil; - return data; - end - return ""; + return frame.data; end conn:setlistener(c2s_listener); @@ -282,19 +291,28 @@ function handle_request(event) end local cache = {}; - local frame, length = parse_frame(frameBuffer); + local frame, length, partial = parse_frame(frameBuffer); while frame do - if length > stanza_size_limit then - session:close({ condition = "policy-violation", text = "stanza too large" }); - return; - end frameBuffer:discard(length); - local result = handle_frame(frame); - if not result then break; end + local result, err_status, err_text = handle_frame(frame); + if not result then + websocket_handle_error(session, err_status, err_text); + break; + end cache[#cache+1] = filter_open_close(result); - frame, length = parse_frame(frameBuffer); + frame, length, partial = parse_frame(frameBuffer); end + + if partial then + -- The header of the next frame is already in the buffer, run + -- some early validation here + local frame_ok, err_status, err_text = validate_frame(partial, stanza_size_limit); + if not frame_ok then + websocket_handle_error(session, err_status, err_text); + end + end + return t_concat(cache, ""); end); -- cgit v1.2.3