From 3fe8ca126025c7377e7a8a5a4f24a228d77dea17 Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Wed, 1 Nov 2023 15:55:11 +0000 Subject: [PATCH] [release/4.x] Cherry pick: Do not enforce default parsing limits on forwarded traffic (#5803) (#5806) --- .daily_canary | 2 +- CHANGELOG.md | 6 +++ CMakeLists.txt | 7 +++ include/ccf/ds/unit_strings.h | 5 ++ include/ccf/http_configuration.h | 16 ++++++ src/http/http_rpc_context.h | 3 +- src/node/rpc/forwarder.h | 5 ++ tests/limits.py | 83 ++++++++++++++++++++++++++++++++ 8 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 tests/limits.py diff --git a/.daily_canary b/.daily_canary index d7f3a48b3274..3fe7c25382eb 100644 --- a/.daily_canary +++ b/.daily_canary @@ -1,4 +1,4 @@ -^- _X_ ___ (- -) (= =) | Y & +--? ( V ) / . \ | +---=---' -/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]] +/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]]{{}}--|| diff --git a/CHANGELOG.md b/CHANGELOG.md index 48ffe3ed355e..5b15424c32eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [4.0.12] + +[4.0.12]: https://github.com/microsoft/CCF/releases/tag/ccf-4.0.12 + +- Lifted parser size limits on forwarded request from default values to more permissive ones. Note that the limits set out on the interface of the inbound node still apply (#5803). + ## [4.0.11] [4.0.11]: https://github.com/microsoft/CCF/releases/tag/ccf-4.0.11 diff --git a/CMakeLists.txt b/CMakeLists.txt index eb7554b1b444..663092aa8d5b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1344,6 +1344,13 @@ if(BUILD_TESTS) ${CMAKE_SOURCE_DIR}/samples/apps/logging/js ) + if(NOT SAN) + add_e2e_test( + NAME e2e_limits PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/limits.py + CONSENSUS cft + ) + endif() + add_e2e_test( NAME e2e_logging_http2 PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/e2e_logging.py diff --git a/include/ccf/ds/unit_strings.h b/include/ccf/ds/unit_strings.h index 689f63551dc3..6a24028715ac 100644 --- a/include/ccf/ds/unit_strings.h +++ b/include/ccf/ds/unit_strings.h @@ -128,6 +128,11 @@ namespace ds value(convert_size_string(str_)) {} + SizeString(const char* str_) : + UnitString(str_), + value(convert_size_string(str_)) + {} + inline operator size_t() const { return value; diff --git a/include/ccf/http_configuration.h b/include/ccf/http_configuration.h index 595581e6b580..535257c051eb 100644 --- a/include/ccf/http_configuration.h +++ b/include/ccf/http_configuration.h @@ -9,6 +9,8 @@ namespace http { + // Default parser limits, used as a DoS protection against + // requests that are too large. static const ds::SizeString default_max_body_size = {"1MB"}; static const ds::SizeString default_max_header_size = {"16KB"}; static const uint32_t default_max_headers_count = 256; @@ -43,4 +45,18 @@ namespace http max_concurrent_streams_count, initial_window_size, max_frame_size); + + // A permissive configuration, used for internally forwarded requests + // that have already been through application-defined limits. + static ParserConfiguration permissive_configuration() + { + ParserConfiguration config; + config.max_body_size = "1GB"; + config.max_header_size = "100MB"; + config.max_headers_count = 1024; + config.max_concurrent_streams_count = 1; + config.initial_window_size = "64KB"; + config.max_frame_size = "16MB"; + return config; + } } \ No newline at end of file diff --git a/src/http/http_rpc_context.h b/src/http/http_rpc_context.h index c1496f4f39ae..8098355d084a 100644 --- a/src/http/http_rpc_context.h +++ b/src/http/http_rpc_context.h @@ -374,8 +374,7 @@ namespace ccf std::shared_ptr s, const std::vector& packed) { http::SimpleRequestProcessor processor; - http::RequestParser parser(processor); - + http::RequestParser parser(processor, http::permissive_configuration()); parser.execute(packed.data(), packed.size()); if (processor.received.size() != 1) diff --git a/src/node/rpc/forwarder.h b/src/node/rpc/forwarder.h index 35e469404232..ddc7121e6a86 100644 --- a/src/node/rpc/forwarder.h +++ b/src/node/rpc/forwarder.h @@ -237,6 +237,11 @@ namespace ccf return ccf::make_fwd_rpc_context( session, raw_request, r.first.frame_format); } + catch (const http::RequestTooLargeException& rexc) + { + LOG_FAIL_FMT("Forwarded request exceeded limit: {}", rexc.what()); + return nullptr; + } catch (const std::exception& err) { LOG_FAIL_FMT("Invalid forwarded request"); diff --git a/tests/limits.py b/tests/limits.py new file mode 100644 index 000000000000..93cf69ddd118 --- /dev/null +++ b/tests/limits.py @@ -0,0 +1,83 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the Apache 2.0 License. +import infra.network +import infra.e2e_args +import infra.checker +import infra.jwt_issuer +import infra.proc +import http +import infra.clients +import infra.crypto +from infra.runner import ConcurrentRunner +import copy + + +def test_forward_larger_than_default_requests(network, args): + new_node = network.create_node( + infra.interfaces.HostSpec( + rpc_interfaces={ + infra.interfaces.PRIMARY_RPC_INTERFACE: infra.interfaces.RPCInterface( + max_http_body_size=10 * 1024 * 1024, + # Deliberately large because some builds (eg. SGX Debug) take + # a long time to process large requests + forwarding_timeout_ms=8000, + ) + } + ) + ) + network.join_node(new_node, args.package, args) + network.trust_node(new_node, args) + + primary, _ = network.find_primary() + + # Big request, but under the cap + with primary.client("user0") as c: + msg = "A" * 512 * 1024 + r = c.post("/app/log/private", {"id": 42, "msg": msg}) + assert r.status_code == http.HTTPStatus.OK.value, r + + # Big request, over the cap for the primary + with primary.client("user0") as c: + msg = "A" * 2 * 1024 * 1024 + r = c.post("/app/log/private", {"id": 42, "msg": msg}) + assert r.status_code == http.HTTPStatus.REQUEST_ENTITY_TOO_LARGE.value, r + + # Big request, over the cap for the primary, but under the cap for the new node + with new_node.client("user0") as c: + msg = "A" * 2 * 1024 * 1024 + r = c.post("/app/log/private", {"id": 42, "msg": msg}) + assert r.status_code == http.HTTPStatus.OK.value, r + + +def run_parser_limits_checks(args): + new_args = copy.copy(args) + # Deliberately large because some builds (eg. SGX Debug) take + # a long time to process large requests + new_args.election_timeout_ms = 10000 + new_args.host_log_level = "info" + new_args.enclave_log_level = "info" + with infra.network.network( + new_args.nodes, + new_args.binary_dir, + new_args.debug_nodes, + new_args.perf_nodes, + pdb=args.pdb, + ) as network: + network.start_and_open(new_args) + + test_forward_larger_than_default_requests(network, new_args) + + +if __name__ == "__main__": + cr = ConcurrentRunner() + + if not cr.args.http2: + # No support for forwarding with HTTP/2 + cr.add( + "parser_limits", + run_parser_limits_checks, + package="samples/apps/logging/liblogging", + nodes=infra.e2e_args.max_nodes(cr.args, f=0), + ) + + cr.run()