From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Kirill Shcherbatov Subject: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack Date: Tue, 5 Mar 2019 14:29:05 +0300 Message-Id: <8d2042cb6269662edfee66e9b3ef3f4b34175975.1551785193.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: tarantool-patches@freelists.org, vdavydov.dev@gmail.com Cc: Kirill Shcherbatov List-ID: https://github.com/tarantool/tarantool/tree/kshch/gh-3900-binary-socket-gibberish-crash https://github.com/tarantool/tarantool/issues/3900 In some cases, only the initial portion of the transmitted client request buffer is a valid msgpack. Function row_header_decode admits such scenario, however, proto_msg_decode used assert(*pos == reqend) to ensure that the data has been fully processed. It is wrong. Based on the error handling policy in this module, let's set diag message and continue processing the buffer. Closes #3900 --- src/box/iproto.cc | 5 ++++- test/box/net.box.result | 25 +++++++++++++++++++++++++ test/box/net.box.test.lua | 11 +++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/box/iproto.cc b/src/box/iproto.cc index c8b83b16b..4e08c8e0b 100644 --- a/src/box/iproto.cc +++ b/src/box/iproto.cc @@ -1159,7 +1159,10 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend, if (xrow_header_decode(&msg->header, pos, reqend)) goto error; - assert(*pos == reqend); + if (unlikely(*pos != reqend)) { + diag_set(ClientError, ER_INVALID_MSGPACK, "packet end"); + goto error; + } type = msg->header.type; diff --git a/test/box/net.box.result b/test/box/net.box.result index b800531b4..9ce6117fc 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -1381,6 +1381,31 @@ test_run:grep_log("default", "ER_NO_SUCH_PROC") box.schema.user.revoke('guest', 'execute', 'universe') --- ... +-- +-- 3900: tarantool can be crashed by sending gibberish to a +-- binary socket +-- +socket = require("socket") +--- +... +sock = socket.tcp_connect(LISTEN.host, LISTEN.service) +--- +... +data = string.fromhex("6783000000000000000000000000000000000000000000800000C8000000000000000000000000000000000000000000FFFF210100373208000000FFFF000055AAEB66486472530D02000000000010A0350001008000001000000000000000000000000000D05700") +--- +... +sock:write(data) +--- +- 104 +... +sock:close() +--- +- true +... +test_run:grep_log('default', 'ER_INVALID_MSGPACK') ~= nil +--- +- true +... -- gh-983 selecting a lot of data crashes the server or hangs the -- connection -- gh-983 test case: iproto connection selecting a lot of data diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 9e5ecfa0d..8d8257d08 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -540,6 +540,17 @@ test_run:cmd("setopt delimiter ''"); test_run:grep_log("default", "ER_NO_SUCH_PROC") box.schema.user.revoke('guest', 'execute', 'universe') +-- +-- 3900: tarantool can be crashed by sending gibberish to a +-- binary socket +-- +socket = require("socket") +sock = socket.tcp_connect(LISTEN.host, LISTEN.service) +data = string.fromhex("6783000000000000000000000000000000000000000000800000C8000000000000000000000000000000000000000000FFFF210100373208000000FFFF000055AAEB66486472530D02000000000010A0350001008000001000000000000000000000000000D05700") +sock:write(data) +sock:close() +test_run:grep_log('default', 'ER_INVALID_MSGPACK') ~= nil + -- gh-983 selecting a lot of data crashes the server or hangs the -- connection -- 2.21.0