From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack References: <8d2042cb6269662edfee66e9b3ef3f4b34175975.1551785193.git.kshcherbatov@tarantool.org> <20190305140013.6xyoniukextm42vw@esperanza> From: Kirill Shcherbatov Message-ID: <035cb093-c2f2-6a74-400e-d20debba7590@tarantool.org> Date: Tue, 5 Mar 2019 18:55:16 +0300 MIME-Version: 1.0 In-Reply-To: <20190305140013.6xyoniukextm42vw@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: > Before the commit, xrow_header_decode treated everything until 'end' as > the packet body while currently it allows a packet to end before 'end'. > > Actually, most callers of xrow_header_decode implicitly assume that this > function still acts like that. So I think that rather than replacing the > assertion in iproto with an error, we should add a bool argument to > xrow_header_decode that would specify whether 'end' is an exact expected > position of the packet end or there may be data beyond it. If the flag > is set, it should raise 'packet body' error unless the packet ends > exactly at 'end'. Hi! Thank you for review. =================================================== Before the commit d9f82b176e24 "More than one row in fixheader. Zstd compression", xrow_header_decode treated everything until 'end' as the packet body while currently it allows a packet to end before 'end'. The iproto_msg_decode may receive an invalid msgpack but it still assumes that xrow_header_decode sets an error in such case and use assert to test it, bit it is not so. Introduced a new boolean flag to control routine behaviour. When flag is set, xrow_header_decode should raise 'packet body' error unless the packet ends exactly at 'end'. Closes #3900 https://github.com/tarantool/tarantool/tree/kshch/gh-2027-telnet-alternative-delimiter https://github.com/tarantool/tarantool/issues/2027 --- src/box/iproto.cc | 2 +- src/box/vy_run.c | 4 ++-- src/box/xlog.c | 2 +- src/box/xrow.c | 6 +++++- src/box/xrow.h | 10 ++++++---- src/box/xrow_io.cc | 6 ++++-- test/box/net.box.result | 25 +++++++++++++++++++++++++ test/box/net.box.test.lua | 11 +++++++++++ test/unit/xrow.cc | 4 ++-- 9 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/box/iproto.cc b/src/box/iproto.cc index c8b83b16b..3b0ba6234 100644 --- a/src/box/iproto.cc +++ b/src/box/iproto.cc @@ -1157,7 +1157,7 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend, { uint8_t type; - if (xrow_header_decode(&msg->header, pos, reqend)) + if (xrow_header_decode(&msg->header, pos, reqend, true)) goto error; assert(*pos == reqend); diff --git a/src/box/vy_run.c b/src/box/vy_run.c index ff39c133d..fae123d8d 100644 --- a/src/box/vy_run.c +++ b/src/box/vy_run.c @@ -698,7 +698,7 @@ vy_page_xrow(struct vy_page *page, uint32_t stmt_no, const char *data_end = stmt_no + 1 < page->row_count ? page->data + page->row_index[stmt_no + 1] : page->data + page->unpacked_size; - return xrow_header_decode(xrow, &data, data_end); + return xrow_header_decode(xrow, &data, data_end, false); } /* {{{ vy_run_iterator vy_run_iterator support functions */ @@ -832,7 +832,7 @@ vy_page_read(struct vy_page *page, const struct vy_page_info *page_info, struct xrow_header xrow; data_pos = page->data + page_info->row_index_offset; data_end = page->data + page_info->unpacked_size; - if (xrow_header_decode(&xrow, &data_pos, data_end) == -1) + if (xrow_header_decode(&xrow, &data_pos, data_end, true) == -1) goto error; if (xrow.type != VY_RUN_ROW_INDEX) { diag_set(ClientError, ER_INVALID_RUN_FILE, diff --git a/src/box/xlog.c b/src/box/xlog.c index d5de8e6d8..692b5f5ed 100644 --- a/src/box/xlog.c +++ b/src/box/xlog.c @@ -1801,7 +1801,7 @@ xlog_tx_cursor_next_row(struct xlog_tx_cursor *tx_cursor, /* Return row from xlog tx buffer */ int rc = xrow_header_decode(xrow, (const char **)&tx_cursor->rows.rpos, - (const char *)tx_cursor->rows.wpos); + (const char *)tx_cursor->rows.wpos, false); if (rc != 0) { diag_set(XlogError, "can't parse row"); /* Discard remaining row data */ diff --git a/src/box/xrow.c b/src/box/xrow.c index 107009c90..a984fa2c9 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -90,7 +90,7 @@ mp_decode_vclock(const char **data, struct vclock *vclock) int xrow_header_decode(struct xrow_header *header, const char **pos, - const char *end) + const char *end, bool ensure_package_read) { memset(header, 0, sizeof(struct xrow_header)); const char *tmp = *pos; @@ -170,6 +170,10 @@ error: header->body[0].iov_base = (void *) body; header->body[0].iov_len = *pos - body; } + if (ensure_package_read && *pos < end) { + diag_set(ClientError, ER_INVALID_MSGPACK, "packet end"); + goto error; + } return 0; } diff --git a/src/box/xrow.h b/src/box/xrow.h index 5317ae22d..9c7ca5c2e 100644 --- a/src/box/xrow.h +++ b/src/box/xrow.h @@ -128,14 +128,16 @@ xrow_header_encode(const struct xrow_header *header, uint64_t sync, * @param header[out] xrow to fill * @param pos[inout] the start of a packet * @param end the end of a packet + * @parma ensure_package_read True to set error if *pos < end + * on success. * * @retval 0 on success * @retval -1 on error (check diag) - * @post *pos == end on success + * @post *pos == end on success when ensure_package_read == true. */ int xrow_header_decode(struct xrow_header *header, - const char **pos, const char *end); + const char **pos, const char *end, bool ensure_package_read); /** * DML request. @@ -676,9 +678,9 @@ vclock_follow_xrow(struct vclock* vclock, const struct xrow_header *row) /** @copydoc xrow_header_decode. */ static inline void xrow_header_decode_xc(struct xrow_header *header, const char **pos, - const char *end) + const char *end, bool ensure_package_read) { - if (xrow_header_decode(header, pos, end) < 0) + if (xrow_header_decode(header, pos, end, ensure_package_read) < 0) diag_raise(); } diff --git a/src/box/xrow_io.cc b/src/box/xrow_io.cc index e9ee6b0c8..4b6e68bc1 100644 --- a/src/box/xrow_io.cc +++ b/src/box/xrow_io.cc @@ -58,7 +58,8 @@ coio_read_xrow(struct ev_io *coio, struct ibuf *in, struct xrow_header *row) if (to_read > 0) coio_breadn(coio, in, to_read); - xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len); + xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len, + true); } void @@ -89,7 +90,8 @@ coio_read_xrow_timeout_xc(struct ev_io *coio, struct ibuf *in, if (to_read > 0) coio_breadn_timeout(coio, in, to_read, delay); - xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len); + xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len, + true); } 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 diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc index d7c172955..68a334239 100644 --- a/test/unit/xrow.cc +++ b/test/unit/xrow.cc @@ -208,7 +208,7 @@ test_xrow_header_encode_decode() char buffer[2048]; char *pos = mp_encode_uint(buffer, 300); is(xrow_header_decode(&header, (const char **) &pos, - buffer + 100), -1, "bad msgpack end"); + buffer + 100, true), -1, "bad msgpack end"); header.type = 100; header.replica_id = 200; @@ -229,7 +229,7 @@ test_xrow_header_encode_decode() begin += fixheader_len; const char *end = (const char *)vec[0].iov_base; end += vec[0].iov_len; - is(xrow_header_decode(&decoded_header, &begin, end), 0, + is(xrow_header_decode(&decoded_header, &begin, end, true), 0, "header decode"); is(header.type, decoded_header.type, "decoded type"); is(header.replica_id, decoded_header.replica_id, "decoded replica_id"); -- 2.21.0