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 ACDD921A40 for ; Wed, 12 Dec 2018 06:40:16 -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 gUDsiRfUFsRD for ; Wed, 12 Dec 2018 06:40:16 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 DFFA62073E for ; Wed, 12 Dec 2018 06:40:15 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] sql: terminate with \0 encoded msgpack References: <20181211162100.55972-1-korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 12 Dec 2018 14:40:10 +0300 MIME-Version: 1.0 In-Reply-To: <20181211162100.55972-1-korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Nikita Pettik Hi! Thanks for the patch! See 1 comment below. On 11/12/2018 19:21, Nikita Pettik wrote: > 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); Please, show where exactly strlen() is called on msgpack? I can not find this place. Also, terminating msgpack with 0 is a crutch, IMO. You will be able to print msgpack and use strlen(), but as you said, it will not be valid msgpack. I will not be able to decode it. This is because of msgpack having zeroes in a middle. So those columns in EXPLAIN with invalid msgpack makes no sense to show in such a case - invalid msgpack is useless. I remember, that msgpack 0 termination existed in earlier SQLite + Tarantool versions, but I removed it deliberately so as to return it without string methods usage. All places, which you crutched up with 0 termination, have SQL_SUBTYPE_MSGPACK subtype, using which you can determine if it is msgpack. I understand, that from EXPLAIN we can not return Lua maps/arrays since 6th column of EXPLAIN output is declared as a string. But we can decode msgpack into JSON or YAML. This output would be both parsable and readable. > > 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); >