From b0d2bea61ec1f79e193641e229968d9d8fa802d2 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
Date: Fri, 26 Jun 2020 16:41:31 +0100
Subject: util.dbuffer: dynamic string buffer

Similar to util.ringbuffer (and shares almost identical API). Differences:

- size limit is optional and dynamic
- does not allocate a fixed buffer of max_size bytes
- focus on simply storing references to existing string objects where possible,
  avoiding unnecessary allocations
- references are still stored in a ring buffer to enable use as a fast FIFO

Optional second parameter to new() provides the number of ring buffer segments. On
Lua 5.2 on my laptop, a segment is ~19 bytes. If the ring buffer fills up, the next
write will compact all strings into a single item.
---
 util/queue.lua | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/queue.lua b/util/queue.lua
index 728e905f..c8e71514 100644
--- a/util/queue.lua
+++ b/util/queue.lua
@@ -51,6 +51,13 @@ local function new(size, allow_wrapping)
 			end
 			return t[tail];
 		end;
+		replace = function (self, data)
+			if items == 0 then
+				return self:push(data);
+			end
+			t[tail] = data;
+			return true;
+		end;
 		items = function (self)
 			--luacheck: ignore 431/t
 			return function (t, pos)
-- 
cgit v1.2.3


From 83e593ac2b7f7fa1c23b06b968148a78af8aaed9 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
Date: Mon, 24 Aug 2020 16:18:13 +0100
Subject: util.dbuffer: Fix :sub() not working with partially-consumed chunks
 (thanks Zash for test case)

This also appears to fix some bugs with chunk-encoded streams in net.http.parser.
---
 util/dbuffer.lua | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)
 create mode 100644 util/dbuffer.lua

diff --git a/util/dbuffer.lua b/util/dbuffer.lua
new file mode 100644
index 00000000..a50f3a64
--- /dev/null
+++ b/util/dbuffer.lua
@@ -0,0 +1,176 @@
+local queue = require "util.queue";
+
+local dbuffer_methods = {};
+local dynamic_buffer_mt = { __index = dbuffer_methods };
+
+function dbuffer_methods:write(data)
+	if self.max_size and #data + self._length > self.max_size then
+		return nil;
+	end
+	local ok = self.items:push(data);
+	if not ok then
+		self:collapse();
+		ok = self.items:push(data);
+	end
+	if not ok then
+		return nil;
+	end
+	self._length = self._length + #data;
+	return true;
+end
+
+function dbuffer_methods:read_chunk(requested_bytes)
+	local chunk, consumed = self.items:peek(), self.front_consumed;
+	if not chunk then return; end
+	local chunk_length = #chunk;
+	local remaining_chunk_length = chunk_length - consumed;
+	if not requested_bytes then
+		requested_bytes = remaining_chunk_length;
+	end
+	if remaining_chunk_length <= requested_bytes then
+		self.front_consumed = 0;
+		self._length = self._length - remaining_chunk_length;
+		self.items:pop();
+		assert(#chunk:sub(consumed + 1, -1) == remaining_chunk_length);
+		return chunk:sub(consumed + 1, -1), remaining_chunk_length;
+	end
+	local end_pos = consumed + requested_bytes;
+	self.front_consumed = end_pos;
+	self._length = self._length - requested_bytes;
+	assert(#chunk:sub(consumed + 1, end_pos) == requested_bytes);
+	return chunk:sub(consumed + 1, end_pos), requested_bytes;
+end
+
+function dbuffer_methods:read(requested_bytes)
+	local chunks;
+
+	if requested_bytes and requested_bytes > self._length then
+		return nil;
+	end
+
+	local chunk, read_bytes = self:read_chunk(requested_bytes);
+	if not requested_bytes then
+		return chunk;
+	elseif chunk then
+		requested_bytes = requested_bytes - read_bytes;
+		if requested_bytes == 0 then -- Already read everything we need
+			return chunk;
+		end
+		chunks = {};
+	else
+		return nil;
+	end
+
+	-- Need to keep reading more chunks
+	while chunk do
+		table.insert(chunks, chunk);
+		if requested_bytes > 0 then
+			chunk, read_bytes = self:read_chunk(requested_bytes);
+			requested_bytes = requested_bytes - read_bytes;
+		else
+			break;
+		end
+	end
+
+	return table.concat(chunks);
+end
+
+function dbuffer_methods:discard(requested_bytes)
+	if requested_bytes > self._length then
+		return nil;
+	end
+
+	local chunk, read_bytes = self:read_chunk(requested_bytes);
+	if chunk then
+		requested_bytes = requested_bytes - read_bytes;
+		if requested_bytes == 0 then -- Already read everything we need
+			return true;
+		end
+	else
+		return nil;
+	end
+
+	while chunk do
+		if requested_bytes > 0 then
+			chunk, read_bytes = self:read_chunk(requested_bytes);
+			requested_bytes = requested_bytes - read_bytes;
+		else
+			break;
+		end
+	end
+	return true;
+end
+
+function dbuffer_methods:sub(i, j)
+	if j == nil then
+		j = -1;
+	end
+	if j < 0 then
+		j = self._length + (j+1);
+	end
+	if i < 0 then
+		i = self._length + (i+1);
+	end
+	if i < 1 then
+		i = 1;
+	end
+	if j > self._length then
+		j = self._length;
+	end
+	if i > j then
+		return "";
+	end
+
+	self:collapse(j);
+
+	return self.items:peek():sub(self.front_consumed+1):sub(i, j);
+end
+
+function dbuffer_methods:byte(i, j)
+	i = i or 1;
+	j = j or i;
+	return string.byte(self:sub(i, j), 1, -1);
+end
+
+function dbuffer_methods:length()
+	return self._length;
+end
+dynamic_buffer_mt.__len = dbuffer_methods.length; -- support # operator
+
+function dbuffer_methods:collapse(bytes)
+	bytes = bytes or self._length;
+
+	local front_chunk = self.items:peek();
+
+	if not front_chunk or #front_chunk - self.front_consumed >= bytes then
+		return;
+	end
+
+	local front_chunks = { front_chunk:sub(self.front_consumed+1) };
+	local front_bytes = #front_chunks[1];
+
+	while front_bytes < bytes do
+		self.items:pop();
+		local chunk = self.items:peek();
+		front_bytes = front_bytes + #chunk;
+		table.insert(front_chunks, chunk);
+	end
+	self.items:replace(table.concat(front_chunks));
+	self.front_consumed = 0;
+end
+
+local function new(max_size, max_chunks)
+	if max_size and max_size <= 0 then
+		return nil;
+	end
+	return setmetatable({
+		front_consumed = 0;
+		_length = 0;
+		max_size = max_size;
+		items = queue.new(max_chunks or 32);
+	}, dynamic_buffer_mt);
+end
+
+return {
+	new = new;
+};
-- 
cgit v1.2.3


From 0b1193317bc4288577567e1c1b0edb90c5457381 Mon Sep 17 00:00:00 2001
From: Kim Alvefur <zash@zash.se>
Date: Mon, 24 Aug 2020 17:28:48 +0200
Subject: util.dbuffer: Simplify test case

An earlier theory involved the bug being related to collapsing multiple
items, so it exercised that too.

Also correct the comment, it referred to the space in "hello world" in
an earlier version before the test string was changed to "foobar", which
was what was tested in a REPL
---
 spec/util_dbuffer_spec.lua | 130 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 spec/util_dbuffer_spec.lua

diff --git a/spec/util_dbuffer_spec.lua b/spec/util_dbuffer_spec.lua
new file mode 100644
index 00000000..af404042
--- /dev/null
+++ b/spec/util_dbuffer_spec.lua
@@ -0,0 +1,130 @@
+local dbuffer = require "util.dbuffer";
+describe("util.dbuffer", function ()
+	describe("#new", function ()
+		it("has a constructor", function ()
+			assert.Function(dbuffer.new);
+		end);
+		it("can be created", function ()
+			assert.truthy(dbuffer.new());
+		end);
+		it("won't create an empty buffer", function ()
+			assert.falsy(dbuffer.new(0));
+		end);
+		it("won't create a negatively sized buffer", function ()
+			assert.falsy(dbuffer.new(-1));
+		end);
+	end);
+	describe(":write", function ()
+		local b = dbuffer.new();
+		it("works", function ()
+			assert.truthy(b:write("hi"));
+		end);
+	end);
+
+	describe(":read", function ()
+		it("supports optional bytes parameter", function ()
+			-- should return the frontmost chunk
+			local b = dbuffer.new();
+			assert.truthy(b:write("hello"));
+			assert.truthy(b:write(" "));
+			assert.truthy(b:write("world"));
+			assert.equal("h", b:read(1));
+
+			assert.equal("ello", b:read());
+			assert.equal(" ", b:read());
+			assert.equal("world", b:read());
+		end);
+	end);
+
+	describe(":discard", function ()
+		local b = dbuffer.new();
+		it("works", function ()
+			assert.truthy(b:write("hello world"));
+			assert.truthy(b:discard(6));
+			assert.equal(5, b:length());
+			assert.equal("world", b:read(5));
+		end);
+	end);
+
+	describe(":collapse()", function ()
+		it("works on an empty buffer", function ()
+			local b = dbuffer.new();
+			b:collapse();
+		end);
+	end);
+
+	describe(":sub", function ()
+		-- Helper function to compare buffer:sub() with string:sub()
+		local s = "hello world";
+		local function test_sub(b, x, y)
+			local string_result, buffer_result = s:sub(x, y), b:sub(x, y);
+			assert.equals(string_result, buffer_result, ("buffer:sub(%d, %s) does not match string:sub()"):format(x, y and ("%d"):format(y) or "nil"));
+		end
+
+		it("works", function ()
+			local b = dbuffer.new();
+			assert.truthy(b:write("hello world"));
+			assert.equals("hello", b:sub(1, 5));
+		end);
+
+		it("works after discard", function ()
+			local b = dbuffer.new(256);
+			assert.truthy(b:write("foobar"));
+			assert.equals("foobar", b:sub(1, 6));
+			assert.truthy(b:discard(3)); -- consume "foo"
+			assert.equals("bar", b:sub(1, 3));
+		end);
+
+		it("supports optional end parameter", function ()
+			local b = dbuffer.new();
+			assert.truthy(b:write("hello world"));
+			assert.equals("hello world", b:sub(1));
+			assert.equals("world", b:sub(-5));
+		end);
+
+		it("is equivalent to string:sub", function ()
+			local b = dbuffer.new(11);
+			assert.truthy(b:write(s));
+			for i = -13, 13 do
+				for j = -13, 13 do
+					test_sub(b, i, j);
+				end
+			end
+		end);
+	end);
+
+	describe(":byte", function ()
+		-- Helper function to compare buffer:byte() with string:byte()
+		local s = "hello world"
+		local function test_byte(b, x, y)
+			local string_result, buffer_result = {s:byte(x, y)}, {b:byte(x, y)};
+			assert.same(string_result, buffer_result, ("buffer:byte(%d, %s) does not match string:byte()"):format(x, y and ("%d"):format(y) or "nil"));
+		end
+
+		it("is equivalent to string:byte", function ()
+			local b = dbuffer.new(11);
+			assert.truthy(b:write(s));
+			test_byte(b, 1);
+			test_byte(b, 3);
+			test_byte(b, -1);
+			test_byte(b, -3);
+			for i = -13, 13 do
+				for j = -13, 13 do
+					test_byte(b, i, j);
+				end
+			end
+		end);
+
+		it("works with characters > 127", function ()
+			local b = dbuffer.new();
+			b:write(string.char(0, 140));
+			local r = { b:byte(1, 2) };
+			assert.same({ 0, 140 }, r);
+		end);
+
+		it("works on an empty buffer", function ()
+			local b = dbuffer.new();
+			assert.equal("", b:sub(1,1));
+		end);
+	end);
+end);
-- 
cgit v1.2.3


From 8a24ec9653c09224a5e2d011e61067ddb11c1fe0 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
Date: Thu, 17 Sep 2020 13:00:19 +0100
Subject: net.websocket.frames: Allow all methods to work on non-string objects

Instead of using the string library, use methods from the passed object,
which are assumed to be equivalent.

This provides compatibility with objects from util.ringbuffer and
util.dbuffer, for example.
---
 net/websocket/frames.lua | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/net/websocket/frames.lua b/net/websocket/frames.lua
index ba25d261..e1b5527a 100644
--- a/net/websocket/frames.lua
+++ b/net/websocket/frames.lua
@@ -16,13 +16,12 @@ local bor = bit.bor;
 local bxor = bit.bxor;
 local lshift = bit.lshift;
 local rshift = bit.rshift;
+local unpack = table.unpack or unpack; -- luacheck: ignore 113
 
 local t_concat = table.concat;
-local s_byte = string.byte;
 local s_char= string.char;
-local s_sub = string.sub;
-local s_pack = string.pack; -- luacheck: ignore 143
-local s_unpack = string.unpack; -- luacheck: ignore 143
+local s_pack = string.pack;
+local s_unpack = string.unpack;
 
 if not s_pack and softreq"struct" then
 	s_pack = softreq"struct".pack;
@@ -30,12 +29,12 @@ if not s_pack and softreq"struct" then
 end
 
 local function read_uint16be(str, pos)
-	local l1, l2 = s_byte(str, pos, pos+1);
+	local l1, l2 = str:byte(pos, pos+1);
 	return l1*256 + l2;
 end
 -- FIXME: this may lose precision
 local function read_uint64be(str, pos)
-	local l1, l2, l3, l4, l5, l6, l7, l8 = s_byte(str, pos, pos+7);
+	local l1, l2, l3, l4, l5, l6, l7, l8 = str:byte(pos, pos+7);
 	local h = lshift(l1, 24) + lshift(l2, 16) + lshift(l3, 8) + l4;
 	local l = lshift(l5, 24) + lshift(l6, 16) + lshift(l7, 8) + l8;
 	return h * 2^32 + l;
@@ -63,9 +62,15 @@ end
 
 if s_unpack then
 	function read_uint16be(str, pos)
+		if type(str) ~= "string" then
+			str, pos = str:sub(pos, pos+1), 1;
+		end
 		return s_unpack(">I2", str, pos);
 	end
 	function read_uint64be(str, pos)
+		if type(str) ~= "string" then
+			str, pos = str:sub(pos, pos+7), 1;
+		end
 		return s_unpack(">I8", str, pos);
 	end
 end
@@ -73,7 +78,7 @@ end
 local function parse_frame_header(frame)
 	if #frame < 2 then return; end
 
-	local byte1, byte2 = s_byte(frame, 1, 2);
+	local byte1, byte2 = frame:byte(1, 2);
 	local result = {
 		FIN = band(byte1, 0x80) > 0;
 		RSV1 = band(byte1, 0x40) > 0;
@@ -102,7 +107,7 @@ local function parse_frame_header(frame)
 	end
 
 	if result.MASK then
-		result.key = { s_byte(frame, length_bytes+3, length_bytes+6) };
+		result.key = { frame:byte(length_bytes+3, length_bytes+6) };
 	end
 
 	return result, header_length;
@@ -121,7 +126,7 @@ local function apply_mask(str, key, from, to)
 	for i = from, to do
 		local key_index = counter%key_len + 1;
 		counter = counter + 1;
-		data[counter] = s_char(bxor(key[key_index], s_byte(str, i)));
+		data[counter] = s_char(bxor(key[key_index], str:byte(i)));
 	end
 	return t_concat(data);
 end
@@ -189,7 +194,7 @@ local function parse_close(data)
 	if #data >= 2 then
 		code = read_uint16be(data, 1);
 		if #data > 2 then
-			message = s_sub(data, 3);
+			message = data:sub(3);
 		end
 	end
 	return code, message
-- 
cgit v1.2.3


From bd7d32aa8d11ae959b90ac18e922261d27d0a3a3 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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.
---
 net/websocket/frames.lua  |  4 ++--
 plugins/mod_websocket.lua | 11 ++++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/websocket/frames.lua b/net/websocket/frames.lua
index e1b5527a..65f9122c 100644
--- a/net/websocket/frames.lua
+++ b/net/websocket/frames.lua
@@ -20,8 +20,8 @@ local unpack = table.unpack or unpack; -- luacheck: ignore 113
 
 local t_concat = table.concat;
 local s_char= string.char;
-local s_pack = string.pack;
-local s_unpack = string.unpack;
+local s_pack = string.pack; -- luacheck: ignore 143
+local s_unpack = string.unpack; -- luacheck: ignore 143
 
 if not s_pack and softreq"struct" then
 	s_pack = softreq"struct".pack;
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 6b5f15910327246464e0b67593fc6b4be186c1d7 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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(-)

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 359de0c75de14ebc1700b07a5f414e5a1c49b413 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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(-)

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 e7f545bae6bdf4d8d7d4753e61783b952df936fd Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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(+)

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 15aaf9446b9c50e5c5f18307ef39d3ae9ab25f6b Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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(-)

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 4836410c4fe46e33f5c4e9afbee74edaedd6f86f Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
Date: Tue, 29 Sep 2020 13:58:32 +0100
Subject: net.websocket.frames: Additionally return partial frame if there is
 one

---
 net/websocket/frames.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/websocket/frames.lua b/net/websocket/frames.lua
index 65f9122c..a0c0d4cd 100644
--- a/net/websocket/frames.lua
+++ b/net/websocket/frames.lua
@@ -141,7 +141,7 @@ end
 
 local function parse_frame(frame)
 	local result, pos = parse_frame_header(frame);
-	if result == nil or #frame < (pos + result.length) then return; end
+	if result == nil or #frame < (pos + result.length) then return nil, nil, result; end
 	result.data = parse_frame_body(frame, result, pos+1);
 	return result, pos + result.length;
 end
-- 
cgit v1.2.3


From 1b29c3c9f98113946e242885a99a595ab9da5b56 Mon Sep 17 00:00:00 2001
From: Matthew Wild <mwild1@gmail.com>
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(-)

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