This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 49085f19 Reject deeply nested JSON replies in naming services (#3592)
49085f19 is described below
commit 49085f190dedc36e09546c7ef0833bf2b97aad68
Author: Weibing Wang <[email protected]>
AuthorDate: Fri Oct 9 11:10:33 2026 +0800
Reject deeply nested JSON replies in naming services (#3592)
* Reject deeply nested JSON replies in naming services
The consul, nacos and discovery naming services parsed registry replies
with rapidjson's default recursive parser, so a deeply nested reply
crashed the consuming process with stack exhaustion. rapidjson also
builds, visits and destroys its DOM recursively, so iterative parsing
alone is not enough.
Route all the reply parsing through a shared ParseNamingServiceJson()
which parses iteratively (kParseIterativeFlag) and rejects replies
nested deeper than kMaxNamingServiceJsonDepth (100, same default as
json2pb) before parsing, bounding every recursion on the document.
Legit replies of these services are only a few levels deep.
* Address review: reject raw NUL and clamp depth in JSON pre-scan
- A raw NUL is invalid JSON anywhere and rapidjson treats it as
end-of-input, so reject replies containing one up front: the depth
scan and the parser now accept exactly the same bytes.
- Don't let the depth counter go negative on malformed text with more
closing than opening brackets.
- Reword the comment about the state of `doc' when the depth limit is
exceeded: it is untouched rather than cleared.
---------
Co-authored-by: brpc-oncall <brpc-oncall@localhost>
---
src/brpc/policy/consul_naming_service.cpp | 8 +-
src/brpc/policy/discovery_naming_service.cpp | 16 ++-
src/brpc/policy/nacos_naming_service.cpp | 7 +-
src/brpc/policy/naming_service_json.h | 95 ++++++++++++++++++
test/brpc_naming_service_unittest.cpp | 140 +++++++++++++++++++++++++++
5 files changed, 258 insertions(+), 8 deletions(-)
diff --git a/src/brpc/policy/consul_naming_service.cpp
b/src/brpc/policy/consul_naming_service.cpp
index bf35f5bb..ed29bf11 100644
--- a/src/brpc/policy/consul_naming_service.cpp
+++ b/src/brpc/policy/consul_naming_service.cpp
@@ -29,6 +29,7 @@
#include "brpc/channel.h"
#include "brpc/policy/file_naming_service.h"
#include "brpc/policy/consul_naming_service.h"
+#include "brpc/policy/naming_service_json.h"
namespace brpc {
@@ -129,7 +130,12 @@ int ConsulNamingService::GetServers(const char*
service_name,
std::set<ServerNode> presence;
BUTIL_RAPIDJSON_NAMESPACE::Document services;
- services.Parse(cntl.response_attachment().to_string().c_str());
+ if (!ParseNamingServiceJson(cntl.response_attachment().to_string(),
+ &services)) {
+ LOG(ERROR) << "Failed to parse the consul's response for "
+ << service_name << " as json";
+ return -1;
+ }
if (!services.IsArray()) {
LOG(ERROR) << "The consul's response for "
<< service_name << " is not a json array";
diff --git a/src/brpc/policy/discovery_naming_service.cpp
b/src/brpc/policy/discovery_naming_service.cpp
index 98751130..42772ff0 100644
--- a/src/brpc/policy/discovery_naming_service.cpp
+++ b/src/brpc/policy/discovery_naming_service.cpp
@@ -27,6 +27,7 @@
#include "brpc/channel.h"
#include "brpc/controller.h"
#include "brpc/policy/discovery_naming_service.h"
+#include "brpc/policy/naming_service_json.h"
namespace brpc {
namespace policy {
@@ -72,7 +73,10 @@ static int ListDiscoveryNodes(const char*
discovery_api_addr, std::string* serve
const std::string response = cntl.response_attachment().to_string();
BUTIL_RAPIDJSON_NAMESPACE::Document d;
- d.Parse(response.c_str());
+ if (!ParseNamingServiceJson(response, &d)) {
+ LOG(ERROR) << "Fail to parse response of discovery nodes as json";
+ return -1;
+ }
if (!d.IsObject()) {
LOG(ERROR) << "Fail to parse " << response << " as json object";
return -1;
@@ -160,7 +164,10 @@ DiscoveryClient::~DiscoveryClient() {
static int ParseCommonResult(const butil::IOBuf& buf, std::string* error_text)
{
const std::string s = buf.to_string();
BUTIL_RAPIDJSON_NAMESPACE::Document d;
- d.Parse(s.c_str());
+ if (!ParseNamingServiceJson(s, &d)) {
+ LOG(ERROR) << "Fail to parse " << buf << " as json";
+ return -1;
+ }
if (!d.IsObject()) {
LOG(ERROR) << "Fail to parse " << buf << " as json object";
return -1;
@@ -383,7 +390,10 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
const std::string response = cntl.response_attachment().to_string();
BUTIL_RAPIDJSON_NAMESPACE::Document d;
- d.Parse(response.c_str());
+ if (!ParseNamingServiceJson(response, &d)) {
+ LOG(ERROR) << "Fail to parse response of /discovery/fetchs as json";
+ return -1;
+ }
if (!d.IsObject()) {
LOG(ERROR) << "Fail to parse " << response << " as json object";
return -1;
diff --git a/src/brpc/policy/nacos_naming_service.cpp
b/src/brpc/policy/nacos_naming_service.cpp
index 95c3e4d5..3c05e97e 100644
--- a/src/brpc/policy/nacos_naming_service.cpp
+++ b/src/brpc/policy/nacos_naming_service.cpp
@@ -23,6 +23,7 @@
#include "brpc/http_status_code.h"
#include "brpc/log.h"
+#include "brpc/policy/naming_service_json.h"
#include "butil/iobuf.h"
#include "butil/logging.h"
#include "butil/third_party/rapidjson/document.h"
@@ -75,8 +76,7 @@ int NacosNamingService::RefreshAccessToken(const char
*service_name) {
}
BUTIL_RAPIDJSON_NAMESPACE::Document doc;
- if (doc.Parse(cntl.response_attachment().to_string().c_str())
- .HasParseError()) {
+ if (!ParseNamingServiceJson(cntl.response_attachment().to_string(), &doc))
{
LOG(ERROR) << "Failed to parse nacos auth response";
return -1;
}
@@ -133,8 +133,7 @@ int NacosNamingService::GetServerNodes(const char
*service_name,
}
BUTIL_RAPIDJSON_NAMESPACE::Document doc;
- if (doc.Parse(cntl.response_attachment().to_string().c_str())
- .HasParseError()) {
+ if (!ParseNamingServiceJson(cntl.response_attachment().to_string(), &doc))
{
LOG(ERROR) << "Failed to parse nacos response";
return -1;
}
diff --git a/src/brpc/policy/naming_service_json.h
b/src/brpc/policy/naming_service_json.h
new file mode 100644
index 00000000..2587fb9d
--- /dev/null
+++ b/src/brpc/policy/naming_service_json.h
@@ -0,0 +1,95 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string.h>
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ // Don't go negative on malformed text with more closing than
+ // opening brackets.
+ if (depth > 0) {
+ --depth;
+ }
+ }
+ }
+ return max_depth;
+}
+
+// Parse a JSON reply of a naming service into `doc' iteratively. Returns
+// false if `text' is not valid JSON or is nested deeper than
+// kMaxNamingServiceJsonDepth; `doc' is untouched in the latter case.
+inline bool ParseNamingServiceJson(const std::string& text,
+ BUTIL_RAPIDJSON_NAMESPACE::Document* doc) {
+ // A raw NUL is not valid JSON anywhere, and rapidjson takes it as
+ // end-of-input, so reject it to keep the scanned and parsed bytes
+ // consistent.
+ if (memchr(text.data(), '\0', text.size()) != nullptr ||
+ MaxJsonDepth(text) > kMaxNamingServiceJsonDepth) {
+ return false;
+ }
+ doc->Parse<BUTIL_RAPIDJSON_NAMESPACE::kParseIterativeFlag>(text.c_str());
+ return !doc->HasParseError();
+}
+
+} // namespace policy
+} // namespace brpc
+
+#endif // BRPC_POLICY_NAMING_SERVICE_JSON_H
diff --git a/test/brpc_naming_service_unittest.cpp
b/test/brpc_naming_service_unittest.cpp
index d54f39d4..2c5ad76d 100644
--- a/test/brpc_naming_service_unittest.cpp
+++ b/test/brpc_naming_service_unittest.cpp
@@ -33,6 +33,7 @@
#include "brpc/policy/remote_file_naming_service.h"
#include "brpc/policy/discovery_naming_service.h"
#include "brpc/policy/nacos_naming_service.h"
+#include "brpc/policy/naming_service_json.h"
#include "echo.pb.h"
#include "brpc/server.h"
@@ -288,6 +289,45 @@ TEST(NamingServiceTest, remotefile) {
}
}
+// Build a JSON array nested `depth' levels deep, e.g. depth=2 -> "[[]]".
+static std::string MakeDeeplyNestedJson(int depth) {
+ return std::string(depth, '[') + std::string(depth, ']');
+}
+
+TEST(NamingServiceTest, naming_service_json_depth_limit) {
+ BUTIL_RAPIDJSON_NAMESPACE::Document doc;
+ // Well-formed replies are parsed.
+ EXPECT_TRUE(brpc::policy::ParseNamingServiceJson(
+ R"({"hosts":[{"ip":"127.0.0.1","port":8888}]})", &doc));
+ EXPECT_TRUE(doc.IsObject());
+ // Malformed replies are rejected.
+ EXPECT_FALSE(brpc::policy::ParseNamingServiceJson(
+ R"({"hosts":[)" + MakeDeeplyNestedJson(1), &doc));
+ // Brackets and braces inside strings don't count towards the depth.
+ EXPECT_TRUE(brpc::policy::ParseNamingServiceJson(
+ R"({"key":"a\\b\"c[[[{{"})", &doc));
+ // Replies at the depth limit are still accepted ...
+ EXPECT_TRUE(brpc::policy::ParseNamingServiceJson(
+ MakeDeeplyNestedJson(brpc::policy::kMaxNamingServiceJsonDepth), &doc));
+ // ... while deeper ones are rejected before parsing, so that neither the
+ // parser nor the DOM built/visited/destroyed recursively by rapidjson
+ // exhausts the stack.
+ EXPECT_FALSE(brpc::policy::ParseNamingServiceJson(
+ MakeDeeplyNestedJson(brpc::policy::kMaxNamingServiceJsonDepth + 1),
+ &doc));
+ // A deeply nested reply must be rejected without stack overflow.
+ EXPECT_FALSE(brpc::policy::ParseNamingServiceJson(
+ MakeDeeplyNestedJson(140000), &doc));
+ // A raw NUL is invalid JSON and must not let a suffix bypass parsing.
+ std::string with_nul(R"({"hosts":[{}]})");
+ with_nul += '\0';
+ with_nul += MakeDeeplyNestedJson(140000);
+ EXPECT_FALSE(brpc::policy::ParseNamingServiceJson(with_nul, &doc));
+ // Extra closing brackets must not underflow the depth counter.
+ EXPECT_FALSE(brpc::policy::ParseNamingServiceJson(
+ "]]]]]]" + MakeDeeplyNestedJson(1), &doc));
+}
+
class ConsulNamingServiceImpl : public test::UserNamingService {
public:
ConsulNamingServiceImpl() : list_names_count(0), touch_count(0) {
@@ -493,6 +533,26 @@ TEST(NamingServiceTest, consul_response_object_types) {
}
}
+TEST(NamingServiceTest, consul_deeply_nested_reply) {
+ GFLAGS_NAMESPACE::FlagSaver flags_saver;
+ brpc::policy::FLAGS_consul_enable_degrade_to_file_naming_service = false;
+ brpc::policy::FLAGS_consul_service_discovery_url = "/v1/health/service/";
+ ConsulResponseService svc(MakeDeeplyNestedJson(140000));
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
+ "/v1/health/service/test => ListNames"));
+ ASSERT_EQ(0, server.Start(0, nullptr));
+ brpc::policy::FLAGS_consul_agent_addr = butil::string_printf(
+ "http://%s";, butil::endpoint2str(server.listen_address()).c_str());
+
+ // The deeply nested reply must be rejected instead of crashing on
+ // unbounded recursion.
+ brpc::policy::ConsulNamingService cns;
+ std::vector<brpc::ServerNode> servers;
+ ASSERT_EQ(-1, cns.GetServers("test", &servers));
+ EXPECT_TRUE(servers.empty());
+}
+
TEST(NamingServiceTest, consul_with_backup_file) {
GFLAGS_NAMESPACE::FlagSaver flags_saver;
brpc::policy::FLAGS_consul_enable_degrade_to_file_naming_service = true;
@@ -814,6 +874,50 @@ TEST(NamingServiceTest, discovery_sanity) {
}
}
+class DeepReplyDiscoveryServiceImpl : public DiscoveryNamingServiceImpl {
+public:
+ void Fetchs(google::protobuf::RpcController* cntl_base,
+ const test::HttpRequest*,
+ test::HttpResponse*,
+ google::protobuf::Closure* done) {
+ brpc::ClosureGuard done_guard(done);
+ brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
+ cntl->response_attachment().append(MakeDeeplyNestedJson(140000));
+ }
+};
+
+TEST(NamingServiceTest, discovery_deeply_nested_reply) {
+ GFLAGS_NAMESPACE::FlagSaver flags_saver;
+ ScopedDiscoveryChannelReset reset_discovery_channel;
+ brpc::Server server;
+ DeepReplyDiscoveryServiceImpl svc;
+ std::string rest_mapping =
+ "/discovery/nodes => Nodes, "
+ "/discovery/fetchs => Fetchs";
+ ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
+ rest_mapping.c_str()));
+ ASSERT_EQ(0, server.Start(0, nullptr));
+ brpc::policy::FLAGS_discovery_api_addr = butil::string_printf(
+ "http://%s/discovery/nodes";, butil::endpoint2str(
+ server.listen_address()).c_str());
+
+ const std::string server_address =
+ butil::endpoint2str(server.listen_address()).c_str();
+ std::string nodes_result(s_nodes_result);
+ const size_t server_address_pos = nodes_result.find("127.0.0.1:8635");
+ ASSERT_NE(std::string::npos, server_address_pos);
+ nodes_result.replace(server_address_pos, strlen("127.0.0.1:8635"),
+ server_address);
+ svc.SetNodesResult(nodes_result);
+
+ // The deeply nested reply must be rejected instead of crashing on
+ // unbounded recursion.
+ brpc::policy::DiscoveryNamingService dcns;
+ std::vector<brpc::ServerNode> servers;
+ ASSERT_EQ(-1, dcns.GetServers("admin.test", &servers));
+ EXPECT_TRUE(servers.empty());
+}
+
class NacosNamingServiceImpl : public test::NacosNamingService {
public:
void Login(google::protobuf::RpcController* cntl_base,
@@ -950,4 +1054,40 @@ TEST(NamingServiceTest, nacos) {
}
}
+class DeepReplyNacosNamingServiceImpl : public NacosNamingServiceImpl {
+public:
+ void List(google::protobuf::RpcController* cntl_base,
+ const test::HttpRequest*,
+ test::HttpResponse*,
+ google::protobuf::Closure* done) override {
+ brpc::ClosureGuard done_guard(done);
+ brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
+ cntl->response_attachment().append(MakeDeeplyNestedJson(140000));
+ }
+};
+
+TEST(NamingServiceTest, nacos_deeply_nested_reply) {
+ GFLAGS_NAMESPACE::FlagSaver flags_saver;
+ brpc::Server server;
+ DeepReplyNacosNamingServiceImpl svc;
+ ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
+ "/nacos/v1/auth/login => Login, "
+ "/nacos/v1/ns/instance/list => List"));
+ ASSERT_EQ(0, server.Start(0, nullptr));
+
+ brpc::policy::FLAGS_nacos_address = butil::string_printf(
+ "http://%s";, butil::endpoint2str(server.listen_address()).c_str());
+ brpc::policy::FLAGS_nacos_username = "nacos";
+ brpc::policy::FLAGS_nacos_password = "nacos";
+
+ // The deeply nested reply must be rejected instead of crashing on
+ // unbounded recursion.
+ const char* service_name =
+ "serviceName=test&groupName=g1&namespaceId=n1&clusters=wx";
+ brpc::policy::NacosNamingService nns;
+ std::vector<brpc::ServerNode> nodes;
+ ASSERT_EQ(-1, nns.GetServers(service_name, &nodes));
+ EXPECT_TRUE(nodes.empty());
+}
+
} //namespace
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
