From 48b245684519efde5cd2b70d27150c47985a32bc Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Thu, 7 May 2020 21:23:07 +0200 Subject: [PATCH] util/token.h: fix possible out-of-bounds read The 'WHITESPACE' case of the _calc_len method wrongly accessed the character before checking upper bound of the token. The problem is fixed by switching the order of both conditions. Fixes #3756 --- repos/base/include/util/token.h | 12 ++-- repos/base/recipes/pkg/test-token/README | 1 + repos/base/recipes/pkg/test-token/archives | 2 + repos/base/recipes/pkg/test-token/hash | 1 + repos/base/recipes/pkg/test-token/runtime | 29 ++++++++ repos/base/recipes/src/test-token/content.mk | 2 + repos/base/recipes/src/test-token/hash | 1 + repos/base/recipes/src/test-token/used_apis | 1 + repos/base/src/test/token/main.cc | 73 ++++++++++++++++++++ repos/base/src/test/token/target.mk | 3 + repos/gems/run/depot_autopilot.run | 1 + 11 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 repos/base/recipes/pkg/test-token/README create mode 100644 repos/base/recipes/pkg/test-token/archives create mode 100644 repos/base/recipes/pkg/test-token/hash create mode 100644 repos/base/recipes/pkg/test-token/runtime create mode 100644 repos/base/recipes/src/test-token/content.mk create mode 100644 repos/base/recipes/src/test-token/hash create mode 100644 repos/base/recipes/src/test-token/used_apis create mode 100644 repos/base/src/test/token/main.cc create mode 100644 repos/base/src/test/token/target.mk diff --git a/repos/base/include/util/token.h b/repos/base/include/util/token.h index 8110d1f5a..8c3c14de9 100644 --- a/repos/base/include/util/token.h +++ b/repos/base/include/util/token.h @@ -179,15 +179,19 @@ class Genode::Token size_t _quoted_string_len(size_t max_len) const { + /* + * The 'end_of_quote' function examines two 'char' values. + * Hence, the upper bound of the index is max_len - 2. + */ unsigned i = 0; - - for (; !end_of_quote(&_start[i]) && i < max_len; i++) + for (; i + 1 < max_len && !end_of_quote(&_start[i]); i++) /* string ends without final quotation mark? too bad! */ if (!_start[i]) return 0; /* exceeded maximum token length */ - if (i == max_len) return 0; + if (i + 1 == max_len) + return 0; /* * We stopped our search at the character before the @@ -234,7 +238,7 @@ class Genode::Token case WHITESPACE: { unsigned i = 0; - for (; is_whitespace(_start[i]) && i < max_len; i++); + for (; i < max_len && is_whitespace(_start[i]); i++); return i; } diff --git a/repos/base/recipes/pkg/test-token/README b/repos/base/recipes/pkg/test-token/README new file mode 100644 index 000000000..b7d8233e2 --- /dev/null +++ b/repos/base/recipes/pkg/test-token/README @@ -0,0 +1 @@ +Scenario that tests the util/token.h utility diff --git a/repos/base/recipes/pkg/test-token/archives b/repos/base/recipes/pkg/test-token/archives new file mode 100644 index 000000000..1eea3639b --- /dev/null +++ b/repos/base/recipes/pkg/test-token/archives @@ -0,0 +1,2 @@ +_/src/init +_/src/test-token diff --git a/repos/base/recipes/pkg/test-token/hash b/repos/base/recipes/pkg/test-token/hash new file mode 100644 index 000000000..ca341dab9 --- /dev/null +++ b/repos/base/recipes/pkg/test-token/hash @@ -0,0 +1 @@ +2020-04-23 42b5b14a84c37b36077cd9c304057c7e3d18163e diff --git a/repos/base/recipes/pkg/test-token/runtime b/repos/base/recipes/pkg/test-token/runtime new file mode 100644 index 000000000..da90071a4 --- /dev/null +++ b/repos/base/recipes/pkg/test-token/runtime @@ -0,0 +1,29 @@ + + + + + finished token test + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/repos/base/recipes/src/test-token/content.mk b/repos/base/recipes/src/test-token/content.mk new file mode 100644 index 000000000..27388dfed --- /dev/null +++ b/repos/base/recipes/src/test-token/content.mk @@ -0,0 +1,2 @@ +SRC_DIR = src/test/token +include $(GENODE_DIR)/repos/base/recipes/src/content.inc diff --git a/repos/base/recipes/src/test-token/hash b/repos/base/recipes/src/test-token/hash new file mode 100644 index 000000000..22801111b --- /dev/null +++ b/repos/base/recipes/src/test-token/hash @@ -0,0 +1 @@ +2020-04-23 0e735b0b95eaeb3f55c36b56e06c8a0b17f3c44b diff --git a/repos/base/recipes/src/test-token/used_apis b/repos/base/recipes/src/test-token/used_apis new file mode 100644 index 000000000..df967b96a --- /dev/null +++ b/repos/base/recipes/src/test-token/used_apis @@ -0,0 +1 @@ +base diff --git a/repos/base/src/test/token/main.cc b/repos/base/src/test/token/main.cc new file mode 100644 index 000000000..1595e6f9f --- /dev/null +++ b/repos/base/src/test/token/main.cc @@ -0,0 +1,73 @@ +/* + * \brief Tokenizer test + * \author Norman Feske + * \date 2020-05-08 + */ + +/* + * Copyright (C) 2020 Genode Labs GmbH + * + * This file is part of the Genode OS framework, which is distributed + * under the terms of the GNU Affero General Public License version 3. + */ + +#include +#include +#include +#include +#include +#include +#include + +using namespace Genode; + + +/** + * Regression test for issue #3756 + */ +static void test_out_of_bounds_access(Env &env) +{ + enum { PAGE_SIZE = 4096U, + SUB_RM_SIZE = PAGE_SIZE*2, + BUF_SIZE = PAGE_SIZE }; + + Rm_connection rm(env); + Region_map_client sub_rm(rm.create(SUB_RM_SIZE)); + + /* allocate physical page of memory as buffer */ + Attached_ram_dataspace buf_ds(env.ram(), env.rm(), BUF_SIZE); + + /* attach buffer at start of managed dataspace, leave 2nd page as guard */ + sub_rm.attach_at(buf_ds.cap(), 0); + + /* locally attach managed dataspace */ + char * const buf_ptr = env.rm().attach(sub_rm.dataspace()); + + auto tokenize_two_tokens_at_end_of_buffer = [&] (char const * const input) + { + log("tokenize: '", input, "'"); + + size_t const input_len = strlen(input); + char * const token_ptr = buf_ptr + BUF_SIZE - input_len; + memcpy(token_ptr, input, input_len); + + typedef ::Genode::Token Token; + + Token t(token_ptr, input_len); + + t = t.next(); + }; + + tokenize_two_tokens_at_end_of_buffer("x "); + tokenize_two_tokens_at_end_of_buffer("x\""); +} + + +void Component::construct(Env &env) +{ + log("--- token test ---"); + + test_out_of_bounds_access(env); + + log("--- finished token test ---"); +} diff --git a/repos/base/src/test/token/target.mk b/repos/base/src/test/token/target.mk new file mode 100644 index 000000000..f460994cb --- /dev/null +++ b/repos/base/src/test/token/target.mk @@ -0,0 +1,3 @@ +TARGET = test-token +SRC_CC = main.cc +LIBS = base diff --git a/repos/gems/run/depot_autopilot.run b/repos/gems/run/depot_autopilot.run index 5ca6334cd..067215f18 100644 --- a/repos/gems/run/depot_autopilot.run +++ b/repos/gems/run/depot_autopilot.run @@ -723,6 +723,7 @@ set default_test_pkgs { test-terminal_crosslink test-timer test-tls + test-token test-trace test-trace_logger test-utf8