From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 6B9CA21C31 for ; Tue, 11 Dec 2018 11:21:05 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id w8ZtJkn2B59k for ; Tue, 11 Dec 2018 11:21:05 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1EBEE21C1F for ; Tue, 11 Dec 2018 11:21:04 -0500 (EST) From: Nikita Pettik Subject: [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack Date: Tue, 11 Dec 2018 19:21:00 +0300 Message-Id: <20181211162100.55972-1-korablev@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: v.shpilevoy@tarantool.org, Nikita Pettik 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