From e9e9db8b7d29203ca654fe1aba3feb8b6d1869f1 Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@tarantool.org>
Date: Wed, 21 May 2014 21:22:07 +0400
Subject: [PATCH] A fix for gh-206: check for mandatory keys in request body.
A server could crash if some of the request fields were missing.
Check in the request parser for presence of all mandatory fields.
Add a test case from the original fir fox gh-206 by @roman
---
src/box/lua/box_net.lua | 7 +++++
src/box/request.cc | 6 +++++
src/errcode.h | 1 +
src/iproto_constants.cc | 55 ++++++++++++++++++++++++++++++++++++++
src/iproto_constants.h | 10 +++++++
test/box/iproto.result | 53 ++++++++++++++++++++++++++++++++++++
test/box/iproto.test.py | 59 +++++++++++++++++++++++++++++++++++++++++
test/box/misc.result | 1 +
test/box/net.box.result | 4 +--
9 files changed, 194 insertions(+), 2 deletions(-)
diff --git a/src/box/lua/box_net.lua b/src/box/lua/box_net.lua
index 695a8d5aaa..b353031e47 100644
--- a/src/box/lua/box_net.lua
+++ b/src/box/lua/box_net.lua
@@ -436,6 +436,10 @@ box.net.box.new = function(host, port, reconnect_timeout)
return
end
local blen = self.s:recv(5)
+ if string.len(blen) < 5 then
+ self:fatal("The server has closed connection")
+ return
+ end
blen = msgpack.decode(blen)
local body = ''
@@ -469,6 +473,9 @@ box.net.box.new = function(host, port, reconnect_timeout)
while not self.closed do
local resp = self:read_response()
+ if resp == nil or string.len(resp) == 0 then
+ break
+ end
local header, offset = msgpack.decode(resp);
local code = header[box.net.box.TYPE]
local sync = header[box.net.box.SYNC]
diff --git a/src/box/request.cc b/src/box/request.cc
index 80f9a008ef..898ce2d3b9 100644
--- a/src/box/request.cc
+++ b/src/box/request.cc
@@ -250,6 +250,7 @@ request_decode(struct request *request, const char *data, uint32_t len)
{
assert(request->execute != NULL);
const char *end = data + len;
+ uint64_t key_map = iproto_body_key_map[request->type];
if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
error:
@@ -263,6 +264,7 @@ request_decode(struct request *request, const char *data, uint32_t len)
continue;
}
unsigned char key = mp_decode_uint(&data);
+ key_map &= ~iproto_key_bit(key);
const char *value = data;
if (mp_check(&data, end))
goto error;
@@ -301,6 +303,10 @@ request_decode(struct request *request, const char *data, uint32_t len)
if (data != end)
tnt_raise(ClientError, ER_INVALID_MSGPACK, "packet end");
#endif
+ if (key_map) {
+ tnt_raise(ClientError, ER_MISSING_REQUEST_FIELD,
+ iproto_key_strs[__builtin_ffsll((long long) key_map) - 1]);
+ }
}
int
diff --git a/src/errcode.h b/src/errcode.h
index 27bc4ec196..9c2cbb36d3 100644
--- a/src/errcode.h
+++ b/src/errcode.h
@@ -118,6 +118,7 @@ enum { TNT_ERRMSG_MAX = 512 };
/* 66 */_(ER_NODE_ID_IS_RO, 2, "Can't reset node id") \
/* 67 */_(ER_NODE_ID_IS_RESERVED, 2, "Can't initialize node id with a reserved value %u") \
/* 68 */_(ER_INVALID_ORDER, 2, "Invalid LSN order for node %u: prev lsn = %llu, new lsn = %llu") \
+ /* 69 */_(ER_MISSING_REQUEST_FIELD, 2, "Missing mandatory field '%s' in request") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/iproto_constants.cc b/src/iproto_constants.cc
index 3f5eb1c516..a64e4b9253 100644
--- a/src/iproto_constants.cc
+++ b/src/iproto_constants.cc
@@ -101,6 +101,61 @@ const char *iproto_request_type_strs[] =
"AUTH"
};
+#define bit(c) (1ULL<<IPROTO_##c)
+const uint64_t iproto_body_key_map[IPROTO_DML_REQUEST_MAX + 1] = {
+ 0, /* unused */
+ bit(SPACE_ID) | bit(LIMIT) | bit(KEY), /* SELECT */
+ bit(SPACE_ID) | bit(TUPLE), /* INSERT */
+ bit(SPACE_ID) | bit(TUPLE), /* REPLACE */
+ bit(SPACE_ID) | bit(KEY) | bit(TUPLE), /* UPDATE */
+ bit(SPACE_ID) | bit(KEY), /* DELETE */
+ bit(FUNCTION_NAME) | bit(TUPLE), /* CALL */
+ bit(USER_NAME) | bit(TUPLE) /* AUTH */
+};
+#undef bit
+
+const char *iproto_key_strs[IPROTO_KEY_MAX] = {
+ "type", /* 0x00 */
+ "sync", /* 0x01 */
+ "node_id", /* 0x02 */
+ "lsn", /* 0x03 */
+ "timestamp", /* 0x04 */
+ "", /* 0x05 */
+ "", /* 0x06 */
+ "", /* 0x07 */
+ "", /* 0x08 */
+ "", /* 0x09 */
+ "", /* 0x0a */
+ "", /* 0x0b */
+ "", /* 0x0c */
+ "", /* 0x0d */
+ "", /* 0x0e */
+ "", /* 0x0f */
+ "space_id", /* 0x10 */
+ "index_id", /* 0x11 */
+ "limit", /* 0x12 */
+ "offset", /* 0x13 */
+ "iterator", /* 0x14 */
+ "", /* 0x15 */
+ "", /* 0x16 */
+ "", /* 0x17 */
+ "", /* 0x18 */
+ "", /* 0x19 */
+ "", /* 0x1a */
+ "", /* 0x1b */
+ "", /* 0x1c */
+ "", /* 0x1d */
+ "", /* 0x1e */
+ "", /* 0x1f */
+ "key", /* 0x20 */
+ "tuple", /* 0x21 */
+ "function name", /* 0x22 */
+ "user name", /* 0x23 */
+ "node uuid" /* 0x24 */
+ "cluster uuid" /* 0x25 */
+ "lsn map" /* 0x26 */
+};
+
void
iproto_header_decode(struct iproto_header *header, const char **pos,
const char *end)
diff --git a/src/iproto_constants.h b/src/iproto_constants.h
index ffbd0e11c3..9730f63b26 100644
--- a/src/iproto_constants.h
+++ b/src/iproto_constants.h
@@ -97,6 +97,12 @@ iproto_body_has_key(const char *pos, const char *end)
#undef bit
+static inline uint64_t
+iproto_key_bit(unsigned char key)
+{
+ return 1ULL << key;
+}
+
extern const unsigned char iproto_key_type[IPROTO_KEY_MAX];
enum iproto_request_type {
@@ -115,6 +121,10 @@ enum iproto_request_type {
};
extern const char *iproto_request_type_strs[];
+/** Key names. */
+extern const char *iproto_key_strs[];
+/** A map of mandatory members of an iproto DML request. */
+extern const uint64_t iproto_body_key_map[];
static inline const char *
iproto_request_name(uint32_t type)
diff --git a/test/box/iproto.result b/test/box/iproto.result
index a5fcc050ed..f08ef53e3f 100644
--- a/test/box/iproto.result
+++ b/test/box/iproto.result
@@ -13,3 +13,56 @@ ping
---
- ok
...
+
+# Test gh-206 "Segfault if sending IPROTO package without `KEY` field"
+
+IPROTO_SELECT
+query {'IPROTO_CODE': 1} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_DELETE
+query {'IPROTO_CODE': 5} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_UPDATE
+query {'IPROTO_CODE': 4} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+query {'IPROTO_CODE': 4} {'IPROTO_SPACE_ID': 280, 'IPROTO_KEY': (1,)}
+ping
+---
+- ok
+...
+
+
+IPROTO_REPLACE
+query {'IPROTO_CODE': 3} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_CALL
+query {'IPROTO_CODE': 6} {}
+ping
+---
+- ok
+...
+query {'IPROTO_CODE': 6} {'IPROTO_KEY': ('procname',)}
+ping
+---
+- ok
+...
+
+
diff --git a/test/box/iproto.test.py b/test/box/iproto.test.py
index 895a8c90ad..096b923227 100644
--- a/test/box/iproto.test.py
+++ b/test/box/iproto.test.py
@@ -2,6 +2,9 @@ import os
import sys
import struct
import socket
+import msgpack
+from tarantool.const import *
+from tarantool import Connection
print """
#
@@ -24,3 +27,59 @@ sql("ping")
# closing connection
s.close()
+
+key_names = {}
+for (k,v) in globals().items():
+ if type(k) == str and k.startswith('IPROTO_') and type(v) == int:
+ key_names[v] = k
+
+def repr_dict(todump):
+ d = {}
+ for (k, v) in todump.items():
+ k_name = key_names.get(k, k)
+ d[k_name] = v
+ return repr(d)
+
+def test(header, body):
+ # Connect and authenticate
+ c = Connection('localhost', server.sql.port)
+ c.connect()
+ print 'query', repr_dict(header), repr_dict(body)
+ header = msgpack.dumps(header)
+ body = msgpack.dumps(body)
+ query = msgpack.dumps(len(header) + len(body)) + header + body
+ # Send raw request using connectred socket
+ s = c._socket
+ try:
+ s.send(query)
+ except OSError as e:
+ print ' => ', 'Failed to send request'
+ c.close()
+ sql("ping")
+
+print """
+# Test gh-206 "Segfault if sending IPROTO package without `KEY` field"
+"""
+
+print "IPROTO_SELECT"
+test({ IPROTO_CODE : REQUEST_TYPE_SELECT }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_DELETE"
+test({ IPROTO_CODE : REQUEST_TYPE_DELETE }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_UPDATE"
+test({ IPROTO_CODE : REQUEST_TYPE_UPDATE }, { IPROTO_SPACE_ID: 280 })
+test({ IPROTO_CODE : REQUEST_TYPE_UPDATE },
+ { IPROTO_SPACE_ID: 280, IPROTO_KEY: (1, )})
+print "\n"
+
+print "IPROTO_REPLACE"
+test({ IPROTO_CODE : REQUEST_TYPE_REPLACE }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_CALL"
+test({ IPROTO_CODE : REQUEST_TYPE_CALL }, {})
+test({ IPROTO_CODE : REQUEST_TYPE_CALL }, { IPROTO_KEY: ('procname', )})
+print "\n"
diff --git a/test/box/misc.result b/test/box/misc.result
index 121f3eaa68..94a63e618a 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -239,6 +239,7 @@ t;
- 'box.error.ER_SPACE_EXISTS : 10'
- 'box.error.ER_UNKNOWN_NODE : 62'
- 'box.error.ER_MODIFY_INDEX : 14'
+ - 'box.error.ER_MISSING_REQUEST_FIELD : 69'
- 'box.error.ER_SECONDARY : 7'
- 'box.error.ER_NODE_ID_IS_RO : 66'
- 'box.error.ER_INVALID_UUID : 64'
diff --git a/test/box/net.box.result b/test/box/net.box.result
index bc82f3272e..5056c5b584 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -430,11 +430,11 @@ remote:close()
...
remote:close()
---
-- error: '[string "-- box_net.lua (internal file)..."]:530: box.net.box: already closed'
+- error: '[string "-- box_net.lua (internal file)..."]:537: box.net.box: already closed'
...
remote:ping()
---
-- error: '[string "-- box_net.lua (internal file)..."]:535: box.net.box: connection
+- error: '[string "-- box_net.lua (internal file)..."]:542: box.net.box: connection
was closed'
...
space:drop()
--
GitLab