Skip to content

admin: Fix crashes on invalid FieldMasks #16979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zuercher merged 6 commits into from
Jun 23, 2021
Merged

admin: Fix crashes on invalid FieldMasks #16979

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
87af1e7
Fix crash on invalid FieldMask
paul-r-gall Jun 14, 2021
316a5b2
check content type
paul-r-gall Jun 16, 2021
491d696
prevalidate FieldMask and update headers
paul-r-gall Jun 21, 2021
6761dd2
add nosniff header
paul-r-gall Jun 21, 2021
6101d83
apply fixes
paul-r-gall Jun 22, 2021
4b95053
fix formatting
paul-r-gall Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions source/server/admin/config_dump_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@ namespace Envoy {
namespace Server {

namespace {

//
Comment thread
paul-r-gall marked this conversation as resolved.
Outdated
bool CheckFieldMaskAndTrimMessage(const Protobuf::FieldMask& field_mask,
Protobuf::Message& message) {
for (const auto& path : field_mask.paths()) {
if (!ProtobufUtil::FieldMaskUtil::GetFieldDescriptors(message.GetDescriptor(), path, nullptr)) {
return false;
}
}
ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, &message);
return true;
}

// Apply a field mask to a resource message. A simple field mask might look
// like "cluster.name,cluster.alt_stat_name,last_updated" for a StaticCluster
// resource. Unfortunately, since the "cluster" field is Any and the in-built
Expand All @@ -31,7 +44,7 @@ namespace {
// this to allow arbitrary indexing through Any fields. This is pretty
// complicated, we would need to build a FieldMask tree similar to how the C++
// Protobuf library does this internally.
void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) {
bool trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) {
Comment thread
paul-r-gall marked this conversation as resolved.
const Protobuf::Descriptor* descriptor = message.GetDescriptor();
const Protobuf::Reflection* reflection = message.GetReflection();
// Figure out which paths cover Any fields. For each field, gather the paths to
Expand Down Expand Up @@ -87,13 +100,14 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag
inner_message.reset(dmf.GetPrototype(inner_descriptor)->New());
MessageUtil::unpackTo(any_message, *inner_message);
// Trim message.
ProtobufUtil::FieldMaskUtil::TrimMessage(inner_field_mask, inner_message.get());
if (!CheckFieldMaskAndTrimMessage(inner_field_mask, *inner_message))
return false;
// Pack it back into the Any resource.
any_message.PackFrom(*inner_message);
reflection->MutableMessage(&message, any_field)->CopyFrom(any_message);
}
}
ProtobufUtil::FieldMaskUtil::TrimMessage(outer_field_mask, &message);
return CheckFieldMaskAndTrimMessage(outer_field_mask, message);
}

// Helper method to get the resource parameter.
Expand Down Expand Up @@ -129,11 +143,21 @@ Http::Code ConfigDumpHandler::handlerConfigDump(absl::string_view url,
if (resource.has_value()) {
auto err = addResourceToDump(dump, mask, resource.value(), include_eds);
if (err.has_value()) {
response_headers.addReference(Http::Headers::get().XContentTypeOptions,
Http::Headers::get().XContentTypeOptionValues.Nosniff);
response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Text);
response.add(err.value().second);
return err.value().first;
}
} else {
addAllConfigToDump(dump, mask, include_eds);
auto err = addAllConfigToDump(dump, mask, include_eds);
if (err.has_value()) {
response_headers.addReference(Http::Headers::get().XContentTypeOptions,
Http::Headers::get().XContentTypeOptionValues.Nosniff);
response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Text);

@jmarantz jmarantz Jun 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry can you also add

   response_headers.addReference(
       Http::Headers::get().XContentTypeOptions,
       Http::Headers::get().XContentTypeOptionValues.Nosniff);

Just a little paranoia to make sure browsers don't flip into HTML mode if some text looks like HTML. Just a bit of paranoia based on past browser behavior :)

@paul-r-gall paul-r-gall Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

response.add(err.value().second);
return err.value().first;
Comment thread
paul-r-gall marked this conversation as resolved.
Outdated
}
}
MessageUtil::redact(dump);

Expand Down Expand Up @@ -176,7 +200,11 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump,
if (mask.has_value()) {
Protobuf::FieldMask field_mask;
ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask);
trimResourceMessage(field_mask, msg);
if (!trimResourceMessage(field_mask, msg)) {
return absl::optional<std::pair<Http::Code, std::string>>{std::make_pair(
Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(),
" could not be successfully used."))};
}
}
auto* config = dump.add_configs();
config->PackFrom(msg);
Expand All @@ -191,9 +219,10 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump,
std::make_pair(Http::Code::NotFound, fmt::format("{} not found in config dump", resource))};
}

void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
const absl::optional<std::string>& mask,
bool include_eds) const {
absl::optional<std::pair<Http::Code, std::string>>
ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
const absl::optional<std::string>& mask,
bool include_eds) const {
Envoy::Server::ConfigTracker::CbsMap callbacks_map = config_tracker_.getCallbacksMap();
if (include_eds) {
// TODO(mattklein123): Add ability to see warming clusters in admin output.
Expand All @@ -213,12 +242,17 @@ void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask);
// We don't use trimMessage() above here since masks don't support
// indexing through repeated fields.
ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, message.get());
if (!CheckFieldMaskAndTrimMessage(field_mask, *message)) {
return absl::optional<std::pair<Http::Code, std::string>>{std::make_pair(
Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(),
" could not be successfully used."))};
}
}

auto* config = dump.add_configs();
config->PackFrom(*message);
}
return absl::nullopt;
}

ProtobufTypes::MessagePtr ConfigDumpHandler::dumpEndpointConfigs() const {
Expand Down
5 changes: 3 additions & 2 deletions source/server/admin/config_dump_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ class ConfigDumpHandler : public HandlerContextBase {
Buffer::Instance& response, AdminStream&) const;

private:
void addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
const absl::optional<std::string>& mask, bool include_eds) const;
absl::optional<std::pair<Http::Code, std::string>>
addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, const absl::optional<std::string>& mask,
bool include_eds) const;
/**
* Add the config matching the passed resource to the passed config dump.
* @return absl::nullopt on success, else the Http::Code and an error message that should be added
Expand Down
50 changes: 48 additions & 2 deletions test/server/admin/config_dump_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,10 +578,10 @@ TEST_P(AdminInstanceTest, ConfigDumpNonExistentMask) {
]
}
)EOF";
EXPECT_EQ(Http::Code::OK,
EXPECT_EQ(Http::Code::BadRequest,
getCallback("/config_dump?resource=static_clusters&mask=bad", header_map, response));
std::string output = response.toString();
EXPECT_EQ(expected_json, output);
EXPECT_THAT(output, testing::HasSubstr("could not be successfully used"));
}

// Test that a 404 Not found is returned if a non-existent resource is passed in as the
Expand Down Expand Up @@ -611,5 +611,51 @@ TEST_P(AdminInstanceTest, ConfigDumpResourceNotRepeated) {
getCallback("/config_dump?resource=version_info", header_map, response));
}

TEST_P(AdminInstanceTest, InvalidFieldMaskWithResourceDoesNotCrash) {
Buffer::OwnedImpl response;
Http::TestResponseHeaderMapImpl header_map;
auto clusters = admin_.getConfigTracker().add("clusters", [] {
auto msg = std::make_unique<envoy::admin::v3::ClustersConfigDump>();
auto* static_cluster = msg->add_static_clusters();
envoy::config::cluster::v3::Cluster inner_cluster;
inner_cluster.add_transport_socket_matches()->set_name("match1");
inner_cluster.add_transport_socket_matches()->set_name("match2");
static_cluster->mutable_cluster()->PackFrom(inner_cluster);
return msg;
});
EXPECT_EQ(Http::Code::BadRequest,
getCallback(
"/config_dump?resource=static_clusters&mask=cluster.transport_socket_matches.name",
Comment thread
paul-r-gall marked this conversation as resolved.
header_map, response));
EXPECT_EQ("FieldMask paths: \"cluster.transport_socket_matches.name\"\n could not be "
"successfully used.",
response.toString());

@jmarantz jmarantz Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also make sure that headerMap.ContentTtype == "text/plain" here? If that was somehow 'text/html' then the error message could contain an XSS.

I think there's a ContentTypeValues.XXX value for that.

@paul-r-gall paul-r-gall Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

EXPECT_EQ(header_map.ContentType()->value().getStringView(),
Http::Headers::get().ContentTypeValues.Text);
EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(),
Http::Headers::get().XContentTypeOptionValues.Nosniff);
}

TEST_P(AdminInstanceTest, InvalidFieldMaskWithoutResourceDoesNotCrash) {
Buffer::OwnedImpl response;
Http::TestResponseHeaderMapImpl header_map;
auto bootstrap = admin_.getConfigTracker().add("bootstrap", [] {
auto msg = std::make_unique<envoy::admin::v3::BootstrapConfigDump>();
auto* bootstrap = msg->mutable_bootstrap();
bootstrap->mutable_node()->add_extensions()->set_name("ext1");
bootstrap->mutable_node()->add_extensions()->set_name("ext2");
return msg;
});
EXPECT_EQ(Http::Code::BadRequest,
getCallback("/config_dump?mask=bootstrap.node.extensions.name", header_map, response));
Comment thread
paul-r-gall marked this conversation as resolved.
EXPECT_EQ("FieldMask paths: \"bootstrap.node.extensions.name\"\n could not be "
"successfully used.",
response.toString());

@jmarantz jmarantz Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: check for content-type: text/plain

EXPECT_EQ(header_map.ContentType()->value().getStringView(),
Http::Headers::get().ContentTypeValues.Text);
EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(),
Http::Headers::get().XContentTypeOptionValues.Nosniff);
}

} // namespace Server
} // namespace Envoy