Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: v.shpilevoy@tarantool.org, Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack
Date: Tue, 11 Dec 2018 19:21:00 +0300	[thread overview]
Message-ID: <20181211162100.55972-1-korablev@tarantool.org> (raw)

During table, index, constraint or trigger creation it is required to
encode entry's parts/opts to msgpack for the next insertion into system
space. Encoded msgpack is known to lack null termination at the end of
binary string. Hence, while such string is passed to printf, strlen or
other function which excpects null termination, buffer overflow may
occur.

This bug was hard to detect due to several reasons.
Firstly, SQLite memory allocators align memory to 8-byte border. Hence,
as a rule even ASAN doesn't detect overflow, since there is additional
alignment which can be addressable.
Secondly, msgpack can contain nulls through the encoded string. As a
result, strlen returns not the real size of binary string, but something
less.

Note that null termination is required only encoded string is to be
displayed (i.e. EXPLAIN statement is executed or vdbe_debug mode is
enabled).

Bug was discovered with help of AddressSanitizer.

Closes #3868
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-3868-buffer-overflow
Issue: https://github.com/tarantool/tarantool/issues/3868

* Notes to the one who will review *

To enable ASAN on macOS:

cmake -DENABLE_ASAN=ON .

Also it may be required to apply this diff:

diff --git a/src/fiber.c b/src/fiber.c
index 7658a2294..743ceff7a 100644
--- a/src/fiber.c
+++ b/src/fiber.c
@@ -54,7 +54,7 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
        __sanitizer_start_switch_fiber((will_switch_back) ? &var_name : NULL, \
                                        (bottom), (size))
 #define ASAN_FINISH_SWITCH_FIBER(var_name) \
-       __sanitizer_finish_switch_fiber(var_name);
+       __sanitizer_finish_switch_fiber(var_name, 0, 0);

To turn off SQLite 8-byte allocation alignment apply this diff:

diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
index 9526cce5e..fd206632b 100644
--- a/src/box/sql/malloc.c
+++ b/src/box/sql/malloc.c
@@ -49,7 +49,7 @@ sql_sized_malloc(int nByte)
 {
        sqlite3_int64 *p;
        assert(nByte > 0);
-       nByte = ROUND8(nByte);
+       //nByte = ROUND8(nByte);
        p = malloc(nByte + 8);
        if (p) {
                p[0] = nByte;
@@ -312,9 +312,9 @@ sqlite3MallocAlarm(int nByte)
 static int
 mallocWithAlarm(int n, void **pp)
 {
-       int nFull;
+       int nFull = n;
        void *p;
-       nFull = ROUND8(n);
+       //nFull = ROUND8(n);
        sqlite3StatusHighwater(SQLITE_STATUS_MALLOC_SIZE, n);
        if (mem0.alarmThreshold > 0) {
                sqlite3_int64 nUsed =

To observe buffer-overflow run this query
(it is taken from sql-tap/table.test.lua):

EXPLAIN CREATE TABLE test1(f1 int primary key);

 src/box/sql/build.c   | 19 ++++++++++++++++---
 src/box/sql/trigger.c |  4 ++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 52f0bde15..be51028e3 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1140,7 +1140,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
 	if (index_parts == NULL)
 		goto error;
 	char *raw = sqlite3DbMallocRaw(parse->db,
-				       index_opts_sz +index_parts_sz);
+				       index_opts_sz + index_parts_sz + 1);
 	if (raw == NULL)
 		return;
 	memcpy(raw, index_opts, index_opts_sz);
@@ -1148,6 +1148,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
 	raw += index_opts_sz;
 	memcpy(raw, index_parts, index_parts_sz);
 	index_parts = raw;
+	index_parts[index_parts_sz] = '\0';
 
 	if (parse->pNewTable != NULL) {
 		sqlite3VdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg);
@@ -1209,8 +1210,18 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
 	char *table_stmt = sql_encode_table(region, table, &table_stmt_sz);
 	if (table_stmt == NULL)
 		goto error;
+	/*
+	 * If we are executing EXPLAIN query or vdbe_debug mode
+	 * is enabled, we should terminate string with \0:
+	 * in internals it is copied to be displayed.
+	 * In turn, to copy string strlen() is called:
+	 * see handling of P4 in sqlite3VdbeList().
+	 * Without null termination call of strlen() may lead to
+	 * buffer-overflow. This bug was detected by ASAN.
+	 */
+
 	char *raw = sqlite3DbMallocRaw(pParse->db,
-				       table_stmt_sz + table_opts_stmt_sz);
+				       table_stmt_sz + table_opts_stmt_sz + 1);
 	if (raw == NULL)
 		return;
 
@@ -1219,6 +1230,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
 	raw += table_opts_stmt_sz;
 	memcpy(raw, table_stmt, table_stmt_sz);
 	table_stmt = raw;
+	table_stmt[table_stmt_sz] = '\0';
 
 	sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
 	sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
@@ -1397,7 +1409,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
 	 * as dynamic and releases memory.
 	 */
 	char *raw = sqlite3DbMallocRaw(parse_context->db,
-				       parent_links_size + child_links_size);
+				       parent_links_size + child_links_size + 1);
 	if (raw == NULL)
 		return;
 	memcpy(raw, parent_links, parent_links_size);
@@ -1405,6 +1417,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
 	raw += parent_links_size;
 	memcpy(raw, child_links, child_links_size);
 	child_links = raw;
+	child_links[child_links_size] = '\0';
 
 	sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
 			  SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 482f40926..cd4d3fb47 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -213,11 +213,11 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
 			parse->rc = SQL_TARANTOOL_ERROR;
 			goto cleanup;
 		}
-		char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
+		char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz + 1);
 		if (opts_buff == NULL)
 			goto cleanup;
 		memcpy(opts_buff, data, opts_buff_sz);
-
+		opts_buff[opts_buff_sz] = '\0';
 		trigger_name = sqlite3DbStrDup(db, trigger_name);
 		if (trigger_name == NULL) {
 			sqlite3DbFree(db, opts_buff);
-- 
2.15.1

             reply	other threads:[~2018-12-11 16:21 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-11 16:21 Nikita Pettik [this message]
2018-12-12 11:40 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-13 13:53   ` n.pettik
2018-12-13 16:30     ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181211162100.55972-1-korablev@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox