From 88993b81f0e87bfb395d8280dd3f42aabd1e454c Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 00:03:25 -0700 Subject: [PATCH 1/9] Harden Pairs de/serialization. Reported by Chris Ertl from Google Security. Signed-off-by: Piotr Sikora --- .bazelrc | 39 ++-- BUILD | 1 + bazel/dependencies.bzl | 5 + bazel/repositories.bzl | 8 + include/proxy-wasm/exports.h | 30 --- include/proxy-wasm/limits.h | 10 + include/proxy-wasm/pairs_util.h | 193 ++++++++++++++++++ src/exports.cc | 82 +++----- test/BUILD | 15 ++ ...h-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 | Bin 0 -> 36 bytes test/pairs_util_fuzzer.cc | 46 +++++ 11 files changed, 329 insertions(+), 100 deletions(-) create mode 100644 include/proxy-wasm/pairs_util.h create mode 100644 test/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 create mode 100644 test/pairs_util_fuzzer.cc diff --git a/.bazelrc b/.bazelrc index 00f6848f4..038141be3 100644 --- a/.bazelrc +++ b/.bazelrc @@ -8,37 +8,48 @@ build:clang --action_env=BAZEL_COMPILER=clang build:clang --action_env=CC=clang build:clang --action_env=CXX=clang++ +# Common flags for Clang sanitizers. +build:clang-xsan --config=clang +build:clang-xsan --copt -O1 +build:clang-xsan --copt -fno-omit-frame-pointer +build:clang-xsan --copt -fno-optimize-sibling-calls +build:clang-xsan --copt -fno-sanitize-recover=all +build:clang-xsan --linkopt -fsanitize-link-c++-runtime +build:clang-xsan --linkopt -fuse-ld=lld +build:clang-xsan --linkopt -rtlib=compiler-rt + # Use Clang compiler with Address and Undefined Behavior Sanitizers. -build:clang-asan --config=clang +build:clang-asan --config=clang-xsan build:clang-asan --copt -DADDRESS_SANITIZER=1 build:clang-asan --copt -DUNDEFINED_SANITIZER=1 -build:clang-asan --copt -O1 -build:clang-asan --copt -fno-omit-frame-pointer -build:clang-asan --copt -fno-optimize-sibling-calls build:clang-asan --copt -fsanitize=address,undefined build:clang-asan --copt -fsanitize-address-use-after-scope build:clang-asan --linkopt -fsanitize=address,undefined build:clang-asan --linkopt -fsanitize-address-use-after-scope -build:clang-asan --linkopt -fsanitize-link-c++-runtime -build:clang-asan --linkopt -fuse-ld=lld build:clang-asan --test_env=ASAN_OPTIONS=check_initialization_order=1:detect_stack_use_after_return=1:strict_init_order=1:strict_string_checks=1 -build:clang-asan --test_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1 +build:clang-asan --test_env=UBSAN_OPTIONS=print_stacktrace=1 build:clang-asan --test_env=ASAN_SYMBOLIZER_PATH # Use Clang compiler with Address and Undefined Behavior Sanitizers (strict version). build:clang-asan-strict --config=clang-asan -build:clang-asan-strict --copt -fsanitize=integer -build:clang-asan-strict --linkopt -fsanitize=integer +build:clang-asan-strict --copt -fsanitize=integer,local-bounds,nullability +build:clang-asan-strict --linkopt -fsanitize=integer,local-bounds,nullability + +# Use Honggfuzz with Address and Undefined Behavior Sanitizers (strict version). +build:clang-asan-honggfuzz --config=clang-asan-strict +build:clang-asan-honggfuzz --@rules_fuzzing//fuzzing:cc_engine=@rules_fuzzing//fuzzing/engines:honggfuzz +build:clang-asan-honggfuzz --@rules_fuzzing//fuzzing:cc_engine_instrumentation=honggfuzz + +# Use LibFuzzer with Address and Undefined Behavior Sanitizers (strict version). +build:clang-asan-libfuzzer --config=clang-asan-strict +build:clang-asan-libfuzzer --@rules_fuzzing//fuzzing:cc_engine=@rules_fuzzing//fuzzing/engines:libfuzzer +build:clang-asan-libfuzzer --@rules_fuzzing//fuzzing:cc_engine_instrumentation=libfuzzer # Use Clang compiler with Thread Sanitizer. -build:clang-tsan --config=clang +build:clang-tsan --config=clang-xsan build:clang-tsan --copt -DTHREAD_SANITIZER=1 -build:clang-tsan --copt -O1 -build:clang-tsan --copt -fno-omit-frame-pointer -build:clang-tsan --copt -fno-optimize-sibling-calls build:clang-tsan --copt -fsanitize=thread build:clang-tsan --linkopt -fsanitize=thread -build:clang-tsan --linkopt -fuse-ld=lld # Use Clang-Tidy tool. build:clang-tidy --aspects @bazel_clang_tidy//clang_tidy:clang_tidy.bzl%clang_tidy_aspect diff --git a/BUILD b/BUILD index 67d8d89df..b2e8fdbfb 100644 --- a/BUILD +++ b/BUILD @@ -62,6 +62,7 @@ cc_library( ], hdrs = [ "include/proxy-wasm/bytecode_util.h", + "include/proxy-wasm/pairs_util.h", "include/proxy-wasm/signature_util.h", ], linkopts = select({ diff --git a/bazel/dependencies.bzl b/bazel/dependencies.bzl index 8e0cfe91d..b6a4cb576 100644 --- a/bazel/dependencies.bzl +++ b/bazel/dependencies.bzl @@ -17,6 +17,8 @@ load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps") load("@proxy_wasm_cpp_host//bazel/cargo/wasmsign:crates.bzl", "wasmsign_fetch_remote_crates") load("@proxy_wasm_cpp_host//bazel/cargo/wasmtime:crates.bzl", "wasmtime_fetch_remote_crates") load("@rules_foreign_cc//foreign_cc:repositories.bzl", "rules_foreign_cc_dependencies") +load("@rules_fuzzing//fuzzing:init.bzl", "rules_fuzzing_init") +load("@rules_fuzzing//fuzzing:repositories.bzl", "rules_fuzzing_dependencies") load("@rules_python//python:pip.bzl", "pip_install") load("@rules_rust//rust:repositories.bzl", "rust_repositories", "rust_repository_set") @@ -25,6 +27,9 @@ def proxy_wasm_cpp_host_dependencies(): rules_foreign_cc_dependencies() + rules_fuzzing_dependencies() + rules_fuzzing_init() + rust_repositories() rust_repository_set( name = "rust_linux_x86_64", diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index af8d5c8b6..87ca15ee8 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -55,6 +55,14 @@ def proxy_wasm_cpp_host_repositories(): url = "https://github.com/bazelbuild/rules_foreign_cc/archive/0.7.1.tar.gz", ) + maybe( + http_archive, + name = "rules_fuzzing", + sha256 = "23bb074064c6f488d12044934ab1b0631e8e6898d5cf2f6bde087adb01111573", + strip_prefix = "rules_fuzzing-0.3.1", + url = "https://github.com/bazelbuild/rules_fuzzing/archive/v0.3.1.zip", + ) + maybe( http_archive, name = "rules_python", diff --git a/include/proxy-wasm/exports.h b/include/proxy-wasm/exports.h index 325d28ff3..376a4d3b6 100644 --- a/include/proxy-wasm/exports.h +++ b/include/proxy-wasm/exports.h @@ -62,36 +62,6 @@ struct RegisterForeignFunction { namespace exports { -template size_t pairsSize(const Pairs &result) { - size_t size = 4; // number of headers - for (auto &p : result) { - size += 8; // size of key, size of value - size += p.first.size() + 1; // null terminated key - size += p.second.size() + 1; // null terminated value - } - return size; -} - -template void marshalPairs(const Pairs &result, char *buffer) { - char *b = buffer; - *reinterpret_cast(b) = htowasm(result.size()); - b += sizeof(uint32_t); - for (auto &p : result) { - *reinterpret_cast(b) = htowasm(p.first.size()); - b += sizeof(uint32_t); - *reinterpret_cast(b) = htowasm(p.second.size()); - b += sizeof(uint32_t); - } - for (auto &p : result) { - memcpy(b, p.first.data(), p.first.size()); - b += p.first.size(); - *b++ = 0; - memcpy(b, p.second.data(), p.second.size()); - b += p.second.size(); - *b++ = 0; - } -} - // ABI functions exported from host to wasm. Word get_configuration(Word value_ptr_ptr, Word value_size_ptr); diff --git a/include/proxy-wasm/limits.h b/include/proxy-wasm/limits.h index d02d4c5e3..9b9ac1cd3 100644 --- a/include/proxy-wasm/limits.h +++ b/include/proxy-wasm/limits.h @@ -30,3 +30,13 @@ #ifndef PROXY_WASM_HOST_WASI_RANDOM_GET_MAX_SIZE_BYTES #define PROXY_WASM_HOST_WASI_RANDOM_GET_MAX_SIZE_BYTES (64 * 1024) #endif + +// Maximum allowed size of Pairs buffer to deserialize. +#ifndef PROXY_WASM_HOST_PAIRS_MAX_BYTES +#define PROXY_WASM_HOST_PAIRS_MAX_BYTES (1024 * 1024) +#endif + +// Maximum allowed number of pairs in a Pairs buffer to deserialize. +#ifndef PROXY_WASM_HOST_PAIRS_MAX_COUNT +#define PROXY_WASM_HOST_PAIRS_MAX_COUNT 1024 +#endif diff --git a/include/proxy-wasm/pairs_util.h b/include/proxy-wasm/pairs_util.h new file mode 100644 index 000000000..c8f445b64 --- /dev/null +++ b/include/proxy-wasm/pairs_util.h @@ -0,0 +1,193 @@ +// Copyright 2016-2019 Envoy Project Authors +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include + +#include "include/proxy-wasm/limits.h" +#include "include/proxy-wasm/word.h" + +namespace proxy_wasm { + +using Sizes = std::vector>; +using Pairs = std::vector>; + +class PairsUtil { +public: + /** + * pairsSize returns the buffer size required to serialize Pairs. + * @param pairs Pairs to serialize. + * @return size of the output buffer. + */ + template static size_t pairsSize(const Pairs &pairs) { + size_t size = sizeof(uint32_t); // number of headers + for (auto &p : pairs) { + size += 2 * sizeof(uint32_t); // size of name, size of value + size += p.first.size() + 1; // NULL-terminated name + size += p.second.size() + 1; // NULL-terminated value + } + return size; + } + + /** + * marshalPairs serializes Pairs to output buffer. + * @param pairs Pairs to serialize. + * @param buffer output buffer. + * @param size size of the output buffer. + * @return indicates whether serialization succeeded or not. + */ + template + static bool marshalPairs(const Pairs &pairs, char *buffer, size_t size) { + if (buffer == nullptr) { + return false; + } + + char *pos = buffer; + const char *end = buffer + size; + + // Write number of pairs. + uint32_t num_pairs = htowasm(pairs.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &num_pairs, sizeof(uint32_t)); + pos += sizeof(uint32_t); + + for (auto &p : pairs) { + // Write name length. + uint32_t name_len = htowasm(p.first.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &name_len, sizeof(uint32_t)); + pos += sizeof(uint32_t); + + // Write value length. + uint32_t value_len = htowasm(p.second.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &value_len, sizeof(uint32_t)); + pos += sizeof(uint32_t); + } + + for (auto &p : pairs) { + // Write name. + if (pos + p.first.size() + 1 > end) { + return false; + } + ::memcpy(pos, p.first.data(), p.first.size()); + pos += p.first.size(); + *pos++ = '\0'; // NULL-terminated string. + + // Write value. + if (pos + p.second.size() + 1 > end) { + return false; + } + ::memcpy(pos, p.second.data(), p.second.size()); + pos += p.second.size(); + *pos++ = '\0'; // NULL-terminated string. + } + + return pos == end; + } + + /** + * toPairs deserializes input buffer to Pairs. + * @param buffer serialized input buffer. + * @return deserialized Pairs. + */ + static Pairs toPairs(std::string_view buffer) { + if (buffer.data() == nullptr || buffer.size() > PROXY_WASM_HOST_PAIRS_MAX_BYTES) { + return {}; + } + + const char *pos = buffer.data(); + const char *end = buffer.data() + buffer.size(); + + // Read number of pairs. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + uint32_t num_pairs = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + + // Check if we're not going to exceed the limit. + if (num_pairs > PROXY_WASM_HOST_PAIRS_MAX_COUNT) { + return {}; + } + if (pos + num_pairs * 2 * sizeof(uint32_t) > end) { + return {}; + } + + Sizes sizes; + sizes.resize(num_pairs); + + for (auto &s : sizes) { + // Read name length. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + s.first = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + + // Read value length. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + s.second = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + } + + Pairs pairs; + pairs.resize(num_pairs); + + for (uint32_t i = 0; i < num_pairs; i++) { + auto &s = sizes[i]; + auto &p = pairs[i]; + + // Don't overread. + if (pos + s.first + 1 > end) { + return {}; + } + p.first = std::string_view(pos, s.first); + pos += s.first; + if (*pos++ != '\0') { // NULL-terminated string. + return {}; + } + + // Don't overread. + if (pos + s.second + 1 > end) { + return {}; + } + p.second = std::string_view(pos, s.second); + pos += s.second; + if (*pos++ != '\0') { // NULL-terminated string. + return {}; + } + } + + if (pos != end) { + return {}; + } + + return pairs; + } +}; + +} // namespace proxy_wasm diff --git a/src/exports.cc b/src/exports.cc index bdefddeb6..96221d33d 100644 --- a/src/exports.cc +++ b/src/exports.cc @@ -14,6 +14,7 @@ // limitations under the License. // #include "include/proxy-wasm/limits.h" +#include "include/proxy-wasm/pairs_util.h" #include "include/proxy-wasm/wasm.h" #include @@ -58,55 +59,6 @@ RegisterForeignFunction::RegisterForeignFunction(const std::string &name, WasmFo namespace exports { -namespace { - -Pairs toPairs(std::string_view buffer) { - Pairs result; - const char *b = buffer.data(); - if (buffer.size() < sizeof(uint32_t)) { - return {}; - } - auto size = wasmtoh(*reinterpret_cast(b)); - b += sizeof(uint32_t); - if (sizeof(uint32_t) + size * 2 * sizeof(uint32_t) > buffer.size()) { - return {}; - } - result.resize(size); - for (uint32_t i = 0; i < size; i++) { - result[i].first = std::string_view(nullptr, wasmtoh(*reinterpret_cast(b))); - b += sizeof(uint32_t); - result[i].second = std::string_view(nullptr, wasmtoh(*reinterpret_cast(b))); - b += sizeof(uint32_t); - } - for (auto &p : result) { - p.first = std::string_view(b, p.first.size()); - b += p.first.size() + 1; - p.second = std::string_view(b, p.second.size()); - b += p.second.size() + 1; - } - return result; -} - -template -bool getPairs(ContextBase *context, const Pairs &result, uint64_t ptr_ptr, uint64_t size_ptr) { - if (result.empty()) { - return context->wasm()->copyToPointerSize("", ptr_ptr, size_ptr); - } - uint64_t size = pairsSize(result); - uint64_t ptr = 0; - char *buffer = static_cast(context->wasm()->allocMemory(size, &ptr)); - marshalPairs(result, buffer); - if (!context->wasmVm()->setWord(ptr_ptr, Word(ptr))) { - return false; - } - if (!context->wasmVm()->setWord(size_ptr, Word(size))) { - return false; - } - return true; -} - -} // namespace - // General ABI. Word set_property(Word key_ptr, Word key_size, Word value_ptr, Word value_size) { @@ -200,7 +152,7 @@ Word send_local_response(Word response_code, Word response_code_details_ptr, if (!details || !body || !additional_response_header_pairs) { return WasmResult::InvalidMemoryAccess; } - auto additional_headers = toPairs(additional_response_header_pairs.value()); + auto additional_headers = PairsUtil::toPairs(additional_response_header_pairs.value()); context->sendLocalResponse(response_code, body.value(), std::move(additional_headers), grpc_status, details.value()); context->wasm()->stopNextIteration(true); @@ -435,7 +387,25 @@ Word get_header_map_pairs(Word type, Word ptr_ptr, Word size_ptr) { if (result != WasmResult::Ok) { return result; } - if (!getPairs(context, pairs, ptr_ptr, size_ptr)) { + if (pairs.empty()) { + if (!context->wasm()->copyToPointerSize("", ptr_ptr, size_ptr)) { + return WasmResult::InvalidMemoryAccess; + } + return WasmResult::Ok; + } + uint64_t size = PairsUtil::pairsSize(pairs); + uint64_t ptr = 0; + char *buffer = static_cast(context->wasm()->allocMemory(size, &ptr)); + if (buffer == nullptr) { + return WasmResult::InvalidMemoryAccess; + } + if (!PairsUtil::marshalPairs(pairs, buffer, size)) { + return WasmResult::InvalidMemoryAccess; + } + if (!context->wasmVm()->setWord(ptr_ptr, Word(ptr))) { + return WasmResult::InvalidMemoryAccess; + } + if (!context->wasmVm()->setWord(size_ptr, Word(size))) { return WasmResult::InvalidMemoryAccess; } return WasmResult::Ok; @@ -451,7 +421,7 @@ Word set_header_map_pairs(Word type, Word ptr, Word size) { return WasmResult::InvalidMemoryAccess; } return context->setHeaderMapPairs(static_cast(type.u64_), - toPairs(data.value())); + PairsUtil::toPairs(data.value())); } Word get_header_map_size(Word type, Word result_ptr) { @@ -541,8 +511,8 @@ Word http_call(Word uri_ptr, Word uri_size, Word header_pairs_ptr, Word header_p if (!uri || !body || !header_pairs || !trailer_pairs) { return WasmResult::InvalidMemoryAccess; } - auto headers = toPairs(header_pairs.value()); - auto trailers = toPairs(trailer_pairs.value()); + auto headers = PairsUtil::toPairs(header_pairs.value()); + auto trailers = PairsUtil::toPairs(trailer_pairs.value()); uint32_t token = 0; // NB: try to write the token to verify the memory before starting the async // operation. @@ -611,7 +581,7 @@ Word grpc_call(Word service_ptr, Word service_size, Word service_name_ptr, Word return WasmResult::InvalidMemoryAccess; } uint32_t token = 0; - auto initial_metadata = toPairs(initial_metadata_pairs.value()); + auto initial_metadata = PairsUtil::toPairs(initial_metadata_pairs.value()); auto result = context->grpcCall(service.value(), service_name.value(), method_name.value(), initial_metadata, request.value(), std::chrono::milliseconds(timeout_milliseconds), &token); @@ -637,7 +607,7 @@ Word grpc_stream(Word service_ptr, Word service_size, Word service_name_ptr, Wor return WasmResult::InvalidMemoryAccess; } uint32_t token = 0; - auto initial_metadata = toPairs(initial_metadata_pairs.value()); + auto initial_metadata = PairsUtil::toPairs(initial_metadata_pairs.value()); auto result = context->grpcStream(service.value(), service_name.value(), method_name.value(), initial_metadata, &token); if (result != WasmResult::Ok) { diff --git a/test/BUILD b/test/BUILD index 196cbc590..c39352a41 100644 --- a/test/BUILD +++ b/test/BUILD @@ -1,5 +1,6 @@ load("@proxy_wasm_cpp_host//bazel:select.bzl", "proxy_wasm_select_engine_null") load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") +load("@rules_fuzzing//fuzzing:cc_defs.bzl", "cc_fuzz_test") licenses(["notice"]) # Apache 2 @@ -31,6 +32,20 @@ cc_test( ], ) +filegroup( + name = "corpus_pairs", + srcs = glob(["corpus_pairs/**"]), +) + +cc_fuzz_test( + name = "pairs_util_fuzzer", + srcs = ["pairs_util_fuzzer.cc"], + corpus = [":corpus_pairs"], + deps = [ + "//:lib", + ], +) + cc_test( name = "signature_util_test", srcs = ["signature_util_test.cc"], diff --git a/test/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 b/test/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 new file mode 100644 index 0000000000000000000000000000000000000000..7be804a261255c5c35bf5ebcd6d32047dc1137f0 GIT binary patch literal 36 TcmZQ!VEE7Q9~C&Da{mJW1JNHQ literal 0 HcmV?d00001 diff --git a/test/pairs_util_fuzzer.cc b/test/pairs_util_fuzzer.cc new file mode 100644 index 000000000..2e2a42621 --- /dev/null +++ b/test/pairs_util_fuzzer.cc @@ -0,0 +1,46 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "include/proxy-wasm/pairs_util.h" + +#include + +namespace proxy_wasm { +namespace { + +extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { + auto input = std::string_view(reinterpret_cast(data), size); + + auto pairs = PairsUtil::toPairs(input); + + if (!pairs.empty()) { + // Verify that non-empty Pairs serializes back to the same bytes. + auto new_size = PairsUtil::pairsSize(pairs); + if (new_size != size) { + __builtin_trap(); + } + std::vector new_data(new_size); + if (!PairsUtil::marshalPairs(pairs, new_data.data(), new_data.size())) { + __builtin_trap(); + } + if (::memcmp(new_data.data(), data, size) != 0) { + __builtin_trap(); + } + } + + return 0; +} + +} // namespace +} // namespace proxy_wasm From 1ede63deb4084f510b101f43717596b76fb09f1a Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 01:10:35 -0700 Subject: [PATCH 2/9] review: add --unwindlib=libgcc. Signed-off-by: Piotr Sikora --- .bazelrc | 1 + 1 file changed, 1 insertion(+) diff --git a/.bazelrc b/.bazelrc index 038141be3..a8212233d 100644 --- a/.bazelrc +++ b/.bazelrc @@ -17,6 +17,7 @@ build:clang-xsan --copt -fno-sanitize-recover=all build:clang-xsan --linkopt -fsanitize-link-c++-runtime build:clang-xsan --linkopt -fuse-ld=lld build:clang-xsan --linkopt -rtlib=compiler-rt +build:clang-xsan --linkopt --unwindlib=libgcc # Use Clang compiler with Address and Undefined Behavior Sanitizers. build:clang-asan --config=clang-xsan From 9dec346917ec575187afb09c6fba7d8e90004fca Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 02:05:56 -0700 Subject: [PATCH 3/9] review: avoid using templates. Signed-off-by: Piotr Sikora --- BUILD | 1 + include/proxy-wasm/pairs_util.h | 154 +++------------------------- src/pairs_util.cc | 171 ++++++++++++++++++++++++++++++++ test/pairs_util_fuzzer.cc | 2 +- 4 files changed, 185 insertions(+), 143 deletions(-) create mode 100644 src/pairs_util.cc diff --git a/BUILD b/BUILD index b2e8fdbfb..46edf1d1c 100644 --- a/BUILD +++ b/BUILD @@ -52,6 +52,7 @@ cc_library( "src/bytecode_util.cc", "src/context.cc", "src/exports.cc", + "src/pairs_util.cc", "src/shared_data.cc", "src/shared_data.h", "src/shared_queue.cc", diff --git a/include/proxy-wasm/pairs_util.h b/include/proxy-wasm/pairs_util.h index c8f445b64..90c91b869 100644 --- a/include/proxy-wasm/pairs_util.h +++ b/include/proxy-wasm/pairs_util.h @@ -15,17 +15,14 @@ #pragma once -#include +#include #include #include -#include "include/proxy-wasm/limits.h" -#include "include/proxy-wasm/word.h" - namespace proxy_wasm { -using Sizes = std::vector>; using Pairs = std::vector>; +using StringPairs = std::vector>; class PairsUtil { public: @@ -34,14 +31,11 @@ class PairsUtil { * @param pairs Pairs to serialize. * @return size of the output buffer. */ - template static size_t pairsSize(const Pairs &pairs) { - size_t size = sizeof(uint32_t); // number of headers - for (auto &p : pairs) { - size += 2 * sizeof(uint32_t); // size of name, size of value - size += p.first.size() + 1; // NULL-terminated name - size += p.second.size() + 1; // NULL-terminated value - } - return size; + static size_t pairsSize(const Pairs &pairs); + + static size_t pairsSize(const StringPairs &stringpairs) { + Pairs views(stringpairs.begin(), stringpairs.end()); + return pairsSize(views); } /** @@ -51,60 +45,11 @@ class PairsUtil { * @param size size of the output buffer. * @return indicates whether serialization succeeded or not. */ - template - static bool marshalPairs(const Pairs &pairs, char *buffer, size_t size) { - if (buffer == nullptr) { - return false; - } - - char *pos = buffer; - const char *end = buffer + size; - - // Write number of pairs. - uint32_t num_pairs = htowasm(pairs.size()); - if (pos + sizeof(uint32_t) > end) { - return false; - } - ::memcpy(pos, &num_pairs, sizeof(uint32_t)); - pos += sizeof(uint32_t); - - for (auto &p : pairs) { - // Write name length. - uint32_t name_len = htowasm(p.first.size()); - if (pos + sizeof(uint32_t) > end) { - return false; - } - ::memcpy(pos, &name_len, sizeof(uint32_t)); - pos += sizeof(uint32_t); - - // Write value length. - uint32_t value_len = htowasm(p.second.size()); - if (pos + sizeof(uint32_t) > end) { - return false; - } - ::memcpy(pos, &value_len, sizeof(uint32_t)); - pos += sizeof(uint32_t); - } + static bool marshalPairs(const Pairs &pairs, char *buffer, size_t size); - for (auto &p : pairs) { - // Write name. - if (pos + p.first.size() + 1 > end) { - return false; - } - ::memcpy(pos, p.first.data(), p.first.size()); - pos += p.first.size(); - *pos++ = '\0'; // NULL-terminated string. - - // Write value. - if (pos + p.second.size() + 1 > end) { - return false; - } - ::memcpy(pos, p.second.data(), p.second.size()); - pos += p.second.size(); - *pos++ = '\0'; // NULL-terminated string. - } - - return pos == end; + static bool marshalPairs(const StringPairs &stringpairs, char *buffer, size_t size) { + Pairs views(stringpairs.begin(), stringpairs.end()); + return marshalPairs(views, buffer, size); } /** @@ -112,82 +57,7 @@ class PairsUtil { * @param buffer serialized input buffer. * @return deserialized Pairs. */ - static Pairs toPairs(std::string_view buffer) { - if (buffer.data() == nullptr || buffer.size() > PROXY_WASM_HOST_PAIRS_MAX_BYTES) { - return {}; - } - - const char *pos = buffer.data(); - const char *end = buffer.data() + buffer.size(); - - // Read number of pairs. - if (pos + sizeof(uint32_t) > end) { - return {}; - } - uint32_t num_pairs = wasmtoh(*reinterpret_cast(pos)); - pos += sizeof(uint32_t); - - // Check if we're not going to exceed the limit. - if (num_pairs > PROXY_WASM_HOST_PAIRS_MAX_COUNT) { - return {}; - } - if (pos + num_pairs * 2 * sizeof(uint32_t) > end) { - return {}; - } - - Sizes sizes; - sizes.resize(num_pairs); - - for (auto &s : sizes) { - // Read name length. - if (pos + sizeof(uint32_t) > end) { - return {}; - } - s.first = wasmtoh(*reinterpret_cast(pos)); - pos += sizeof(uint32_t); - - // Read value length. - if (pos + sizeof(uint32_t) > end) { - return {}; - } - s.second = wasmtoh(*reinterpret_cast(pos)); - pos += sizeof(uint32_t); - } - - Pairs pairs; - pairs.resize(num_pairs); - - for (uint32_t i = 0; i < num_pairs; i++) { - auto &s = sizes[i]; - auto &p = pairs[i]; - - // Don't overread. - if (pos + s.first + 1 > end) { - return {}; - } - p.first = std::string_view(pos, s.first); - pos += s.first; - if (*pos++ != '\0') { // NULL-terminated string. - return {}; - } - - // Don't overread. - if (pos + s.second + 1 > end) { - return {}; - } - p.second = std::string_view(pos, s.second); - pos += s.second; - if (*pos++ != '\0') { // NULL-terminated string. - return {}; - } - } - - if (pos != end) { - return {}; - } - - return pairs; - } + static Pairs toPairs(std::string_view buffer); }; } // namespace proxy_wasm diff --git a/src/pairs_util.cc b/src/pairs_util.cc new file mode 100644 index 000000000..63f411317 --- /dev/null +++ b/src/pairs_util.cc @@ -0,0 +1,171 @@ +// Copyright 2016-2019 Envoy Project Authors +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "include/proxy-wasm/pairs_util.h" + +#include +#include +#include + +#include "include/proxy-wasm/limits.h" +#include "include/proxy-wasm/word.h" + +namespace proxy_wasm { + +using Sizes = std::vector>; + +size_t PairsUtil::pairsSize(const Pairs &pairs) { + size_t size = sizeof(uint32_t); // number of headers + for (auto &p : pairs) { + size += 2 * sizeof(uint32_t); // size of name, size of value + size += p.first.size() + 1; // NULL-terminated name + size += p.second.size() + 1; // NULL-terminated value + } + return size; +} + +bool PairsUtil::marshalPairs(const Pairs &pairs, char *buffer, size_t size) { + if (buffer == nullptr) { + return false; + } + + char *pos = buffer; + const char *end = buffer + size; + + // Write number of pairs. + uint32_t num_pairs = htowasm(pairs.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &num_pairs, sizeof(uint32_t)); + pos += sizeof(uint32_t); + + for (auto &p : pairs) { + // Write name length. + uint32_t name_len = htowasm(p.first.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &name_len, sizeof(uint32_t)); + pos += sizeof(uint32_t); + + // Write value length. + uint32_t value_len = htowasm(p.second.size()); + if (pos + sizeof(uint32_t) > end) { + return false; + } + ::memcpy(pos, &value_len, sizeof(uint32_t)); + pos += sizeof(uint32_t); + } + + for (auto &p : pairs) { + // Write name. + if (pos + p.first.size() + 1 > end) { + return false; + } + ::memcpy(pos, p.first.data(), p.first.size()); + pos += p.first.size(); + *pos++ = '\0'; // NULL-terminated string. + + // Write value. + if (pos + p.second.size() + 1 > end) { + return false; + } + ::memcpy(pos, p.second.data(), p.second.size()); + pos += p.second.size(); + *pos++ = '\0'; // NULL-terminated string. + } + + return pos == end; +} + +Pairs PairsUtil::toPairs(std::string_view buffer) { + if (buffer.data() == nullptr || buffer.size() > PROXY_WASM_HOST_PAIRS_MAX_BYTES) { + return {}; + } + + const char *pos = buffer.data(); + const char *end = buffer.data() + buffer.size(); + + // Read number of pairs. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + uint32_t num_pairs = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + + // Check if we're not going to exceed the limit. + if (num_pairs > PROXY_WASM_HOST_PAIRS_MAX_COUNT) { + return {}; + } + if (pos + num_pairs * 2 * sizeof(uint32_t) > end) { + return {}; + } + + Sizes sizes; + sizes.resize(num_pairs); + + for (auto &s : sizes) { + // Read name length. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + s.first = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + + // Read value length. + if (pos + sizeof(uint32_t) > end) { + return {}; + } + s.second = wasmtoh(*reinterpret_cast(pos)); + pos += sizeof(uint32_t); + } + + Pairs pairs; + pairs.resize(num_pairs); + + for (uint32_t i = 0; i < num_pairs; i++) { + auto &s = sizes[i]; + auto &p = pairs[i]; + + // Don't overread. + if (pos + s.first + 1 > end) { + return {}; + } + p.first = std::string_view(pos, s.first); + pos += s.first; + if (*pos++ != '\0') { // NULL-terminated string. + return {}; + } + + // Don't overread. + if (pos + s.second + 1 > end) { + return {}; + } + p.second = std::string_view(pos, s.second); + pos += s.second; + if (*pos++ != '\0') { // NULL-terminated string. + return {}; + } + } + + if (pos != end) { + return {}; + } + + return pairs; +} + +} // namespace proxy_wasm diff --git a/test/pairs_util_fuzzer.cc b/test/pairs_util_fuzzer.cc index 2e2a42621..aaef0d099 100644 --- a/test/pairs_util_fuzzer.cc +++ b/test/pairs_util_fuzzer.cc @@ -14,7 +14,7 @@ #include "include/proxy-wasm/pairs_util.h" -#include +#include namespace proxy_wasm { namespace { From dc90a09a0aca58ac379a8daf61a10a36d3e6d391 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 03:29:44 -0700 Subject: [PATCH 4/9] review: fix clang-tidy errors. Signed-off-by: Piotr Sikora --- src/pairs_util.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pairs_util.cc b/src/pairs_util.cc index 63f411317..d49259216 100644 --- a/src/pairs_util.cc +++ b/src/pairs_util.cc @@ -28,7 +28,7 @@ using Sizes = std::vector>; size_t PairsUtil::pairsSize(const Pairs &pairs) { size_t size = sizeof(uint32_t); // number of headers - for (auto &p : pairs) { + for (const auto &p : pairs) { size += 2 * sizeof(uint32_t); // size of name, size of value size += p.first.size() + 1; // NULL-terminated name size += p.second.size() + 1; // NULL-terminated value @@ -52,7 +52,7 @@ bool PairsUtil::marshalPairs(const Pairs &pairs, char *buffer, size_t size) { ::memcpy(pos, &num_pairs, sizeof(uint32_t)); pos += sizeof(uint32_t); - for (auto &p : pairs) { + for (const auto &p : pairs) { // Write name length. uint32_t name_len = htowasm(p.first.size()); if (pos + sizeof(uint32_t) > end) { @@ -70,7 +70,7 @@ bool PairsUtil::marshalPairs(const Pairs &pairs, char *buffer, size_t size) { pos += sizeof(uint32_t); } - for (auto &p : pairs) { + for (const auto &p : pairs) { // Write name. if (pos + p.first.size() + 1 > end) { return false; From c0788fa29fdd89493db331c60f7faa614ab29868 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 03:28:04 -0700 Subject: [PATCH 5/9] review: don't build fuzzers on Windows. Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 4 +++- test/BUILD | 15 -------------- test/fuzz/BUILD | 19 ++++++++++++++++++ ...h-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 | Bin test/{ => fuzz}/pairs_util_fuzzer.cc | 0 5 files changed, 22 insertions(+), 16 deletions(-) create mode 100644 test/fuzz/BUILD rename test/{ => fuzz}/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 (100%) rename test/{ => fuzz}/pairs_util_fuzzer.cc (100%) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 55c9822eb..8c3be9552 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -125,6 +125,7 @@ jobs: os: windows-2019 arch: x86_64 action: test + targets: -//test/fuzz/... - name: 'V8 on Linux/x86_64' engine: 'v8' repo: 'v8' @@ -228,6 +229,7 @@ jobs: os: windows-2019 arch: x86_64 action: test + targets: -//test/fuzz/... - name: 'WAVM on Linux/x86_64' engine: 'wavm' repo: 'com_github_wavm_wavm' @@ -287,7 +289,7 @@ jobs: --test_output=errors --define engine=${{ matrix.engine }} ${{ matrix.flags }} - //test/... + //test/... ${{ matrix.targets }} - name: Bazel build/test (signed Wasm module) if: ${{ matrix.engine != 'null' && !startsWith(matrix.os, 'windows') }} diff --git a/test/BUILD b/test/BUILD index c39352a41..196cbc590 100644 --- a/test/BUILD +++ b/test/BUILD @@ -1,6 +1,5 @@ load("@proxy_wasm_cpp_host//bazel:select.bzl", "proxy_wasm_select_engine_null") load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") -load("@rules_fuzzing//fuzzing:cc_defs.bzl", "cc_fuzz_test") licenses(["notice"]) # Apache 2 @@ -32,20 +31,6 @@ cc_test( ], ) -filegroup( - name = "corpus_pairs", - srcs = glob(["corpus_pairs/**"]), -) - -cc_fuzz_test( - name = "pairs_util_fuzzer", - srcs = ["pairs_util_fuzzer.cc"], - corpus = [":corpus_pairs"], - deps = [ - "//:lib", - ], -) - cc_test( name = "signature_util_test", srcs = ["signature_util_test.cc"], diff --git a/test/fuzz/BUILD b/test/fuzz/BUILD new file mode 100644 index 000000000..bf7699bbc --- /dev/null +++ b/test/fuzz/BUILD @@ -0,0 +1,19 @@ +load("@rules_fuzzing//fuzzing:cc_defs.bzl", "cc_fuzz_test") + +licenses(["notice"]) # Apache 2 + +package(default_visibility = ["//visibility:public"]) + +filegroup( + name = "corpus_pairs", + srcs = glob(["corpus_pairs/**"]), +) + +cc_fuzz_test( + name = "pairs_util_fuzzer", + srcs = ["pairs_util_fuzzer.cc"], + corpus = [":corpus_pairs"], + deps = [ + "//:lib", + ], +) diff --git a/test/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 b/test/fuzz/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 similarity index 100% rename from test/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 rename to test/fuzz/corpus_pairs/crash-0bf4371e72f7ae83f9c5bf3e2485531cdcaaaa04 diff --git a/test/pairs_util_fuzzer.cc b/test/fuzz/pairs_util_fuzzer.cc similarity index 100% rename from test/pairs_util_fuzzer.cc rename to test/fuzz/pairs_util_fuzzer.cc From f3d76d635809df561742e66c4907e39a010bb437 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 03:58:47 -0700 Subject: [PATCH 6/9] review: fix syntax. Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8c3be9552..853b51bbf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -289,7 +289,7 @@ jobs: --test_output=errors --define engine=${{ matrix.engine }} ${{ matrix.flags }} - //test/... ${{ matrix.targets }} + -- //test/... ${{ matrix.targets }} - name: Bazel build/test (signed Wasm module) if: ${{ matrix.engine != 'null' && !startsWith(matrix.os, 'windows') }} From d5b2dde17738e044befe63b73c32b7febe9db811 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 10:37:52 -0700 Subject: [PATCH 7/9] review: fix syntax, maybe. Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 853b51bbf..95778b1f2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -282,6 +282,7 @@ jobs: done - name: Bazel build/test + shell: bash run: > ${{ matrix.run_under }} bazel ${{ matrix.action }} From bd4c6264b364429d53e513077b552dd9684fd0da Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 10:37:09 -0700 Subject: [PATCH 8/9] review: don't run fuzzers in cross-compiled builds. Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 95778b1f2..3ed6ca714 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -156,6 +156,7 @@ jobs: os: ubuntu-20.04 arch: aarch64 action: test + targets: -//test/fuzz/... flags: --config=zig-cc-linux-aarch64 --@v8//bazel/config:v8_target_cpu=arm64 deps: qemu-user-static libc6-arm64-cross cache: true From 35ea3e3322c0642f1c4891d2ca0d4abbda0e520e Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 4 Aug 2022 10:46:37 -0700 Subject: [PATCH 9/9] review: add Mike's comment. Signed-off-by: Piotr Sikora --- include/proxy-wasm/pairs_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/proxy-wasm/pairs_util.h b/include/proxy-wasm/pairs_util.h index 90c91b869..54df2fd88 100644 --- a/include/proxy-wasm/pairs_util.h +++ b/include/proxy-wasm/pairs_util.h @@ -55,7 +55,7 @@ class PairsUtil { /** * toPairs deserializes input buffer to Pairs. * @param buffer serialized input buffer. - * @return deserialized Pairs. + * @return deserialized Pairs or an empty instance in case of deserialization failure. */ static Pairs toPairs(std::string_view buffer); };